PARQUET-1368: ParquetFileReader should close its input stream for the failure in constructor#510
Conversation
HyukjinKwon
changed the title
| this.footer = readFooter(file, options, f, converter); | ||
| try { | ||
| this.footer = readFooter(file, options, f, converter); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Why catch the exception and rethrowing it instead of closing the stream in finally?
There was a problem hiding this comment.
In that case, this will always be closed.
There was a problem hiding this comment.
Ah, ok, sorry. Next time I should read the comment in the PR before asking silly questions.
By the way, isn't this a problem in the other constructors in this class, and shouldn't we extend the scope of this try-catch until the end of the constructor? I guess if filterRowGroups throws an exception for some reason, then it could result in unclosed streams too no?
There was a problem hiding this comment.
Ah, it's fine. It took me a while before I reply since that's a common pattern.
I was thinking about that too. Can the same thing happen in other constructors? I thought other constructors are fine since other constructors look not directly using the stream and deprecated.
There was a problem hiding this comment.
I'm not sure if any exception could happen in other constructors, but in my opinion in this case it would make sense to wrap the entire constructor body in try-catch blocks like you did. @gszadovszky what do you think?
There was a problem hiding this comment.
I don't think any other code paths need this change, unless I missed something.
I'm not following the logic of expanding the try-catch blocks. I'm going to go ahead and merge this. If we want to follow up with more fixes, lets do that in other issues. Thanks, everyone!
There was a problem hiding this comment.
I agree with Nandor. Creating a Closable object in a constructor means that if anything throws an exception (a RuntimeException possibly) in the rest of the constructor, the Closable won't be closed.
There was a problem hiding this comment.
Makes sense. Do you want to follow this up with a patch to expand the try/catch?
|
Let me also cc @rdblue as well who's also active in Spark side. |
|
+1 |
|
Thanks guys! |
|
Thank you, @HyukjinKwon ! |
… failure in constructor (apache#510)
5 participants
This PR proposes to close the stream open in ParquetFileReader's constructor when it throws an error when it is used (
readFooter), which causes a resource leak. Otherwise, looks there's no way to close it outside.For more details, please see the JIRA ticket https://issues.apache.org/jira/browse/PARQUET-1368.