Increased thread-safety by using Lock context manager syntax #192
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a safer and more readable way to prevent accidental missed release() calls on logical branching of code/execution flow. Functions that may not be readily converted to this format have been tagged as TODOs for potential future investigation after refactoring.
Accomplished by refactoring released lock with sleep and recursive call out of the middle of the function and instead raising an exception to the main calling register() function. This should be functionally equivalent whilst allowing for the context manager syntax, which is safer against missing release() calls.
Accomplished by refactoring released lock with sleep and recursive call out of the middle of the function and instead raising an exception to the main calling deregister() function. This should be functionally equivalent whilst allowing for the context manager syntax, which is safer against missing release() calls.
This is just cleaner code without changing much. __(de)register() no longer safely locks on its, but only their public equivalents call them anyway. We can hoist the context manager wrapping there and prevent two functions from being entirely indented by a `with ...:` statement.
…blocking() Like the other preceding commits, this should eliminate the risk of missing release() and corresponding setblocking(True) calls for this function.
…cking Since recv_loop pairs acquiring a lock and setting a socket as non-blocking together, as well as their corresponding release/blocking, combining them into a single context manager function makes it safer.
|
Thank you for this PR @f3ndot It is not only a major improvement, but it is well written and I can tell you read the Thank you again! |
tayler6000
approved these changes
Jan 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current code base makes liberal use of
Lock.acquire()andLock.release()throughout, however that syntax can suffer from missing release calls in branched logic flows. Indeed, #125 was opened due to a missing release.Thankfully Python's threading module supports the context manager protocol for Lock which bakes in a guaranteed release after the inner code runs or excepts at any time.
This PR refactors almost all existing acquired/released locks to use the context manager syntax instead, removing the risk of a missing release. A small amount of refactoring was required to deal with cases where some functions intentionally release their lock in the middle of their code to sleep and recursively call themselves.
One utility function was added to simplify where the main receive loop function paired a Lock's locking/releasing action to a corresponding socket's nonblocking/blocking state. This also guarantees both are "toggled" in all possible branched flows too.
This PR passes
pytest --check-functionalitywith the test Docker image running, but I've not tried it in a project yet. I suggest @tayler6000 gives it a good once-over and testing in a real application context before considering it for merging. I'm most interested in testing the "sleep 5 seconds, try (de)registering again" logic to see if that's preserved.