Register existing signal handlers in simpervisor's atexit module #39
Conversation
Ensure that we do not override the existing signal handlers registered in Python
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
We check if the existing handlers for SIGINT and SIGTERM are different from default handlers. Only when they are custom handlers, we execute them in our custom handler
for more information, see https://pre-commit.ci
consideRatio
force-pushed
the
fix_signal_handler_overrides
branch
2 times, most recently
from
826a14b
to
b21f08d
Compare
|
Thank you for working this @mahendrapaipuri and sorry for the slow turnaround of review. I'm not confident this is right yet. I pushed few commits, but is this working correctly etc? I've not added a test, which i think would be good to have. I also don't feel confident on the windows parts here. |
| # call previously registered non-default Python callable handler or exit | ||
| if prev_handler: | ||
| prev_handler(signum, *args) |
There was a problem hiding this comment.
Should get review and ideally tested on windows:
- I made this
prev_handler(signum, *args) instead ofprev_handler(signum, None)`, is this right?
There was a problem hiding this comment.
*args will be frame argument. Not sure if it works on windows though. I think we can pass *args to handler.
I'm not confident this is right, but not confident on the passing None either. What is correct here?
consideRatio
force-pushed
the
fix_signal_handler_overrides
branch
2 times, most recently
from
80d7579
to
4d2e774
Compare
|
I've got access to a Windows VM with conda. It's not clear how to test this specific functionality though. |
|
@consideRatio Thanks a lot for looking into this PR. I still need to look into changes you have pushed. I dont have a lot of experience with windows and neither I have access to windows machine to test it. @manics If you have access to windows machine, we can test this as described in the issue. Do you think it is doable? |
* Test will check if registered non default handlers are called * Cast port number to int in simplehttpserver.py script Signed-off-by: Mahendra Paipuri <[email protected]>
for more information, see https://pre-commit.ci
|
@consideRatio @manics I have pushed a unit test to check if we are calling existing non default handlers. Please have a look at it when you can. Not sure if we can test this functionality on windows easily though! |
Effectively, we are checking if the existing handlers are different from the default ones and if they are not default, we execute these handlers in the signal handler. I am not sure if it is the most elegant solution but happy to get any insights!!
Fixes #38