Skip to content

Remove unnecessary InputStream.close call #1982

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

Merged
merged 2 commits into from
Jul 4, 2025

Conversation

eric-maynard
Copy link
Contributor

#1942 changed the way that the bootstrap init script is handled, but it added an extra InputStream.close call that shouldn't be needed after the BufferedReader here is closed. This PR removes that extra call.

@snazy
Copy link
Member

snazy commented Jul 1, 2025

The current code is correct. This change leaves the risk of not closing the input stream.

@eric-maynard
Copy link
Contributor Author

eric-maynard commented Jul 1, 2025

What makes you say that, @snazy? Have you found a JDK that behaves that way?

Oracle documents BufferedReader#close as [Closing] the stream and releases any system resources associated with it. here and InputStreamReader#close the same way here. I believe this comes from the javadoc of Reader itself.

Looking at OpenJDK, the BufferedReader will close the InputStreamReader, and then InputStreamReader will close the stream.

@snazy
Copy link
Member

snazy commented Jul 1, 2025

@eric-maynard you see that a) the BufferedReader is created somewhere inside some function calls and b) that the given stream should be closed nontheless, right?

@eric-maynard
Copy link
Contributor Author

a) the BufferedReader is created somewhere inside some function calls

What function calls? It's created inside a try-with-resources which is visible in the diff here.

that the given stream should be closed nontheless, right?

That is what the try-with-resources is for

try (Statement statement = connection.createStatement();
BufferedReader reader =
new BufferedReader(
new InputStreamReader(Objects.requireNonNull(scriptInputStream), UTF_8))) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of the .close() call, I just realised that runWithinTransaction() has re-tries... So if this code is called again, the reading of the scriptInputStream will lead to invalid data. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to fix that I think we need to go back to having two try-with-resources. Still, the extra close call can be removed. Just pushed a fix.

Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for the fix @eric-maynard ! It LGTM 👍

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 2, 2025
@eric-maynard eric-maynard merged commit 7f3b781 into apache:main Jul 4, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants