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

StompSubProtocolHandler responds with ERROR (Session closed.) on DISCONNECT when using SimpleBrokerMessageHandler [SPR-14568] #19137

Closed
spring-projects-issues opened this issue Aug 8, 2016 · 6 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Aug 8, 2016

Gottfried Huber opened SPR-14568 and commented

I think this was introduced with #16893

StompSubProtocolHandler.getStompHeaderAccessor converts a DISCONNECT_ACK to an ERROR message.

This has the effect that on the client side, the error-callback is called instead of the expected disconnect-callback, which leads to some very dirty workarounds for keeping the state sane.

This only happens when using SimpleBrokerMessageHandler. With the StompBrokerRelayMessageHandler, the MessageHeaderAccessor is already instanceof StompHeaderAccessor, and I don't get into the else if clause where the ERROR thing happens.

I would expect to see -StompHeaderAccessor.create(StompCommand.DISCONNECTED)- a RECEIPT frame response according to the receipt header of the DISCONNECT frame


Affects: 4.2.7, 4.3.1

Issue Links:

  • #19133 SockJS heartbeat is causing application send Message to fail similar to (SPR-14356)
  • #16893 Allow server-side code to send DISCONNECT messages to the broker

Referenced from: commits cc33bfa, 8b4f60b, 198a74d

Backported to: 4.2.8

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 25, 2016

Rossen Stoyanchev commented

When the client sends a DISCONNECT it also closes the WebSocket connection immediately after, or at least that's what stomp.js does. It is surprising that the ERROR frame from the server actually makes it to the client before the connection is closed. Perhaps it happens on occasion? That said I see your point that the error callback should not be possible to occur on disconnect.

The intent of #16893 was to allow disconnecting a session from the server side. In that case sending an ERROR frame seems still justified to notify the client that the server closed the session. My plan is to update handleDisconnect so that it sends a DISCONNECT_ACK only if the disconnect does not have a client session id (i.e. originated from the server side).

That should address your issue but my only concern is I don't quite follow your point about the RECEIPT frame as the simple broker does not support RECEIPT frames.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 25, 2016

Gottfried Huber commented

Alright I upgraded to the 3.0.0 release of stompjs on this fork repo https://github.com/ThoughtWire/stomp-websocket, the author of the original one (jmesnil) retired from the project. I really am not sure if this is the official successor. Or if there is such a thing.

This version explicitly sets the receipt header on the DISCONNECT frame and will not close the socket before receiving the RECEIPT. For version 3.0.0 they added STOMP 1.2 support, maybe this is the reason for this change.

I know Spring says that the simple broker is not meant for production. It's just so much more convenient to deploy and efficient, and totally does the job for our needs (we do not require load balancing for most of our single page web applications).

Maybe the simple broker should not claim to support STOMP 1.2 in the CONNECTED frame? Then I could treat disconnection differently, e.g. force close the socket without waiting for a response. I think receipt for DISCONNECT was introduced with 1.1, so it would have to be 1.0. (I've implemented it in a GWT based framework, not a single Application, so it should work no matter what broker the developer decides to use)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 26, 2016

Rossen Stoyanchev commented

Okay thanks for the additional detail. There is no official successor that I know of. There is also https://github.com/JSteunou/webstomp-client based on the discussion under issue #21 but indeed the fork you're using seems to enforce the use of a receipt id with DISCONNECT. That's a bit too opinionated IMO and there is nothing in the spec that mandates this, version 1.2 has very minor changes.

At any rate general support for RECEIPT in the simple broker could be a separate ticket for 5.0. We can address this issue by using the DISCONNECT_ACK and turning it into a RECEIPT if the DISCONNECT came with a receipt allowing the client to close the connection and such a fix can be ported back to 4.2.x.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 29, 2016

Rossen Stoyanchev commented

I've added support for DISCONNECT with a receipt. If you could confirm the fix works for you with 4.3.3.BUILD-SNAPSHOT that would be great. Thanks!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2016

Gottfried Huber commented

Just tested it, it works! I get the receipt frame on disconnect! Thanks a lot!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 30, 2016

Rossen Stoyanchev commented

Thanks for reporting and verifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants