race condition when polling and upgrading

[ruxkor]

if there is an ongoing polling request and the client is not receiving any data, the upgrade blocks until the client gets a response on the poll, usually a ping packet. this makes the usage of websockets difficult, if the server is not continously sending messages.

this behavior can be observed more often if connecting to a remote server; it almost never happens if connecting via localhost in my tests.

tracebacks:

first part:

debug  : polling
debug  : xhr poll
debug  : sending xhr with url http://.../engine.io?uid=944483157247398450060002506&transport=polling&sid=7954197634546385 | data null
debug  : probe transport "websocket" opened - pinging
debug  : probe transport "websocket" pong - upgrading
debug  : pausing current transport "polling"
debug  : we are currently polling - waiting to pause

(after 20 seconds)

debug  : polling got 1:2
debug  : socket receive: type "ping" | data "undefined"
debug  : pre-pause polling complete
debug  : paused
debug  : changing transport and sending upgrade packet
debug  : clearing existing transport

[rauchg]

One thing we could do is have polling send a noop packet when websocket connects to speed up the poll cycle.

[rauchg]

It also occurred to me that an elegant way to handle this is to fire events on the current transport when another potential upgrade transport connects.

[ruxkor]

I am still not able to consistently reproduce this behavior. We were able to fix this by having the server send a ping packet after a successful probe was received.

Obviously, this is not The Right Way to do things, but it helps to close an opening poll transport in a reasonable amount of time. You can find the changes for this in my branch (I didnt request a pull because I hope somebody will provide a better solution for this)

[rauchg]

What do you think about sending a noop packet to the open transport after a probe connection is established?

[ruxkor]

I am not sure if this is the right approach:

To give a context, I am posting a part of the maybeUpgrade function of the engine.io server. Please note the 'PLACEHOLDER' comment

Socket.prototype.maybeUpgrade = function (transport) {
  // [...]
  var self = this;
  function onPacket (packet) {
    if ('ping' == packet.type && 'probe' == packet.data) {
      transport.send([{ type: 'pong', data: 'probe' }]);
      /* PLACEHOLDER */
    } else if ('upgrade' == packet.type) {
      // [...]
    } else {
      transport.close();
    }
  }
  transport.on('packet', onPacket);
};

At first, I thought the issue could be solved be directly sending some information on the 'not-probed' transport.
something like self.sendPacket('ping','post-probe') The problem is, that sometimes the self.transport was not in a state to be able to send something --> Error

Same thing goes for self.transport.send([]); (well, the Socket calls it in flush, so it was clear it would fail too).

I "resolved" the issue by changing the ping function to accept also a custom pingInterval. Everything in the range from 500ms to 2000ms works consistently. But, as I said before, I doubt that specific time-based intervals are a good approach for this.

Maybe some kind of serialization would help on the client though? Something like "probe only when a poll connection is established", so it is guaranteed there is a poll connection going to the server, that can then be closed? Only thinking aloud here though, but I'd love to see some good ideas we could try to implement.

[rauchg]

The key is to queue the packet on the Socket instead of trying to write it directly to the Transport (keep in mind that socket does the buffering, not the transport). And we also probably want to add a noop type to the protocol parser in engine.io-client.

[rauchg]

Fixed in engine.io