http server upgrade event

[jordbaer]

• Version: 11.0.0, 10.0.0, and probably more
• Platform: macos
• Subsystem: http

http server upgrade event:

When the 'upgrade' event is emitted, the http server doesn't unregister the 'timeout' handler (function socketOnTimeout).

If a timeout event occurs later, the http server code will destroy the socket.

workaround:

server.on("upgrade", (req,socket,head) => {
socket.removeAllListeners("timeout");
socket.setTimeout(0);
...
});

[oyyd]

the http server doesn't unregister the 'timeout' handler

Hi, @jordbaer. I think that is expected. And socket.setTimeout(0); works well on my Mac(without socket.removeAllListeners("timeout");).

Did you expect socket.setTimeout(0); to be done automatically when "upgrade" events are emitted?

[jordbaer]

I don't think that is expected. The http code doesn't know what the upgraded protocol needs to do. Maybe the protocol wants to send a message to the socket when a timeout happens. Doesn't work, because the http code destroys the socket before the upgraded protocol event gets the "timeout" event.

Yes, I expect socket.setTimeout(0) and the removal of the http timeout listener to be done before the "upgrade" event is emitted. Because all other http listeners to all other events are removed too. And because the http server is no longer in charge of the socket.

[apapirovski]

@nodejs/http

[lpinca]

I'm not sure. I agree that we should remove the default listener of the 'timeout' event but I don't know if it's a good idea to also disable the existing idle timeout.

The user can set a custom timeout:

server.on('connection', function(socket) {
  socket.setTimeout(1000);
});

and in this case it might surprising to see that the idle timeout is gone when the 'upgrade' event is emitted. The same applies to the client.

[lpinca]

I think it's better to override it in user code after the 'upgrade' event is emitted as suggested in the workaround but I'm 👍 on removing the default listener.