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

Move server socket teardown to an atexit handler #155

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

samipfjo
Copy link

Fix for #146

bjoern.py Show resolved Hide resolved
@jonashaag
Copy link
Owner

I see! Thanks for the fix!

I wonder what happens if you invoke .run() multiple times in the same Python interpreter. Not sure how atexit behaves. Also the teardown is done only when the interpreter exits, shouldn't we close the socket as soon as possible? Or better even, do both, i.e. have a teardown in a finally block as before this patch and an atexit handler?

We'll also have to check if the issue reported in #146 is even reproducible anymore with the fix in #91 – and then we'll also have to re-check when we make any changes resulting from the discussion in #91

@samipfjo
Copy link
Author

I don't see any commited code referenced in #91, only discussion; am I missing something?

@samipfjo
Copy link
Author

As for calling run() multiple times, that's handled before the atexit register happens (it would throw a RuntimeError).

@samipfjo
Copy link
Author

samipfjo commented May 18, 2019

I just reviewed the atexit docs and realized there's a problem with my fix.

The functions registered via this module are not called when the program is killed by a signal not handled by Python, when a Python fatal internal error is detected, or when os._exit() is called.

Replacing the try/except block that my original commit removed and making the except clause call _close_server_socket() would fix this oversight.

Additionally, I'm not super well read into Python's reference counting implementation, but if it doesn't play well with atexit it may be wise to pass sock as an argument to _close_server_socket() instead of relying on the _default_instance global existing. I'm 99% sure that the implementation as it stands will work fine, as I trust that Python handles this, but that 1% of paranoia is still there.

@jonashaag
Copy link
Owner

Thanks for looking that stuff up! Let's wait for the resolution of #91 before we continue here.

- Handlers registered via `atexit` do not run when an exception is thrown, so a try/finally block has been added
- Add some comments to clarify behavior so readers don't have to reference the dense documentation for `socket`.
@samipfjo
Copy link
Author

Welp, I had my head in the code and missed your most recent comment, so the fixes have been implemented regardless haha.

@danigosa danigosa mentioned this pull request Jun 15, 2019
Closed
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.

2 participants