fix memory leak of job cancellation contexts #243
Merged
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
bgentry
force-pushed
the
bg-fix-job-cancel-context-memory-leak
branch
from
36b2cb6 to
36aa883
Compare
When remote job cancellation was added, a new cancellable context was allocated within the producer before the executor is spawned. The cancel func here was only called if the job was actually cancelled remotely or via a parent context cancellation, meaning we would slowly leak memory for every job worked that wasn't cancelled. Thank you @brandur for pinpointing the issue. Fixes #239. Co-Authored-By: Brandur Leach <brandur@brandur.org>
bgentry
force-pushed
the
bg-fix-job-cancel-context-memory-leak
branch
from
36aa883 to
aa7e128
Compare
Builds on the rest of #243 with a few things I'd done in my own version of the fix: * Add a test case that verifies that that the executor's cancel function was called even in the event of no explicit job cancellation. * Add a default context cancellation error that's never user visible, but which can easily be recognized for testing purposes.
brandur
approved these changes
Mar 1, 2024
There was a problem hiding this comment.
Looks great, thanks!!
Happily, the fix I'd written looks almost identical to yours. I pushed one more test case into your branch that I'd written this morning that checks that the context was cancelled even if the executor itself was not explicitly cancelled.
brandur
added a commit
that referenced
this pull request
Mar 1, 2024
Prepares `v0.0.24`, a small release, but one which contains an important fix from #243 which resolves a context memory leak that'd cause River's memory usage to bloat over time.
Merged
brandur
added a commit
that referenced
this pull request
Mar 1, 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.
When remote job cancellation was added, a new cancellable context was allocated within the producer before the executor is spawned. The cancel func here was only called if the job was actually cancelled remotely or via a parent context cancellation, meaning we would slowly leak memory for every job worked that wasn't cancelled.
In my recent testing, the memory usage of my sample program is now stable. After churning through >100k jobs with pprof running, it's still cruising at ~17MB usage.
Thank you @brandur for pinpointing the issue, and @shawnstephens for getting us to look into it 🙏
Fixes #239.