Skip to content

remove false comment about try-with-resources statement in StompSubProtocolHandler#35053

Closed
Pankraz76 wants to merge 3 commits intospring-projects:mainfrom
Pankraz76:fix-StompSubProtocolHandler
Closed

remove false comment about try-with-resources statement in StompSubProtocolHandler#35053
Pankraz76 wants to merge 3 commits intospring-projects:mainfrom
Pankraz76:fix-StompSubProtocolHandler

Conversation

@Pankraz76
Copy link
Copy Markdown

No description provided.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 14, 2025
@Pankraz76 Pankraz76 force-pushed the fix-StompSubProtocolHandler branch from b0f0f9a to 547a0dc Compare June 14, 2025 09:09
@Pankraz76 Pankraz76 marked this pull request as ready for review June 14, 2025 09:09
@@ -399,8 +399,6 @@ private void sendErrorMessage(WebSocketSession session, Throwable error) {
headerAccessor.setMessage(error.getMessage());

byte[] bytes = this.stompEncoder.encode(headerAccessor.getMessageHeaders(), EMPTY_PAYLOAD);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

primitive data type is not implementing interface autocloseable, seems impossible to use try-with-resources, right?.

not possible in L:413 ether, so idk if we still need this comment, as misleading.

image

@bclozel
Copy link
Copy Markdown
Member

bclozel commented Jun 14, 2025

I don't understand the goal of this PR. The WebSocketSession is not a primitive type and the comment is very much on point. We want IO exceptions to be thrown during writing, but we want to silently swallow the ones happening during close. Again, this link should tell you more about this pattern.

@bclozel bclozel closed this Jun 14, 2025
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 14, 2025
@Pankraz76
Copy link
Copy Markdown
Author

can we merge test then at least to improve cover?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: declined A suggestion or change that we don't feel we should currently apply

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants