Skip to content
This repository has been archived by the owner on Nov 16, 2020. It is now read-only.

web-stomp under SockJS is mis-reporting heart-beat availability #28

Closed
odysseus654 opened this issue Nov 24, 2015 · 8 comments
Closed

Comments

@odysseus654
Copy link

My understanding is that SockJS is a polyfill that will gracefully convert to a websocket on browsers that support such things. The SockJS protocol currently appears to not support heartbeats while raw websockets do. If the server misreports the availability of heart-beat, then it is up to the client to somehow determine the nature of the underlying connection and know when heartbeats are unavailable.

This is a sample STOMP request sent through SockJS with default heartbeat settings:

CONNECT
login:webclient
passcode:}a3$#w(3:E6t7Rr
host:/
accept-version:1.1,1.0
heart-beat:10000,10000

This is the server response:

session:session-q2YPI1RiSM-CShZXYSlKKQ
heart-beat:10000,10000
server:RabbitMQ/3.5.6
version:1.1

...And this is the frustration of a spurned client library

LOG: connected to server RabbitMQ/3.5.6 
LOG: send PING every 10000ms 
LOG: check PONG every 10000ms 
LOG: >>> PING 
LOG: >>> PING 
LOG: did not receive server activity for the last 20011ms 
LOG: Whoops! Lost connection to https://msg.mywebserver.com/stomp 
@michaelklishin
Copy link
Member

What is your suggestion, hardcode heartbeats for SockJS endpoints to 0,0?

@odysseus654
Copy link
Author

I will be forcing them to 0,0 in the client for the short term, but that would prevent the client from utilizing heartbeats on browsers that can make raw websockets.

Would it be difficult for either rabbitmq-web-stomp or sockjs-erlang to flag the connection as explicitly heart-beat:0,0 ?

@essen
Copy link
Contributor

essen commented Nov 24, 2015

I suspect STOMP heartbeats are disabled simply because SockJS may use more than one connection for some of its transports, and so aren't a good fit for killing SockJS connections.

Websocket doesn't have this problem; however, while SockJS over Websocket could in theory support STOMP heartbeats rather nicely, we can't easily enable them just when Websocket is used. SockJS was designed to abstract Websocket away, and the SockJS Erlang client reflects that, making such a patch harder to implement.

This would also not fix issues you pointed out on the ML about other transports. A SockJS-specific heartbeat would be a better solution.

Forcing 0,0 heartbeats on the server is easy however; I should have a patch ready during the day.

Note that in 3.6.0 we do have Websocket with STOMP heartbeats, but that's without SockJS, and on a different path (/ws instead of /sockjs).

@odysseus654
Copy link
Author

FYI, from the sockjs-client logic, these are the locations in use by the various transports:

eventSource: /eventsource
htmlfile: /htmlfile
iframe: /iframe.html
jsonp-polling: /jsonp
websocket: (wss|ws):.../websocket
xdr-polling: /xhr
xdr-streaming: /xhr_streaming
xhr-polling: /xhr
xhr-streaming: /xhr_streaming

My point with this is that it's not clear to me that there is a way for the server to tell the difference between WebSocket with SockJS vs WebSocket without SockJS. That is, unless your server differentiates between /ws and /websocket

@essen
Copy link
Contributor

essen commented Nov 24, 2015

Yes, but these are not the full paths. I was incorrect however, everything there is not under /sockjs but under /stomp. So for example you have /stomp/eventsource and /stomp/iframe.html.

The new thing we added is a plain /ws for non-SockJS clients, speaking STOMP directly on top of Websocket.

@odysseus654
Copy link
Author

Uff, you're right. Lol, so much for SockJS being a polyfill.

It would be helpful to somehow force heart-beat:0,0 on the server-side in any case.

@essen
Copy link
Contributor

essen commented Nov 26, 2015

I sent three PRs to make the server properly report a 0,0 heartbeat: the first two do the disabling, and the third one fixes the examples so they don't need to disable heartbeats manually anymore.

Ready for review/merge.

@michaelklishin
Copy link
Member

@essen thank you, I like this solution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants