Skip to content

Conversation

@samipfjo
Copy link

Fix for #146

@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