compat: stream:error -> [res|res]:error broadcasting

[sebdeckers]

I'm using Http2Stream.respondWithFile from an app that also uses the compatibility layer. Error handling is getting pretty bloated, IMO, because the stream error event is broadcasted to both request and response.

Current code:

const noop = () => {}
request.on('error', noop)
response.on('error', noop)
response.stream.on('error', () => {
  request.removeListener('error', noop)
  response.removeListener('error', noop)
  if (response.stream.destroyed === false) {
    response.writeHead(500, {'content-type': 'text/plain'})
    response.end('Internal Server Error')
  }
})
response.stream.respondWithFile('/my/file/path', response.getHeaders())

Desired code:

response.stream.on('error', () => {
  if (response.stream.destroyed === false) {
    response.writeHead(500, {'content-type': 'text/plain'})
    response.end('Internal Server Error')
  }
})
response.stream.respondWithFile('/my/file/path', response.getHeaders())

What would be needed is to rename or stop propagating stream error events. It's a very simple change in the two error event handlers (for res and req). However I'm hesitant because I don't know what the thought process was behind that broadcasting.

Perhaps related, socket:error events do not seem to bubble up to the req/res instances in the http1 layer.

@jasnell @mcollina Thoughts?

[mcollina]

I'm not convinced that:

response.stream.on('error', () => {
  if (response.stream.destroyed === false) {
    response.writeHead(500, {'content-type': 'text/plain'})
    response.end('Internal Server Error')
  }
})
response.stream.respondWithFile('/my/file/path', response.getHeaders())

Is nice, specifically on the response.stream.on('error' part.

request.on('error', noop)
response.on('error', noop)

is also very bad practice.

Errors on the underlining socket bubble up differently in require('http'), see https://github.com/nodejs/node/blob/master/lib/_http_server.js#L45. Maybe we should not forward them, and rout them to server.on('clientError').

[sebdeckers]

@mcollina Apologies for the confusion. I totally agree, the noop handlers are egregious — I'm merely highlighting that handling of an underlying stream error in two distinct event handlers is awkward, yet not having both listeners would throw an 'uncaught error event' warning. I'm also leaning towards the clientError idea, so that handling the lower level event is possible, yet optional. I am happy to roll a small PR (patch + tests) for this if there is support.

[mcollina]

@jasnell can you please have a look?

[jasnell]

I'm thinking that for the compatibility layer, errors that are happening on the server side should likely only be broadcast to the res object unless those specifically have to do with handling in the request. In other words, any stream.on('error') forwarding would go to res only.