Improve HTTP server 'upgrade' event generation logic

[mykola-mokhnach]

What is the problem this feature will solve?

There is an interesting bug/limitation we've stumbled upon recently in the node.js's http server implementation.

We had the 'upgrade' event listener defined in our code to properly map web sockets, although this lead to an issue with HTTP/2 h2c upgrade requests.
Basically, the current logic to emit the 'upgrade' event only considers the presence of the Upgrade header for a particular request without verifying other values. Such approach causes the event to be also emitted for HTTP/2 upgrade requests, which have similar syntax to websocket upgrades, just the value of the Upgrade header differs.
In our case the presence of the upgrade event listener was always causing h2c upgrade requests to be unexpectedly rejected, while the expected behaviour would be to just downgrade the connection to HTTP/1.1.
A workaround to that was to remove the upgrade listener and add a http middleware instead, although this creates another bug where the server.requestTimeout is applied to web sockets created by such middleware. After the timeout is fired websocket connection is killed forcefully and there is no way to customize this behaviour.

See the issue appium/appium#20760 for more details and code examples

What is the feature you are proposing to solve the problem?

I expect there is a possibility to either be able to explicitly provide to which types of upgrade request the upgrade event must be triggered (e.g. only web socket upgrades)

OR

there is a possibility to continue handling the request as if no upgrade event has been defined (similarly to calling next() in a middleware handler), so I could default to HTTP/1.1 if I detect it's a h2c request type after checking the request headers (or any other non-websocket upgrade request)

OR

there is a possibility to remove requestTimeout from the particular socket if we upgrade it to a websocket in a middleware rather than via the upgrade event

What alternatives have you considered?

Unfortunately I am out of other options for now.

[github-actions]

There has been no activity on this feature request for 5 months. To help maintain relevant open issues, please add the never-stale Mark issue so that it is never considered stale label or close this issue if it should be closed. If not, the issue will be automatically closed 6 months after the last non-automated comment.
For more information on how the project manages feature requests, please consult the feature request management document.

[mykola-mokhnach]

This issue is still relevant. Please do not close it

[pimterry]

This is interesting @mykola-mokhnach, I think you're right that this needs a better solution. It's very difficult if not impossible to accept some upgrade requests (websockets but not h2c) in the current implementation.

What do you think about adding a callback in the HTTP server options to handle this? E.g:

http.createServer({
  shouldUpgradeCallback: (req, socket, head) => {
    return req.headers.upgrade !== 'h2c';
  }
});

The callback gets the same arguments as the upgrade event handler, but returns a boolean that decides whether the request triggers an 'upgrade' event (effectively accepting or hard-rejecting the request) or a normal 'request' event (ignoring the upgrade suggestion). Handling this with a separate option avoids breaking any existing code, and is reasonably consistent with how we handle other negotiations like ALPN & SNI.

This would default to server.listenerCount('upgrade') > 0 (i.e. if you set an upgrade listener, all upgrades are accepted, otherwise all ignored) which should preserve existing behaviour and ensure this doesn't break anything.

[mykola-mokhnach]

Thank you @pimterry for taking a look into this issue.

I think the above proposal should be able to solve the issue we have.

[pimterry]

PR opened: #59824

Minor difference to the above, in that the callback is called only with the request (not the socket or head data). It might be possible to add the other arguments, but I think it's unlikely to be useful and adds quite a bit of complexity, and we can safely add extra args later, so better to avoid for now.

[mykola-mokhnach]

@pimterry Is there any way for us to know when this patch is going to be available in the published node version?