-
Notifications
You must be signed in to change notification settings - Fork 11.1k
Fix resource leak in FileBackedOutputStream to prevent file handle exhaustion #7986
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
Closed
+96
−1
Closed
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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 file contains hidden or 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
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a short summary of what the new tests cover beyond what the existing
testThresholdtests cover? I'm sure I can work it out, but you may know offhand :)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new tests are regression tests that were added alongside the resource management fix to verify normal operation works correctly.
testThresholdCrossing_ResourceManagement()tests normal threshold crossing behavior through the memory-to-file transitiontestMultipleThresholdCrossings()tests normal operation across repeated threshold crossingsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that threshold crossing in some sense is covered by
testThreshold(), but it looks like that covers only the case in which we have written exactly the number of bytes that fit before and then write some new bytes that immediately trigger the use of the file. Your new test writes a smaller number of bytes and then writes a large number at once, enough to take us from "not at the threshold" to "over the threshold." So that seems like a new thing.For "multiple threshold crossings," I don't think that's the phrasing I'd use, but your test is covering a write that occurs after the write that crosses the threshold. I don't think we cover that yet, except in the
singleBytecase, which doesn't take the same code path as you're covering.Does that all sound right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that all sounds right!
For
testThresholdCrossing_ResourceManagement(): You've understood correctly. The existingtestThreshold()writes exactly the number of bytes that fit (reaching the threshold), then immediately writes more bytes. My new test covers a different scenario: writing a smaller amount first (40 bytes), then writing a large amount at once (30 bytes) that takes us from "not at threshold" to "over the threshold."For
testMultipleThresholdCrossings(): This is indeed misleading phrasing. The test is actually covering writes that occur after the initial write that crosses the threshold. Once we've crossed the threshold and transitioned to file storage, this test verifies that continued writes to the file work correctly. As you noted, this scenario isn't covered except in thesingleBytecase, which takes a different code path.I'll update the PR to:
testMultipleThresholdCrossings()totestWriteAfterThresholdCrossing()testThresholdCrossing_ResourceManagement()differs from the existingtestThreshold()Thanks for the careful review and clarification!