Concurrent websocket requests do not get upgraded

[jrust]

The fact that debounce could cause issues with simultaneous requests was noted in #112, but since that turned out to be a different issue, I thought it best to create a separate issue. @chimurai noted in that ticket that debounce could cause issues, and in a local setup where we have several headless browsers all connecting frequently I saw the problem of debounce causing some websockets never to be upgraded. Debounce was added to solve #57, and so this issue is to ask if there is an alternative method or a workaround? The end-result is that the proxy is not reliable when simultaneous websockets are being created by different clients.

Expected behavior

Websockets should always get upgraded, even if created concurrently.

Actual behavior

Because the upgrade handler is debounced only the last one gets upgraded. This causes the other websockets to never finish initializing.

Setup

• http-proxy-middleware: 0.17.1
• http-proxy-middleware configuration: websocket proxy with external websocket upgrade being invoked
• server: express proxying to jersey

Reproducible Demo

I am able to repro by launching multiple browser tabs simultaneously and having each create a websocket connection. Unfortunately, it's part of a larger system so I don't have a script to demonstrate.

[chimurai]

Sounds plausible; the debounce might cause issues with concurrent upgrade requests.

I'm currently away and won't be able to investigate / work on it.

Do you mind trying to create a fix for this issue?

[jrust]

I can confirm that two calls to proxy.ws() on the same socket object gives the error described in #57. Namely, RangeError: Invalid WebSocket frame:, which I think is because it is getting sent the proxy response twice.

I see two options for solving:

• Remove the debounce and let users of the middleware figure out whether to use the http version or the manually invoked ws version. What I don't know is if the two are mutually exclusive. Is the setup in sockjs: WebSocketSession not yet initialized #57 valid? Maybe that user should not have been using the listener.on('upgrade', proxy.upgrade).
• Try to track the state of whether a socket has been upgraded by adding a property to the socket like socket._hpmProxied = true. However, I'm wary of messing with the low-level socket and don't know if the property would persist in the case of double-invocation.

[chimurai]

Sorry for the late reply @jrust.

I removed the debounce, hopefully it'll solve the concurrent issue.

Just published a beta version.
Can you give it try and see if the fix works?

npm install [email protected]

[chimurai]

Hi @jrust

It would be really nice if you can verify the fix.

[chimurai]

Found an issue in the previous fix.

Published a new beta:
npm install [email protected]

[jrust]

Sorry, I've been swamped, but should have some time this week and will report back.

[jrust]

Hi, I've tested 0.20.0-beta.2 and found it working well -- able to start up multiple concurrent websockets without issue. Thanks for your work on this!

[chimurai]

I'll try to push a new version this weekend.

Thanks for reporting the issue and verifying the fix!

[jrust]

Hi Chimurai,
Any chance you can release the next version? It also contains a new micromatch which fixes a vulnerability downstream. Thanks!

[chimurai]

Found a tiny bug in #344.
Have to do some additional testing. Will publish a new version soon.

[chimurai]

Hi @jrust. Still facing the bug.

Was wondering which of the two options you were using to enable WebSockets.

Currently was thinking to make the manual proxy.upgrade and the http way (ws: true) mutually exclusive.

[jrust]

What is the bug you are finding? We are using both the ws: true and upgrade as a way to add additional logging -- here is the specific config:

    wsProxy = proxy(['/agent-link', '/events'], {
      target: `ws://${constants.BASE_ADDRESS}:${options.port}`,
      ws: true,
      changeOrigin: true,
      logLevel: 'debug',
      logProvider: _.constant(logger)
    });

      const origUpgrade = wsProxy.upgrade;
      wsProxy.upgrade = (req, socket, head) => {
        logger.info(`New websocket to ${req.url} from ${req.connection.remoteAddress}`);

        socket.on('error', (err, errReq, errRes) => {
          logger.error(`Websocket error: ${err}, request: ${util.inspect(errReq)}, response: ${util.inspect(errRes)}`);
        });
        socket.on('close', () => logger.info(`Websocket disconnected: ${req.url}`));

        origUpgrade(req, socket, head);
      };

Given that we are just using the upgrade event as a hook to add more logging we could do without it if need be. Though maybe some of the logging we have would be useful in the core library?

[chimurai]

Think I've found it. Issue was reproducible with the ws example in the repo + Safari.

http calls (like /favicon.ico) caused multiple subscriptions, once an ws upgrade was requested, all those subscriptions where fired...

Somehow Safari kept requesting /favicon.ico and Chrome didn't

[chimurai]

Just released a new version to npm: [email protected]

@jrust Thanks for reporting this issue and helping out.