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

Update unregister implementation to remove all references to the previously registered function #5

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

cygann
Copy link
Contributor

@cygann cygann commented Aug 17, 2023

When pyterminate unregisters a function, not all references to that function get cleared. This causes problems when the registered function holds references to objects that are expected to be deallocated after that function gets unregistered from pyterminate.

This updates the unregister implementation to:

  • Remove the exit handler wrapper of func from the _func_to_wrapper dictionary
  • If func is the actively registered signal handler, register the previous signal handler instead. In any case, for any registered signal handler whose previous handler was func, update the previous handler to func's previous.
  • Remove func from the _signal_to_prev_handlerdictionary

Natalie Cygan and others added 2 commits August 16, 2023 18:20
Corrected implementation of reference cleanup for signals and when
exiting normally.

Added tests to ensure functionality.

The case where exiting normally was tested locally but no test was added
due to higher complexity and low marginal benefit now that normal exit
behavior will call unregister and use the same codepaths.
Behavior is odd on 3.7, and there's no good reason to maintain support
for these.
@jeremyephron
Copy link
Owner

@cygann Thanks! I've added some changes to your fork to get it ready to merge. The implementation changes can be seen in 9d74749.

Essentially, your original implementation did not properly handle signal unregistration in a couple of ways (trying to register None, not unregistering if there was a default handler, ...). A bit more of the code needed to adjust to accommodate that. Additionally, it didn't handle proper deletion of references during the normal exit path where the signal or exit handler runs. The changes I added address that.

Importantly, I've also added tests to ensure this works and doesn't regress.

For some reason, Python 3.7 behaves a bit strangely (caught in the tests). I don't necessarily think this is worth the time to look into right now, so I've dropped support for Python 3.6 and 3.7, which seems appropriate given Ubuntu 20.04 is on Python 3.8.

I think this is an important improvement to the package. Merging.

@jeremyephron jeremyephron merged commit 285551e into jeremyephron:master Aug 21, 2023
9 checks passed
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.

None yet

2 participants