MINOR: Close output stream when ParquetFileWriter construction fails on encryption validation#3663
Open
anxkhn wants to merge 1 commit into
Open
MINOR: Close output stream when ParquetFileWriter construction fails on encryption validation#3663anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
…on encryption validation The shared ParquetFileWriter constructor opens the output stream early (via OutputFile.create/createOrOverwrite), then validates that every encrypted column named in the encryption properties exists in the file schema. When a configured encrypted column is absent, it throws ParquetCryptoRuntimeException. Because this validation runs after the stream is opened and is not guarded, the half-constructed writer is never returned to the caller, so its close() (the only place that closes the stream) can never run and the open stream leaks. A service that repeatedly builds writers with a misconfigured encryption schema accumulates leaked handles until exhaustion. Wrap the encryption setup in a try/catch that closes the already-opened stream before rethrowing, mirroring the existing guard in the ParquetFileReader constructor. Add a regression test that constructs a writer with an encrypted column missing from the schema and asserts the stream is closed on failure. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
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.
Rationale for this change
The shared private
ParquetFileWriterconstructor opens the output stream early(
OutputFile.create/createOrOverwrite), and only afterwards validates thatevery column named in the encryption properties actually exists in the file
schema. When a configured encrypted column is missing, it throws
ParquetCryptoRuntimeException.That validation runs after the stream is opened and is not guarded, so on the
throw the half-constructed writer is never returned to the caller. Its
close()is the only place that closes the stream, and it can never run, so the open
stream (and its underlying file/DFS handle and buffers) leaks. A service that
repeatedly constructs writers against a misconfigured encryption schema
accumulates leaked handles until exhaustion.
This is the same acquire-then-throw hazard the sibling
ParquetFileReaderconstructor already guards against by closing its stream before rethrowing, so
the writer should behave consistently.
What changes are included in this PR?
ParquetFileWriter, wrap the encryption-setup and encrypted-column-in-schemavalidation block (which runs after
this.outis assigned) in atry { ... } catch (Exception e) { out.close(); throw e; }. On any exceptionthe already-opened stream is closed before the exception propagates, mirroring
the existing guard in the
ParquetFileReaderconstructor. The early return forthe no-encryption case is left outside the guard (nothing to clean up there).
testConstructorClosesStreamWhenEncryptedColumnMissing,that constructs a writer with an encrypted column absent from the schema,
asserts the expected
ParquetCryptoRuntimeException, and asserts the stream wasclosed. It uses a small
RecordingOutputFilewhosePositionOutputStreamflips an
AtomicBooleanonclose(), so it needs no disk I/O.Two files change:
ParquetFileWriter.java(the guard) andTestParquetFileWriter.java(the test plus helper).Are these changes tested?
Yes.
with it. It runs parameterized (vectored true/false), 2/2 passing.
TestParquetFileWriter(38),TestParquetWriter(19),
TestEncryptionOptions(1), 0 failures / 0 errors.Commands used:
Are there any user-facing changes?
No. Behavior on the success path is unchanged; the only difference is that a
failed writer construction (invalid encrypted-column configuration) now releases
the output stream it opened instead of leaking it. No public API changes.