use try-with-resources statement#35046
use try-with-resources statement#35046Pankraz76 wants to merge 1 commit intospring-projects:mainfrom
try-with-resources statement#35046Conversation
.../src/main/java/org/springframework/jms/listener/AbstractPollingMessageListenerContainer.java
Show resolved
Hide resolved
spring-projects-issues
added
the
status: waiting-for-triage
Pankraz76
force-pushed
the
use-try-with-resources-statement-
branch
from
4e2e1be to
9aa6812
Compare
Signed-off-by: Vincent Potucek <vpotucek@me.com>
Pankraz76
force-pushed
the
use-try-with-resources-statement-
branch
from
9aa6812 to
d8d4b6e
Compare
bclozel
left a comment
bclozel
left a comment
There was a problem hiding this comment.
Thanks for raising this, but as this review shows, these stylistic changes are mostly breaking the behavior or adding little value.
| continue; | ||
| } | ||
| InputStream is = cl.getResourceAsStream(c.getName().replace('.', '/') + ".class"); | ||
| if (is == null) { |
There was a problem hiding this comment.
Please don't change files in the cglib package, this part is manually shaded into our repository and we shouldn't change the source if we want to easily keep up with the original.
| Set bridges = (Set) entry.getValue(); | ||
| try { | ||
| InputStream is = classLoader.getResourceAsStream(owner.getName().replace('.', '/') + ".class"); | ||
| if (is == null) { |
| name.replace('.','/') + ".class" | ||
| ); | ||
|
|
||
| if (is == null) { |
| } | ||
| return size; | ||
| } | ||
| finally { |
There was a problem hiding this comment.
Please revert. The exception thrown while closing the stream is not caught/logged anymore.
See this message for a discussion on possible workarounds. I think we should keep the current code.
| try { | ||
| con.close(); | ||
| } | ||
| catch (SQLException ex) { |
| } | ||
| try { | ||
| handleListenerException(ex); | ||
| } |
There was a problem hiding this comment.
please revert this new line change.
| if (ex instanceof JMSException jmsException) { | ||
| throw jmsException; | ||
| } | ||
| } |
There was a problem hiding this comment.
please revert this new line change.
| TransactionSynchronizationManager.unbindResource(obtainConnectionFactory()); | ||
| } | ||
| observation.stop(); | ||
| scope.close(); |
There was a problem hiding this comment.
This is not a neutral change. The scope is now closed before the observation is stopped and the transaction finished. This will change the behavior and will have unintended consequences.
Please revert.
| in.close(); | ||
| } | ||
| catch (Throwable ex) { | ||
| // ignore, see SPR-12999 |
There was a problem hiding this comment.
Please revert. This will catch all Throwable thrown by the code block, not just when the resource is closed.
| responseHeaders.setContentLength(rangeLength); | ||
|
|
||
| InputStream in = region.getResource().getInputStream(); | ||
| // We cannot use try-with-resources here for the InputStream, since we have |
There was a problem hiding this comment.
This is literally called out in this comment...
There was a problem hiding this comment.
yes need to stay open i know this behavior from maven. should give more detail then as ignored exc for good reason.
|
Given the state of this PR, I don't think it's worth spending time on this - we're more likely to break behavior than anything. |
bclozel
added
status: declined
|
yes of course will SOC. if this changes anything its a interesting to invest, as its a valid feature used all around already in this code base: |
|
if can not use java like supposed to, it seems smell. |
Pankraz76
left a comment
Pankraz76
left a comment
There was a problem hiding this comment.
thx. for detailed review. yes this need special attention.
| responseHeaders.setContentLength(rangeLength); | ||
|
|
||
| InputStream in = region.getResource().getInputStream(); | ||
| // We cannot use try-with-resources here for the InputStream, since we have |
There was a problem hiding this comment.
yes need to stay open i know this behavior from maven. should give more detail then as ignored exc for good reason.
3 participants
Java 7 introduced the try-with-resources statement. This statement ensures that each resource is closed at the end of the statement. It avoids the need of explicitly closing the resources in a finally block. Additionally exceptions are better handled: If an exception occurred both in the try block and finally block, then the exception from the try block was suppressed. With the try-with-resources statement, the exception thrown from the try-block is preserved.
try-with-resourcesstatement openrewrite/rewrite-static-analysis#591try-with-resourcesstatement inLookupWagonMojoapache/maven#2426try-with-resourcesstatement quarkusio/quarkus#48364