fastify / help

Need help with Fastify? File an Issue here.
https://www.fastify.io/
65 stars 8 forks source link

Aborted requests handling with streams #607

Closed Allain55 closed 2 years ago

Allain55 commented 2 years ago

šŸ’¬ Question here

If a request gets aborted than reply.send will log an ERR_STREAM_PREMATURE_CLOSE error if a strem was given to it. But should these kind of errors be treated as errors actually?

const {Readable} = require('stream')
const {setTimeout} = require('timers/promises')
const fastify      = require('fastify')({
    logger: true,
    disableRequestLogging: true
});

fastify.get('/', async (req, res) => {
    res.header('Content-Type', 'text')

    await setTimeout(1000)

    // abort the request before getting here

    res.send(
        new Readable({
            read() {
                this.push('whatever')
                this.push(null)
            }
        })
    )
    // alternative stream: res.send(require('fs').createReadStream('package.json'))
});

fastify.listen(8080)
    .catch(console.error)

If I cancel the ongoing request somehow (close the browser tab for example) within the delay in setTimeout then reply.send results in an ERR_STREAM_PREMATURE_CLOSE error. I also get errors if I test this example using autocannon and clinic. req.raw.aborted is true so shouldn't fastify handle aborted requests and don't throw an error in this case?

I found a comment in reply.js:

// We must not treat ERR_STREAM_PREMATURE_CLOSE as
// an error because it is created by eos, not by the stream.

But this part of the code is not called in this example. Could this be a bug?

Your Environment

mcollina commented 2 years ago

What would you like to see instead? How can you treat them as errors? What that would entail? We have already started replying to the end user, so we cannot send them a response - and they won't be there to watch for it anyway. I think the only thing we can do in this case is to log something.

From my understanding of your issue, this is working as expected / as I designed. Are you asking for a better / more explicative log message?

Allain55 commented 2 years ago

Actually no, in this case we haven't started replying to the end user. In this example the request is aborted before the stream was created so when reply.send is called it could have known that the request was aborted.

My question is if this is a bug or by design? The reply.then method already ignores this kind of errors so I thought it could be a bug

If this is by design then I'd like to discuss it if this is a good thing. Imho ERR_STREAM_PREMATURE_CLOSE errors should be ignored because it's not an error you can fix. Logging these doesn't have any value. In this case there is no one listening for the response so for me it makes sense to check if a request was aborted inside the send method and if so, don't do anything

mcollina commented 2 years ago

Actually no, in this case we haven't started replying to the end user. In this example the request is aborted before the stream was created so when reply.send is called it could have known that the request was aborted.

Ah, I understand what you mean. We never really focused on this use case - I mean the user is not really there to listen to whatever we return them. If you would like to send a PR with an implementation of this feature, go ahead.

Allain55 commented 2 years ago

Will do. Thank you!

EDjur commented 8 months ago

Hey!

I'm running into this issue 2 years later and aborted is always false for me. reply.raw.destroyed however is true when my request is aborted. šŸ¤ 

It seems that aborted is deprecated since a while now.

Should we add reply.request.raw.destroyed === true to that conditional that was fixed in this PR?

Im on Node v20.11.0.

Thanks!

metcoder95 commented 8 months ago

What do you mean exactly? Also, please feel free to open a new issue to continue the conversation šŸ™‚

EDjur commented 8 months ago

Sure! I created https://github.com/fastify/help/issues/999

Thanks!