Do not hang in polling while websocket upgrade is ongoing

[salimaboubacar]

Should fix:

• Couldn't upgrade the protocol in some random cases #160
• Intermittent long delay during connection upgrade python-socketio#393

And deprecate this other PR which seems to indirectly adresses the issue (probably in a less clean way): #76

This fixes a race condition between the client side handling of the PONG packet (websocket upgrade) and the NOOP packet queued for the current long polling connexion. The behavior in the Javascript version of the server is here: https://github.com/socketio/engine.io/blob/cb0ac6fddcad12c454651bf0e1a312a154e228a4/lib/transports/polling.js#L112-L117

[miguelgrinberg]

Thanks for looking into this. I like your fix, but I'd like to understand how I can reproduce this problem so that I can then test the fix. Have you been able to reliably reproduce the issue?

[salimaboubacar]

You need a race condition to happen in the client. From what I understand in the python client, it can't happen because we do only one GET on polling anyways, and don't try to upgrade while actively polling (right ?), so no race condition is possible with the python client.

The way I reproduced was by spawning thousands of clients in the browser and detecting when one was stuck like this (not so reliable 😅). In the javascript version, we don't send the upgrade packet while there is still a GET polling ongoing: we wait for it to finish first. When a GET finishes we send another one except if we had a pong on the websocket already. The case where we get stuck is if the handler of the GET (receiving the noop) is called before the handler of the pong on websocket: the handler of the GET will send another GET and the handler of the pong will wait for the polling to finish before sending an upgrade packet (this polling will not finish without this patch)

So if you want to simulate what the javascript client can end up doing, the steps are:
1- Send a GET poll asynchronously
2- Probe the websocket, connect, ping and wait for the pong
3- After 1- finishes, send a GET poll (this should return noop immediately but without this patch we are stuck here)
4- Affter 3- finishes, send the upgrade packet on the websocket to finish the upgrade

If you want to reproduce in real life conditions somehow, I suppose you could try to alter the code of the javascript client to force the events to happen in this order.

L312: https://github.com/socketio/engine.io-client/blob/2847411dd03eab4ab003b50d81c7a44f7bead564/lib/socket.js

    transport.once('packet', function (msg) {
        ...
    })

could become

    transport.once('packet', function (msg) {
        setTimeout(() => {
            ...
        }, 1000);
    })

(I haven't tested this myself)

[miguelgrinberg]

Unfortunately I haven't been able to reproduce this problem, so I cannot test this solution. Do you think you could use the self.upgrading and self.upgraded booleans instead of adding the new self.poll_ended? You could equate self.poll_ended == False to not self.upgraded or self.upgrading I think, and self.poll_ended == True would be the same as self.upgraded == True.

[miguelgrinberg]

Superseded by f2cce2b.