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

Prevent auto_register in Server from being wrongly set to False #445

Merged
merged 2 commits into from May 2, 2021
Merged

Prevent auto_register in Server from being wrongly set to False #445

merged 2 commits into from May 2, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 30, 2021

Resolves #441

The attribute auto_register in Server and its inherited classes is used to register/unregister the Server to a registry.
If set to True, it will :
1. set itself to False in the _register() method
2. spawn _bg_register() in the _register() method
3. call registrar.unregister() in the close() method

Actions 1 and 3 are conflicting because 1 will prevent 3 from ever happening.
By removing 1, the auto_register mechanism will work correctly, without any side-effects because it is not called anywhere else.

Test case : I haven't had time to add one yet, but if necessary I will look into it.

@comrumino comrumino self-requested a review May 2, 2021 06:51
@comrumino
Copy link
Collaborator

comrumino commented May 2, 2021

The commit that introduced the conflicting steps was 3a3d0ec5. I can't see any obvious reason the changes made so far would be problematic.

@comrumino comrumino marked this pull request as ready for review May 2, 2021 07:26
@comrumino
Copy link
Collaborator

Since there isn't existing testing for auto_register, I'm going to go ahead and merge this. Thanks!

@comrumino comrumino merged commit e0370d4 into tomerfiliba-org:master May 2, 2021
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

Successfully merging this pull request may close these issues.

Server doesn't unregister on close() when using auto_register=True
1 participant