rabbitmq / rabbitmq-web-stomp

Provides support for STOMP over WebSockets
Other
89 stars 26 forks source link

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

Closed odysseus654 closed 8 years ago

odysseus654 commented 8 years ago

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 commented 8 years ago

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

odysseus654 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@essen thank you, I like this solution.