upgrade socket & clientError

[ronag]

When handling a on('request', (req, res) => {}) on does not have to add listener for the error events on req and res since any errors will be forwarded to clientError.

However, when doing on('upgrade', (req, socket, head) => {}), socket can emit an error which might be uncaugth. On common way to implement upgrade is:

server.on(`upgrade`, (req, socket, head) => {
   handle(req, socket, head, err => {
    if (err) {
      // socket.on('error', () => {}) Should this really be neccessary?
      if (socket.writable && err.statusCode) {
        socket.end(Buffer.from(`HTTP/1.1 ${err.statusCode} ${getStatusText(err.statusCode)}\r\n\r\n`, 'ascii'))
      } else {
        socket.destroy()
      }
    }
  })
})

However, the problem with the above is that it might crash the server if the callback is called with an error and either socket.end or socket.destroy is called and an error is emitted afterward. In such a case shouldn't forward the error to clientError automatically for the user?

[lpinca]

In such a case shouldn't forward the error to clientError automatically for the user?

Imho no, the socket is freed once the 'upgrade' event is emitted and it's user responsibility to handle errors after that just as if you created the socket yourself.

If the default error listener is not removed, spurious 'clientError' events with errors with meaningless bytesParsed and rawPacket properties will be emitted.

I've addressed this in #18868

[apapirovski]

Seems like this was addressed by @lpinca. I agree that given the current semantics of upgrade it wouldn't make much sense to handle errors. The socket is provided to the user as-is without any event handlers.

(Also FWIW upgrade is far less strict as of 10.0.0 and allows a user to just ignore upgrade requests on the server by not listening to the event.)