Skip to content
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

Set reason for WebSocket CloseStatus.SESSION_NOT_RELIABLE #29220

Conversation

zhmaeff
Copy link
Contributor

@zhmaeff zhmaeff commented Sep 29, 2022

Providing a reason in CloseStatus gives a better understanding of the root cause on the WebSocket client side.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 29, 2022
@zhmaeff zhmaeff marked this pull request as ready for review September 29, 2022 12:17
@zhmaeff
Copy link
Contributor Author

zhmaeff commented Sep 29, 2022

Hi @rstoyanchev , could you pls review it?

@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 24, 2023
Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

Yes, providing a reason would be useful. My one concern would be to expose information to external clients that reveals the server being used. In that sense the response status isn't meant to be for debugging but more of a hint for what happened. Perhaps we could choose a a fixed reason for this case that is a bit more helpful than just the status code?

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Jan 30, 2023
@zhmaeff
Copy link
Contributor Author

zhmaeff commented Jan 30, 2023

Yes, providing a reason would be useful. My one concern would be to expose information to external clients that reveals the server being used. In that sense the response status isn't meant to be for debugging but more of a hint for what happened. Perhaps we could choose a a fixed reason for this case that is a bit more helpful than just the status code?

Hi @rstoyanchev , do you mean the same reason for both send time limit exceeded AND the buffer size limit exceeded cases?

Current impl contains the following possible reasons:
Send time x (ms) for session 'y' exceeded the allowed limit z
Buffer size x bytes for session 'y' exceeds the allowed limit z

Would reason like this be acceptable? It hides session ids, current config values but still hints possible issues.
Send time or buffer size limit exceeded

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 30, 2023
@rstoyanchev
Copy link
Contributor

Something like that, perhaps we can say "Failed to send message within the configured send limit". After all, it's hard to say whether it took too long because the network is slow, or because too many messages are sent, or combination of both. We just know that the send throughput cannot be sustained.

@zhmaeff zhmaeff force-pushed the add_reason_for_web_socket_session_close_status branch from 51c27da to af43d98 Compare January 30, 2023 21:16
@zhmaeff
Copy link
Contributor Author

zhmaeff commented Jan 30, 2023

@rstoyanchev updated according to your suggestion

@rstoyanchev rstoyanchev self-assigned this Jan 31, 2023
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jan 31, 2023
@rstoyanchev rstoyanchev added this to the 6.0.5 milestone Jan 31, 2023
@zhmaeff zhmaeff deleted the add_reason_for_web_socket_session_close_status branch February 13, 2023 13:06
mdeinum pushed a commit to mdeinum/spring-framework that referenced this pull request Jun 29, 2023
mdeinum pushed a commit to mdeinum/spring-framework that referenced this pull request Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants