xhr-polling connections are never closed

[3rd-Eden]

I've found this bug while I was writing primus. When a client calls the close method a simple close type packet is send to the server. The server only calls a self.onClose function but never actually closes the accepted connection (s) causes the server.close call to hang indefinitely.

Pull request is coming.

[3rd-Eden]

Not sure if this also happening for other transports like WebSockets etc.

[rauchg]

This is not a good idea since the socket could be re-used by the browser for other requests.

[3rd-Eden]

@guille leaving the server spinning because it has unanswered requests isn't good either.

[rauchg]

CPUs won't actually be spinning, it'll just wait on I/O.

It's the responsibility of the client to sever the connection if it wants
to do so. We might want to add this behavior to engine.io-client when we
know we have access to the underlying socket (ie it's running from node).

On Wednesday, July 3, 2013, Arnout Kazemier wrote:

@guille https://github.com/guille leaving the server spinning because
it has unanswered requests isn't good either.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/178#issuecomment-20417210
.

Guillermo Rauch
Cloudup CTO
http://devthought.com

[c4milo]

@guille this PR seems legit to me. Wouldn't leaving the socket open cause more harm than good?

[rauchg]

@c4milo no because node should clean it up.

If you look at the "Hello World" of node:

you never actually close the socket yourself, and the server is keep-alive compliant out of the box.

Engine.IO shouldn't "take over" the socket necessarily. We could make it an option, for example if you plan on having a dedicated domain for engine.io

[c4milo]

Nodejs cleans it up after 2 minutes which is the timeout by default for idle sockets. If you don't close the socket and your server has too many concurrent connections, you might run out of file descriptors too soon, even if your clients are closing the connections upon disconnections. Another scenario, from of the top of my head, that might not work reliably is when you want to support presence, you might see clients connected when in reality they are not.

[mokesmokes]

@guille - perhaps I'm missing something but in this case the client sent a 'close' packet - thus the client's intent is pretty much clear - no? It just seems like the benefit of keeping the socket open versus closing it is rather small - i.e. the real-time issue pointed out by @c4milo, memory consumption, etc. Thoughts?

[rauchg]

@mokesmokes keep in mind the engine.io socket does not equal TCP socket. The browser could continue to use the TCP socket for other requests that are NOT captured by engine.io. The close packet is clear in the intent of closing the engine.io socket, which is why we fire close immediately, not when the underlying TCP socket closes.

Like I said, the improvement we could make here is to enforce the TCP close if the user desires that. But that behavior must be opt-in.

[mokesmokes]

@guille - here's a scenario: in a mobile app (both Android & iOS) - the app frequently voluntarily disconnects. For example - any time the home button is clicked, screen locked, etc. This is a requirement since any I/O in this state may crash the app. We then reconnect when the app is re-focused. So potentially there will be loads of zombie sockets hanging around. Perhaps the solution is to have the standard 'close' as you have today, but also add an additional 'hardClose' client command - which can be used when required? (e.g. the scenario described above)

[rauchg]

close(true) maybe ? (famous last words)

[mokesmokes]

I hope we have a deal ;-)

[c4milo]

close(true) looks good to me. But IMHO, true should be the default value. The reason being is that once you wrap your network stack with engine.io, the more intuitive behavior is to use it to manage your connection lifecycle as well. If a browser really wants to re-use existing connections it would do it despite your actions with the actual user facing API. Chrome, for example, has an internal pool of sockets and it does re-use them despite of what you do with the API**. Additionally, remember that not all the clients for an engine.io service are browsers.

**You can test it by yourself using chrome://net-internals/#sockets

[rauchg]

Sure but closing the socket would incur in an entire roundtrip to set it up again if the browser needs to extend the pool

[c4milo]

sure, I just don't think that default behavior is intuitive and legit for non xhr-polling cases. If anything, it will be seen as an issue instead.

[c4milo]

Anyway, I'm fine with having it as an opt-in :) Thank you @guille

[kapouer]

@guille
it's only a concern if the engine.io server is attached to http.Server, no ?
Why not opt-in when engine.listen is called, and opt-out when engine.attach is called ?

[mokesmokes]

@kapouer - it should be left up to the developer. You're right that if the code is engine.listen then the default should be true, but also in cases where it's attached to the http server it may be true (application dependent) - e.g. my scenario above (which is what I'm doing BTW).