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
Handle aborted requests #3651
Handle aborted requests #3651
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be more comfortable in shipping this as a semver-major change, could you target the next branch? The error handling path has been significantly improved there.
lib/reply.js
Outdated
@@ -130,6 +130,10 @@ Reply.prototype.send = function (payload) { | |||
return this | |||
} | |||
|
|||
if (this.request.raw && this.request.raw.aborted === true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a TODO comment here to use the destroyed
field (see https://nodejs.org/dist/latest-v16.x/docs/api/http.html#messageaborted, aborted is deprecated) as soon as we drop support for Node v14? Thanks
I also think that raw
would always be defined here. When is it not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raw
should be always defined according to the types but in this case a few test cases would be broken. I had to choose to add raw
to these tests or do this. Would you prefer if I update those test cases?
Will do the rest.
lib/reply.js
Outdated
@@ -130,6 +130,10 @@ Reply.prototype.send = function (payload) { | |||
return this | |||
} | |||
|
|||
if (this.request.raw && this.request.raw.aborted === true) { | |||
return this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run the onError hook with a new Error (create an ad-hoc
one inside lib/error.js). Note that if the payload is a stream you must call .destroy() on it too to avoid memory leaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure you want to treat aborted requests as errors? Is it not a special case?
Currently when reply.send is called in this case with anything other than a stream there would be no error. Errors happen only with streams
For example if someone wants to handle aborted requests it is possible before calling reply.send
Thanks for the tip on destroy(). Do you think this would be a good way to destroy streams?
if (payload && payload.destroyed === false && typeof payload.destroy === 'function') {
payload.destroy()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I think there is nothing we can do in this PR and the current behavior is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK that's fine by me if you think so, I can fix this problem in my codebase using a decorator or an onSend hook. So was my description not clear about the problem at fastify/help#607 ?
test/internals/reply.test.js
Outdated
} | ||
const reply = new Reply(response, { raw: { aborted: true } }) | ||
t.equal(reply.send('hello'), reply) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add another test by using a real request?
I leave here the 2 possible solutions I came up with if anyone else needs to fix the potential issue with streams:
|
The way you have described above are not equivalent to this PR. this PR completely skips the onSend/onError hooks, while your workaround actually replace the payload - in that sense it's similar to the error that is being returned. Your workaround is something I would be more inclined to land vs just skipping all the hooks. |
I've reworked this to a much simpler solution to only handle errors coming from streams and target the next branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I rebased it on top of main as we merged next in main. |
Can you rebase? There is a lint error that I am not seeing on the current |
138dae2
to
52a42a2
Compare
I thought I have, that's why i didn't understand the lint error. It's done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
If a request was aborted before calling the reply.send method, it won't try to send a response since no one is waiting for it. This way logging errors like ERR_STREAM_PREMATURE_CLOSE can be avoided too.
Full discussion: fastify/help#607
Checklist
npm run test
andnpm run benchmark
and the Code of conduct