[lumina] Release the vector file when FileBackedDataset fails to construct - #9481
Open
PDGGK wants to merge 1 commit into
Open
[lumina] Release the vector file when FileBackedDataset fails to construct#9481PDGGK wants to merge 1 commit into
PDGGK wants to merge 1 commit into
Conversation
…truct FileBackedDataset opens a RandomAccessFile and then validates the record size and allocates the read buffer, either of which can throw. A caller using try-with-resources never receives an object whose constructor threw, so nothing closes that handle. The enclosing writer already guards its own file this way; this brings the nested reader in line.
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
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.
Purpose
FileBackedDatasetopens the vector file and then does work that can fail:checkedRecordSizethrowsIllegalStateExceptionwhen a record does not fit the read buffer, andallocateDirectcan fail on its own. Both run after the file is open.Both call sites use try-with-resources:
but try-with-resources never receives an object whose constructor threw, so its
close()never runs and the handle is stranded.The enclosing writer already guards its own file this way at
:112, so this brings the nested reader in line.Tests
LuminaFileBackedDatasetCloseTest#testFailedConstructionReleasesTheFileruns 200 failed constructions with a dimension the buffer cannot hold, counting entries in/dev/fdbefore and after. It skips itself where that directory is not available.Reverting the change turns it red:
[200 failed constructions must not strand 200 descriptors].A note on how the assertion got there, since the obvious versions do not work. Asserting that the exception is raised passes either way. Letting the descriptors run out does not work either — the
RandomAccessFilefinalizer releases them under GC pressure, so twenty thousand leaked handles still never exhaust the limit and the test stays green with the bug in place. Counting open descriptors directly is what actually separates the two.mvn test -pl paimon-luminapasses; spotless and checkstyle clean.