Core: Update TableMetadataParser to ensure all streams closed #11220
Conversation
erik-grepr
changed the title
| generator.flush(); | ||
| } | ||
| } else { | ||
| try (OutputStreamWriter osw = new OutputStreamWriter(os, StandardCharsets.UTF_8)) { |
There was a problem hiding this comment.
I am not convinced we need the two code branches for this, nor that it's desired.
OutputStream::close must obey general Closeable::close contract which requries the implementation to be idempotent.
Thus, if we had
OutputStream stream = overwrite ? outputFile.createOrOverwrite() : outputFile.create();
try (OutputStream ou = isGzip ? new GZIPOutputStream(stream) : stream;
we now should be able to:
try (OutputStream stream = overwrite ? outputFile.createOrOverwrite() : outputFile.create();
OutputStream ou = isGzip ? new GZIPOutputStream(stream) : stream;
guaranteeing all streams to be closed, regardless at which point an exception is thrown.
Can you try that?
There was a problem hiding this comment.
updated I think. Is there a gradle target to auto-format?
There was a problem hiding this comment.
thanks!
Is there a gradle target to auto-format?
./gradlew -DallModules=true spotlessApply
( see also https://gist.github.com/findepi/da2ea85d095eaa0f3cfdcbb311ff59ed#reformat-code )
|
@erik-grepr can you please review the build results? |
|
Should be good to re-run the workflows. |
findepi
changed the title
|
Merged, thanks! |
…apache#11220)" This reverts commit 2b55fef.
Minor try-with-resources tweak to fix an unclosed stream memory leak I encountered.
More specifically, this occurs when the file is Gzipped and file.newStream().read() throws an exception.
The
GZIPInputStreamconstructor calls read on it's input to load the header.see: https://stackoverflow.com/questions/12552863/correct-idiom-for-managing-multiple-chained-resources-in-try-with-resources-bloc