http: IncomingMessage does not emit error on premature close

[ronag]

This is an issue that got stuck in the Node repo due to concerns of breaking the ecosystem. Thought it might be worth a quick look in this group to see whether we find a solution we haven't considered yet?

The problem here is that instead of emitting an ECONNRESET 'error' on the IncomingMessage object we emit 'aborted' which is surprising when the object is sent in a function that expects a regular stream.

function myStreamProcessor(stream, cb) {
  // Stream logic that will deadlock if stream 
  // is an `IncomingMessage` since it does not listen to `'aborted'`
  stream
    .on('error, cb)
    .on('data', /* do something */)
    .on('end', cb)
}

The current workaround, which is probably good enough for most cases, is to encourage usage of pipeline which properly handles/normalizes this issue.

This is not a totally uncommon issue, e.g. I saw it recently at https://github.com/szmarczak/http-timer (3,233,355 weekly downloads) where an 'aborted' listener is missing and instead an 'error' listener is registered.

nodejs/node#28172 (issue)
nodejs/node#28677 (closed proposal PR)

I would recommend a read through of the comments for PR above to understand existing concerns.

[wesleytodd]

I believe that will be a breaking change for this module, right? Previously it would error with ECONNABORTED but will now error with ECONNRESET. This module is what Express body-parser uses, so is probably the most relied upon implementation in the ecosystem.

That said, I think this is a good change overall as it reduces complexity in userland code. Is there specific action the team can take to help unblock this?

[wesleytodd]

@dougwilson in case you didn't see this, I imagine you have some opinion. Also it is on the agenda for the call later this week, so might be a good one to make if possible since I will be unable to attend.

[dougwilson]

Well, as far as raw-body is concerned, it worked on any stream, not just the HTTP ones. The handling of the aborted event is just due to HTTP, so consolidating it into error is actually a good thing there. Of course it's the users who would potentially see the different error arise (the aborted error vs the reset error), but raw-body could still make that distinction if necessary.

That is to say, overall, I don't think this is a breaking issue for that module, only perhaps something to just account for in a future update.

[ronag]

Is there specific action the team can take to help unblock this?

I'm not sure. Can we find a way to do this in a way that we can argue that the effect on the ecosystem would be acceptable?

Some concerns:

nodejs/node#28677 (comment)

I think this is a pretty big breaking change as an 'error' listener should always be added on res now (if there is no 'aborted' listener) or the process will crash. It was sufficient to have an 'error' listener of req before.

nodejs/node#28677 (review)

Considering the breakage in the ecosystem, I don't think we should land this.

Just to summarize shortly. The problem is that doing this change as is would potentially cause lots of modules/users to end up in uncaught exception errors since they do not necessarily listen to the 'error' event, instead only listen for 'aborted'.

The proposal in nodejs/node#28677 was to emit 'error' only if no 'aborted' handler exists. But that would still cause problems for modules that don't register a 'aborted' nor 'error' handler.

Another option, which I guess has not been considered would be to always emit both events, but in the case of 'error' have a hidden noop handler which would stop it from causing uncaught exception (maybe with a warning?). Not optimal but maybe a compromise better than the status quo?

[wesleytodd]

as an 'error' listener should always be added on res now

This is a valid concern and one which should not be taken lightly, IMO. I wonder if having all the major frameworks implement this would be enough to mitigate this concern? If we covered that case, is there any way we could know the remaining impact of this change?

I am not familiar enough with the implementation off the top of my head, but what if an error handler on req also added an error handler to it's corresponding res (only if it did not have an abort or error handler of it's own)? Would this work?

But that would still cause problems for modules that don't register a 'aborted' nor 'error' handler.

I am very interested in how common this is seeing that the major frameworks do this correctly.

[ronag]

but what if an error handler on req also added an error handler to it's corresponding res (only if it did not have an abort or error handler of it's own)? Would this work?

That is an interesting option. I believe that might work.

So if done in user space the equivalent would be something like:

OUTDATED EXAMPLE REMOVED, see below

[dougwilson]

Wait, are we talking about web server frameworks or web clients? That example is a client call.

[ronag]

Wait, are we talking about web server frameworks or web clients? That example is a client call.

Both, this relates to IncomingMessage which is used both by server and client. Should probably have made that clear in the beginning.

[dougwilson]

Gotcha. Following some of the above conversation, I think what an interesting thing is that in Node.js 0.10, when the request stream on the server data came paused by default, the ecosystem built up around async middleware, where (for example the body-parser of express) may not engage against the request object for some amount of time after the server accepted the connection (perhaps it is authenticating the call with a db call against an Authorization header). This could mean that even when apps are registering these events, they may be doing them too late unless the aborted error is delay until the data stream is unpaused.

[ronag]

,they may be doing them too late unless the aborted error is delay until the data stream is unpaused.

So you mean on server side, not emitting an error in this case is actually by design where the users later check the aborted property and/or event?

Would it be an option to delay emitting the error until the stream is unpaused? Could that work?

User space equivalent would be something like:

// Server
function rootMiddleware (req, res) {
  let aborted = false;
  req
    .on('unpause', () => {
      if (aborted && req.listenerCount('aborted') === 1) {
        req
          .on('error', err => res.destroy(err))
          .destroy(new Error('aborted'));
      }
    })
    .on('aborted', () => {
      aborted = true;
    });
}

// Client
function makeRequest(options) {
  const req = http.request(options);
  // Do unpause trick here as well?
  req.on('response', res => {
    res
      .on('aborted', () => {
        if (res.listenerCount('aborted') === 1) {
          res
            .on('error', err => req.destroy(err)) // propagate error to request and avoid uncaught exception
            .destroy(new Error('aborted'));
        }
      })
  });
  return req;
}

[dougwilson]

Right, as far as the server-side goes, I don't think that emitting the abort as an error event is wrong (vs aborted). But based on how people are coding around it, they typically get the request, perform the action, and write the response, without checking anything. Before Node.js 0.10, this hurt when people wanted to do pre-work actions like authentication, as the data events would start immediately, so in 0.10 the streams were changed to be paused-by-default (IDK the specific reasons why though--I wasn't part of anything when that happened, only know from the user's experience). This was a nice change, as then I could have everything together logically.

But of course, that means that typically users have consolidated all the code to add all needed listeners (like the error listener) in the parsing area, which may not get attached until later, at the time the stream is unpaused. If we are looking for "compromises" I think that may be a good one--to emit that error as soon as the user unpauses the request stream as they are saying "I'm ready to read/interact with this request now" and would be very likely expecting errors to occur. It could be a very good middle ground if that is desired to have.

P.S. I cannot speak for your examples @ronag as I don't really use the Node.js client. Since this is the web server framework WG tracker, it make make sense to show the examples from the server POV.

[ronag]

P.S. I cannot speak for your examples @ronag as I don't really use the Node.js client. Since this is the web server framework WG tracker, it make make sense to show the examples from the server POV.

rootMiddleware is an example for server side.

Sorry to bring in the client side here as well. Since these are so closely related they kind of go as one in my head.

[dougwilson]

rootMiddleware is an example for server side.

Oh, haha. That is my fault too; did not look too closely at it. Yea, I think that is what I'm saying. I say "think" because I didn't off-hand understand the implications of the res.destroy there is all.

[ronag]

the implications of the res.destroy there is all.

res.destroy(err) should destroy the object and emit err as 'error' and then 'close'. Might not be the case right now though. That's how it is in streams.

[ronag]

What do server frameworks usually do in terms of error handling? i.e. when do they register error and/or aborted handlers and on which object req and/or res?

[ronag]

This landed in node.

[ronag]

nodejs/node#33172