search

xmppo / node-xmpp-bosh

An XMPP BOSH & WebSocket server (connection manager) written on node.js using Javascript
https://github.com/xmppo/node-xmpp-bosh
263 stars 85 forks source link

Further stream close improvements; addition of WebSocket ping #79

Closed gavllew closed 11 years ago

gavllew commented 11 years ago

I was seeing some server restarts on occasion when a websocket client closes a connection - the XMPP server would attempt to distribute the unavailable presence, and this would get passed to the closed websocket server, throwing an uncaught exception.

In attempting to fix this, I ended up reworking a lot of the stream/connection close code - it should now be compliant with section 3.5 of draft-moffitt-xmpp-over-websocket-03 (i.e. respond to a stream close with another stream close, and let the initiator send the websocket close frame).

It also now sends a websocket ping frame every thirty seconds for each connection, and checks for the 'pong' response. This is for the following reasons:

dhruvbird commented 11 years ago

Looks good otherwise (y)

dhruvbird commented 11 years ago

I'm also curious to know if the error manifested itself as an uncaught exception or an unhandled 'error' event on the websocket-server object?

gavllew commented 11 years ago

The error was an uncaught exception. Unfortunately I don't have the logs anymore, but the exception thrown from the websocket server was "Error('not opened')" - see https://github.com/einaros/ws/blob/master/lib/WebSocket.js#L174

dhruvbird commented 11 years ago

@gavllew Thanks for the link to the error - it was an exception as you mentioned.

dhruvbird commented 11 years ago

This is the commit I'm going to merged (squashed for easy reading). Let me know if it looks good to you. 2c5cfced126211ec46cd9785704f2c620c644128

gavllew commented 11 years ago

Looks good to me, thanks!

dhruvbird commented 11 years ago

Merged as a1ab095625678dd876986cba8c252612e817b0b6 Please could you submit another request to include magic constants 30000 and 60000 (possibly 30000 * 2??) as a config. variable. See https://github.com/dhruvbird/node-xmpp-bosh/blob/master/bosh.conf.example.js for an example. Units should be second. Name could be something like websocket_ping_interval.

Update: README.md would also need to be updated.

dhruvbird commented 11 years ago

Thank you for submitting a patch. I appreciate it!

gavllew commented 11 years ago

I will try to add the config soon.