Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abort a streaming response. #2700

Closed
gustavnikolaj opened this issue Jul 8, 2015 · 14 comments
Closed

Abort a streaming response. #2700

gustavnikolaj opened this issue Jul 8, 2015 · 14 comments
Assignees
Labels

Comments

@gustavnikolaj
Copy link

Recently I have found my self in two different situations where I needed to abort a response that I already started streaming. Doing this in a reliable way across different situations proved to be quite hard. We finally came up with a solution to the problem.

function abortStreamingResponse(res) {
    /* If you encounter an error after initializing a streaming response,
     * it is not straight forward to abort it and cause the response to
     * be interpreted as an error in the browser.
     *
     * This was the only reliable way we found to interrupt the streaming
     * response in all cases.
     *
     * There's two cases that you have to consider when doing this:
     *
     * 1: You encounter an error before the headers are sent.
     * 2: You encounter an error after the headers are sent.
     *
     * Express will claim that the headers are sent (res.headersSent), even
     * before they are actually written to the socket. By writing something
     * that looks like a valid response header in addition to what Express
     * has already written, or has qued to be written, you will cause the
     * response to inevitable cause an invalid chunked encoding error.
     */
    res.connection.write(
        'HTTP/1.1 500 Let\'s ruin that response!\r\n' +
        'Content-Type: text/plain\r\n' +
        'Transfer-Encoding: chunked\r\n\r\n'
    );
    res.end();
}

We had multiple places across our application where we could encounter errors in the source streams that we pipe to res, so we actually went even further. We added an error handler, that incorporated this method.

return function (err, req, res, next) {
    var responseObj = { ... };
    // If res.headersSent is true, Express has either written
    // the response headers to the socket, or qued them to be
    // written. Which means that we will get an exception if
    // we try to set the status code of the response etc. When
    // we need to handle an error after that moment we need to
    // corrupt the response already given to the client.
    if (res.headersSent) {
        return abortStreamingResponse(res);
    }
    return res.status(statusCode).send(responseObj);
}

This allows us to pass errors to the next callback, even after we started writing the response. The result in the browser will be an error ERR_INVALID_CHUNKED_ENCODING in Chrome, and similar errors in other browsers.

What have you done when you have been in similar situations?

It seems like a problem that you would run into constantly when developing applications where you are streaming stuff out to clients over HTTP. But I have not been able to find any mentions about it when searching around. It seems like something you'd want to have a baked in solution to in Express - I'm not saying that it should be my solution - but just some solution.

@dougwilson
Copy link
Contributor

We provide a mechanism to this this out of the box if you just call next() with an error after you started streaming. If you added a custom error handler, however, you will probably have to do it yourself still.

@dougwilson
Copy link
Contributor

It's also probably useful to report this up to Node.js, as I see you struggling with things like res.headersSent, which is actually nothing provided or controlled by Express, but rather by Node.js itself. Node.js recommends simply calling req.destroy() if you want to kill the request in an unknown state.

@gustavnikolaj
Copy link
Author

Thanks @dougwilson. I was able to make it work by defering to the default error handler if the headers are already sent.

As I see it, there are two causes of this problem; first, that our error handler masked that you actually already provided this functionality, and secondly the that I weren't able to find any documentation regarding this. It would probably be a good idea to make a note of it on the error handling documentation.

For future reference, should anyone get here looking for an answer. Changing the custom error handler to the following made it work like intended.

return function (err, req, res, next) {
    var responseObj = { ... };
    if (res.headersSent) {
        return next(err);
    }
    return res.status(statusCode).send(responseObj);
}

The finalhandler module is what is taking care of destroying the socket, when it's not able to report an error.

@dougwilson
Copy link
Contributor

Good suggestion for the web site documentation! Please make a request (or even a PR) over at https://github.com/strongloop/expressjs.com so it get added :)

@toymachiner62
Copy link

toymachiner62 commented Mar 20, 2019

@dougwilson This comment about using req.destroy() is a life saver. Where in the Node docs did you find this?

In my case i'm actually streaming a file through my express app straight to s3. If i have an error on subsequent processing, res.status(500).send() doesn't do anything and the request eventually times out.

By calling

req.destroy()
res.status(500).send('some error')

it works as expected and returns a 500 response.

EDIT Actually the issue with my code above is that once you call req.destroy() it destroys the request and so you can't actually call res.* since `req.destroy() already destroyed this request.

Now i'm curious if there is a way to end a streaming request AND return a response that I want to (such as res.status(500).send('some error')

@dougwilson
Copy link
Contributor

Hi @toymachiner62 I'm glad you found it useful. I can't really recall that after 4 year since that comment now, especially when the Node.js documentation has changed quite a lot in that time! As for your question / edit, you may want to open an issue with Node.js directly -- these objects are all managed by Node.js (and res.status(500).send('some error') is simply res.statusCode = 500; res.end('some error') effectively in Node.js HTTP, which you'll likely see the exact same behavior.

@vasilionjea
Copy link

Probably a stupid question, but I'm running into a situation where browsers allow only a handful of open persistent connections (the magic number seems to be 6). Is there a way to manage multiple streams by closing one before opening another one so the client won't end up surpassing the limit of open connections?

On a high level, perhaps something like this:

const send = require('send');

let current = {
  stream: null,
  req: null,
  res: null
};

app.get('/tracks/:name', (req, res) => {
  const track = Tracks.getByName(req.params.name);

  if (track) {

    // Close the previous MP3 stream
    if (current.stream) {
      // How do I close the previous persistent stream connection?
      // ---------------------------------------------------------
      // current.stream.emit('end');
      // current.req.destroy();
      // current.res.emit('end');
    }

    // Stream this MP3 track
    send(req, track.src)
      .on('stream', (stream) => {
        current.stream = stream;
        current.req = req;
        current.res = res;
      })
      .pipe(res);
  } else {
    res
    .status(404)
    .end(null);
  }
});

@dougwilson
Copy link
Contributor

@vasilionjea likely current.res.destroy() would be the best method in your example.

@vasilionjea
Copy link

vasilionjea commented Jan 26, 2020

@dougwilson Thanks for the reply. Unfortunately still the previous connections seem to continue in the open state on the client side. The moment I trigger a stream beyond the magic number 6, the following requests stay in the pending state indefinitely.

@dougwilson
Copy link
Contributor

Gotcha. I have never personally experienced it, so don't know off-hand. I would imagine that it will be something Node.js specific that needs to be done, so my be worth asking on Node.js issue tracker. Otherwise you're welcome to open a new issue here and provide a full app and a way to replicate the issue so we can try and experiment to figure out a solution. The experts would be in the Node.js issue tracker, as it would all be Node.js APIs (which is why we would have to try and learn it as well).

@vasilionjea
Copy link

No worries, and thanks for the suggestions! Sounds good, I'll put together a sample app using send and express and I'll open a new issue. In parallel I'll also ask on the Node.js issue tracker.

@vasilionjea
Copy link

@dougwilson Actually after a bunch of looking into it, I discovered it has nothing to do with the server side (of course... 😄). The client side <audio> element kept fetching more data from the stream, therefore keeping the connection alive (per the keep-alive header).

The TL;DR is that I wasn't properly unloading the stream from the <audio> element, instead I was only causing it to stop buffering, which kept the connection alive indefinitely.

Thanks for your help! 👍

@alex-nishikawa
Copy link

@toymachiner62
Did you find out how to end a streaming request AND return a response

@toymachiner62
Copy link

toymachiner62 commented Feb 20, 2020 via email

@expressjs expressjs locked as resolved and limited conversation to collaborators Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants