Node: http's 'upgrade' event should be handled one handler at a time

[SEAPUNK]

• Subsystem: http (maybe others)

From what I understand, there is only one way to handle connection upgrades: with http.Server's 'upgrade' event. However, from working on a WebSocket implementation, I noticed that there's (what I believe is) an architectural flaw with how the connection upgrades are handled:

From the documentation:

Emitted each time a client requests a http upgrade. If this event isn't listened for, then clients requesting an upgrade will have their connections closed.

For example:

// from the 'ws' module
server.on('upgrade', (req, socket, upgradeHead) => {
  if (req.headers.upgrade !== 'websocket') {
    // close or return?
  }
  // handle the websocket connection
})


// from another module that listens to a different type of upgrade
server.on('upgrade', (req, socket, upgradeHead) => {
  if (req.headers.upgrade !=== 'some-other-protocol') {
    // close or return?
  }
  // handle this connection
})

If the WebSocket isn't supposed to handle the socket, should the handler close the socket, or return in hopes that another upgrade handler handles the connection?

...and I'm not even mentioning non-blocking calls within handlers, which could be done as a part of determining whether the socket connection should be handled by the handler.

If there is no other handler for the 'upgrade' event, the socket just stays idle until either the server or the client closes the connection, most likely via timeout (which in itself can cause problems if the timeout is specified to "none").

While I do know that what I am suggesting is more than likely a breaking change, could upgrade handling be converted to (or something along the lines) to what is essentially a middleware setup, a bit like connect? This way, the socket would only be handled by one handler at a time (blocking or non-blocking), a specific handler can withhold a socket it's handling from the other handlers (prevents conflicts), and if everything falls through, it can fall through to node.js's 'last' middleware, which closes the connection.

Will something like this be possible?

_edit:_

On my way back home, I realized that the 'connect' event is pretty much the same, and express mitigates it with connect + various libraries that write themselves around it, but I just felt that it could be more standardized in node so people who write libraries that use the event e.g. socket authors would be more convinced to add the option where the websocket handling is done as a middleware instead of an event handler. Either way, middleware (like Promise core API) would be nice.

[alexhultman]

+1

[simonkcleung]

The socket is shared among handlers.
You can add a flag .handled to the socket.

// from the 'ws' module
server.on('upgrade', (req, socket, upgradeHead) => {
  if (req.headers.upgrade.toLowerCase() === 'websocket') {
   // handle the websocket connection
  socket.handled=true;
}
})


// from another module that listens to a different type of upgrade
server.on('upgrade', (req, socket, upgradeHead) => {
  if (req.headers.upgrade.toLowerCase() === 'some-other-protocol') {
  // handle this connection
  socket.handled=true;
}})

server.on('upgrade', (req, socket) => {
  if (!socket.handled) {
  socket.end('HTTP/1.1 503 Service Unavailable\r\n\r\n','ascii');
}})

[SEAPUNK]

@simonkcleung How will the other libraries know? I'm not in control of how ws does its handleUpgrade method, and creating a PR to add such functionality will "fix" that library, but not the other ones, because this sort of checking is non-standard. I have no doubt that if I created a PR at some other library that does upgrade event handling, it would end up in bikeshedding ("Don't do .handled, do ._handled!").

Then, it's a question of if the handler is the last handler. If the socket isn't handled, and the last handler is not going to handle it, it falls through and nothing happens to the socket, making it idle until the socket times out on either end.

_edit_: I noticed that your example attaches a 'last' upgrade method, but that is a problem within itself: It's hacky, and not guaranteed to work: Why can't the 'last' handler be node.js's own, if it's already automatically closing the connection if there are 0 listeners for the event? What if some library adds a listener to the 'upgrade' method dynamically, sometime after we create the 'last' listener? What then?

[alexhultman]

Given how overly bloated EventEmitter is, I'm shocked there's no support for registering a listener in the very end of the chain. Also, if a listener could abort the traversal of the listener chain this could be solved easily. Why was this never considered for EventEmitter? @simonkcleung That solution is not guaranteed to work at all.

[jasnell]

Currently, handlers are invoked in the order they are added. Calling on() to register a handler adds it to the end of the listeners array. However, we do not make any guarantees as far as the API is concerned that the listeners will be invoked in any particular order. Given that, the correct way of implementing what you're looking for here would be register a single upgrade listener that handles the upgrade by deferring to your own chain of options. This can be implemented easily using a module such as async.

[alexhultman]

The problem is that you cannot know for sure (or even slightly assume) that _you_ are the only listener. If you have an HTTP server that one WebSocket server attaches to, then there is no way to know if another, potentially completely separate, WebSocket server attaches later to the same HTTP server. Also, the whole point of supplying a "path option" to the WebSocket server is to allow multiple different "routes" on the same HTTP server leading to different WebSocket servers.

[jasnell]

The listenerCount method can tell you how many listeners there are.
On Apr 23, 2016 5:38 PM, "Alex Hultman" [email protected] wrote:

The problem is that you cannot know for sure (or even slightly assume)
that _you_ are the only listener. If you have an HTTP server that one
WebSocket server attaches to, then there is no way to know if another,
potentially completely separate, WebSocket server attaches later to the
same HTTP server. Also, the whole point of supplying a "path option" to the
WebSocket server is to allow multiple different "routes" on the same HTTP
server leading to different WebSocket servers.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/issues/6339#issuecomment-213858198

[alexhultman]

Sure, but is there any way to get your ID in the chain? The important factor here is to be able to know for sure if you are the last listener or not.

[SEAPUNK]

@alexhultman I guess a workaround for that would be doing an indexOf with emitter.listeners(eventName), making sure that the index of the listener is the length of the listener array - 1

[alexhultman]

I guess you could use EventEmitter.listeners('the event') to get a copy of the array of listeners and check if you are the last entry. That should work but is very inefficient.

[alexhultman]

@SEAPUNK Yep, that's probably the only solution but this includes memory allocation, copy, linear search for every upgrade.

[SEAPUNK]

Then comes the question of handled sockets, with listeners after it. There should be a standard (even if not implemented in core) way of saying "hey other listeners, I am going to handle this socket", so they don't mess with it.

[alexhultman]

Right, EventEmitter needs an .abort() method to abort the traversal.

[jasnell]

You can removeAllListeners prior to adding your own, and check
listenerCount when triggered to make sure none others have been added.
On Apr 23, 2016 5:42 PM, "Alex Hultman" [email protected] wrote:

Sure, but is there any way to get your ID in the chain? The important
factor here is to be able to know for sure if you are the last listener or
not.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/issues/6339#issuecomment-213858291

[SEAPUNK]

I don't think removeAllListeners is a good way to do something like this, because there can always be listeners that listen to connections and upgrades for the purpose of statistics collecting, and such.

[SEAPUNK]

The biggest point of this issue to me (even if it doesn't get implemented anytime soon), is that there should be a better way to do connection/upgrade handling, so the module writers can write handlers that are pretty much guaranteed to not break or cause side effects without utilizing tricks like the ones discussed above.

[alexhultman]

@jasnell That's not a solution, this will break all other listeners like if you have Express or anything like that listening. I'm pretty sure 11/10 would rage if one server comes along and breaks the chain of listeners like that. Also, you want to have the ability to register multiple listeners on different paths like I mentioned. Otherwise the whole purpose of having an EventEmitter is completely wasted.

[vkurchatkin]

While I do know that what I am suggesting is more than likely a breaking change, could upgrade handling be converted to (or something along the lines) to what is essentially a middleware setup, a bit like connect

It's possible to implement this in userland, the same way connect did. In general, it's true, it doesn't make sense to have multiple 'upgrade handlers or request handlers.

[simonkcleung]

Current API of EventEmitter guarantes all registered handlers are executed. It is fine. The socket is passed to all registered handlers. This is how the socket supposed to be handled. If there is any conflict bewteen the handlers, it is the problem of the writer(s).

[131]

As @vkurchatkin, i'll just say this have to be left to the userland.
My guess, the library, WHEN registered on a "path" that it does not understand , should just return and let the socket opened (as in https://github.com/websockets/ws/blob/master/lib/WebSocketServer.js#L164) , it must then close it if no path was specified (and if the upgrade protocol is not handled)

I do use websocket for transporting h264 binary frames, and believe me, using multiple WS server on same http instance is very usefull.

globalapp = MyAppControler.connect(new Websocket());

var liveStreamCam01 = new Websocket("/cam/hallway");//binary data only
liveStreamCam01.pipe(h264decoder).display($("#canvas1"))

var liveStreamCam02 = new Websocket("/cam/parking");//binary data only
liveStreamCam01.pipe(h264decoder).display($("#canvas2"))

globalapp.on('message', function thatHaveNothingToDoWithCameras)

[Trott]

This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

[ccantill]

I'm having another issue with how these upgrade events are handled; I have an HTTP client that sends h2c (HTTP/2) upgrade request headers, which I don't intend to honor. This is in itself not an issue if there are no upgrade event listeners, as those headers are then ignored and the requests are just handled like normal HTTP/1.1 requests as they should.
However, I'm also using WebSockets so an upgrade event listener is installed on the server. This breaks the entire thing because as soon as that event listener is there (even though it doesn't intend to do anything with h2c upgrade requests), the request processing is aborted and it becomes very hard to do anything sensible with it afterwards. See also https://github.com/websockets/ws/issues/1673
There should be a way for the upgrade event listener to signal that it doesn't intend to do anything with the upgrade request so normal processing can continue.

I'm currently working around it by changing IncomingMessage:

    // workaround
    Object.defineProperty(http.IncomingMessage.prototype, "upgrade", {
        get() {
            return "connection" in this.headers && "upgrade" in this.headers && this.headers.connection.startsWith("Upgrade") && this.headers.upgrade.toLowerCase() == 'websocket';
        },
        set(v) {
        }
    });

That's dirty and extremely brittle, but I can't see any other way currently to make this work.