use try-with-resources statement in ResourceHttpMessageConverter
#35048
+125
−26
Conversation
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
Pankraz76
commented
Jun 13, 2025
| } | ||
| catch (FileNotFoundException ex) { | ||
| // ignore, see SPR-12999 | ||
| catch (Throwable ignored) { |
There was a problem hiding this comment.
Suggested change
| catch (Throwable ignored) { | |
| catch (Throwable | NullPointerException | FileNotFoundException ignored) { |
thats whats going on, right? @bclozel
Pankraz76
commented
Jun 13, 2025
| in.transferTo(out); | ||
| out.flush(); | ||
| } | ||
| catch (NullPointerException ex) { |
There was a problem hiding this comment.
NPE is ignored by try-with-resource.
Pankraz76
commented
Jun 13, 2025
| } | ||
| finally { | ||
| try { | ||
| in.close(); |
There was a problem hiding this comment.
will be called anyways by try-with-resource.
Pankraz76
commented
Jun 13, 2025
| in.close(); | ||
| } | ||
| catch (Throwable ex) { | ||
| // ignore, see SPR-12999 |
There was a problem hiding this comment.
yes we still ignore but in 1 catch not in 3.
first 2 are now handled by try-with-resource.
spring-projects-issues
added
the
status: waiting-for-triage
Signed-off-by: Vincent Potucek <[email protected]>
Pankraz76
force-pushed
the
fix-ignored-exceptions-fix-try-with-resources
branch
from
07e6101 to
a143254
Compare
Pankraz76
commented
Jun 13, 2025
| @@ -156,5 +156,123 @@ public void writeContentInputStreamThrowingNullPointerException() throws Excepti | |||
|
|
|||
| assertThat(outputMessage.getHeaders().getContentLength()).isEqualTo(0); | |||
| } | |||
| @Test | |||
There was a problem hiding this comment.
Suggested change
| @Test | |
| @Test |
checkstyle solution seems broken and costly to obtain such a custom tool not working. better use spot.
Pankraz76
commented
Jun 13, 2025
| @@ -147,31 +145,14 @@ protected boolean supportsRepeatableWrites(Resource resource) { | |||
|
|
|||
|
|
|||
| protected void writeContent(Resource resource, HttpOutputMessage outputMessage) | |||
| throws IOException, HttpMessageNotWritableException { | |||
|
|
|||
| // We cannot use try-with-resources here for the InputStream, since we have | |||
There was a problem hiding this comment.
its seems to be just a try catch around the resource operation.
BUILD SUCCESSFUL
Pankraz76
changed the title
try-with-resources statement in ResourceHttpMessageConverter
bclozel
added
status: declined
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.
before:
now: