Terminating request after handling an error

[fatfisz]

I've been tinkering with file upload streams and came across such scenario:

1. something causes an error and this error is passed down to the custom error handler
2. the custom error handler invokes res.status(500).send(message)
3. request is still not terminated
   This is caused by data waiting on the request to be handled. I want the request to terminated soon after 2 happens.

I've looked into the express code and noticed that there is a last handler I could pass the error to. The problem is, in "production" mode a generic error message would be sent to the user, whereas I'd prefer a custom message. Tampering with http.STATUS_CODES is out of question for me.

After looking a bit further and experimenting, I've created such a snippet:

app.use((err, req, res, next) => {
  // Print error
  console.error(err.stack);

  function write() {
    res.status(500).send('Internal server error - see logs for details');
  }

  if (req.complete) {
    return write();
  }

  // Terminate request if there is unsent data
  req.unpipe();
  req.on('end', write);
  req.resume();
});

The bottom part is obviously copied from the finalhandler lib.

My question (at last) is: can this be done a bit cleaner? Getting there wasn't that easy and the result looks a bit complicated. Did I miss something in the docs?

[dougwilson]

No, you did not miss anything. The issue here is a Node.js core issue. The finalhandler contains a workaround for the Node.js core issue, but it is not fool proof and can have some nasty effects, so I don't recommend using it if you want to be sure the client gets your message.

You would have the exact same problem if you were just using the "http" module from Node.js core directly and not Express.

Typically, the suggested pattern to get around this problem is two things 1) open an issue with Node.js to see what can be done or 2) at the site you are reading from the request, even if you have an error, finish reading the entire request before calling next(error) (this is what the body-parser and multiparty modules do).

[fatfisz]

Thanks for the quick response :) That clarifies things a bit; I will look further into this issue.

About 2 - it would be better to terminate the request immediately, since there may be a lot of data awaiting. If the error occurs at 5% of upload, then keeping user waiting for the next 95% is undesirable for me.

Could you write more about those "nasty effects"? Is this about memory leaks or something else?

[dougwilson]

would be better to terminate the request immediately, since there may be a lot of data awaiting. If the error occurs at 5% of upload, then keeping user waiting for the next 95% is undesirable for me.

Right, but using the pattern in finalhandler, you'll just end up waiting for that 95% anyway, so you are not saving any time by copying that pattern :) The only successful way Node.js give you to terminate at the 5% is using req.destroy(), which won't send your response (which leads back to possibility a bug in Node.js core).

Could you write more about those "nasty effects"? Is this about memory leaks or something else?

It is with things like calling .unpipe() in a context where you do not know what is piped--that destination may now never close some related file descriptor and you'll end up with a fd leak or other really hard to notice things. That's why I would recommend handling this condition at the site you are reading from the request, since you'll know what you need to clean up.

[fatfisz]

Right, but using the pattern in finalhandler, you'll just end up waiting for that 95% anyway, so you are not saving any time by copying that pattern :)

I've missed that :/ Just tested this with a 3.5GB file, which takes 14s to upload. When I invoke an error after 1GB is uploaded, it is printed immediately (after 4s or so), but the request still takes full 14s to end.

It seems that the best way for me is to save an info about error somewhere on the server side, then destroy the request, and handle this on client side by querying the server with a request id or something.

Or just abandon the whole "immediate termination" idea and just let users reap what they've sown by trying to upload too much data.

Thanks again for help!

[dougwilson]

It's no problem! I'm going to close this issue for now, but please feel free to reach out! Also, let me know if there is something you wanted to keep this issue for and I can definitely reopen.