Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugs in singleton and signal handling for csclient #72

Open
ftsfranklin opened this issue Jul 27, 2022 · 0 comments
Open

Bugs in singleton and signal handling for csclient #72

ftsfranklin opened this issue Jul 27, 2022 · 0 comments

Comments

@ftsfranklin
Copy link

ftsfranklin commented Jul 27, 2022

csclient registers a signal handler for SIGTERM, which cleans up EventingCSClient and then exits with 0.

def clean_up_reg(signal, frame):
"""
When 'cppython remote_port_forward.py' gets a SIGTERM, config_store_receiver.py doesn't
clean up registrations. Even if it did, the comm module can't rely on an external service
to clean up.
"""
EventingCSClient('CSClient').stop()
sys.exit(0)
signal.signal(signal.SIGTERM, clean_up_reg)

In reality, it creates a NEW EventingCSClient (with a new registry) and calls stop on it. If there is already a singleton with the exact class EventingCSClient, it will use that one instead. If there is a subclass of EventingCSClient, it will not use that one, because of how the singleton lookup works.

There is also a misunderstanding of how __init__ works. The __init__ function is called on the result of __new__ even if the object was already initialized. That means the signal handler will replace the registry before it cleans it up.

Sample code demonstrating the double init:

class A:
    _instances = {}
    def __new__(cls, *na, **kwna):
        """ Singleton factory (with subclassing support) """
        try: instance = cls._instances[cls]
        except KeyError: instance = cls._instances[cls] = super().__new__(cls)
        return instance

class B(A):
    def __init__(self, value):
        self.value = value
        print('init', value)

b0 = B('first')          # init first
b1 = B('second')         # init second
print(f"{b0 is b1 = }")  # b0 is b1 = True
print(f"{b0.value = }")  # b0.value = 'second'

If there is no object with exact class EventingCSClient, the stop() call will crash when it tries to access self.event_sock, because that is only created by the start call, not for a fresh new object.

What is the intent of this signal handler? It refers to "remote_port_forward.py" and "config_store_receiver.py", neither of which are in this repo.

Turning a SIGTERM into a SystemExit(0) changes the behavior of applications. For example, non-daemon threads can ignore SystemExit but not SIGTERM. Is that intentional?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant