Fix deadlocks and memory leaks from the watcher/watchDispatcher implementation. #129
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 prequel refactoring that helps to lure out the deadlocks in the watcher later. Signed-off-by: ttimonen <[email protected]>
Signed-off-by: ttimonen <[email protected]>
While watchDispatcher doesn't grow unboundedly anymore, it can't be torn down trivially. This is fixable, but a fix for that would leak more complexity into the storage layer. Signed-off-by: ttimonen <[email protected]>
ttimonen
force-pushed
the
ttimonen/watch2
branch
from
50034ba to
e6665bc
Compare
|
Thanks for your PR @ttimonen I'm going away for a week and will probably need a few days to fully comprehend your changes... stay tuned! |
matthyx
approved these changes
Aug 20, 2024
|
@ttimonen thanks again for your work, it took me a while to validate it but it works perfectly! |
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.
There's relatively verbose analysis in the comments on why the watch.Interface is
quite convoluted to implement in a way that doesn't deadlock. In particular adhering to both the Hyrum's law version
of the API, and the documented version of the API is highly impractical.
Also simplify the testing implementation and add test-cases for the deadlocks.
Possibly controversal part: The context lifetime propagation in the Watch() call. The API expectation on that is not obvious.
The changes are probably easiest to read one commit at a time, since they are quite self-contained.