-
Notifications
You must be signed in to change notification settings - Fork 80
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
NSFS | Add checks for request/response aborted in namespace_fs read_object_stream() #6955
Conversation
37befc4
to
3aca1e9
Compare
75540b0
to
a6543fa
Compare
92063c0
to
3698f73
Compare
bc6df68
to
d47c6cb
Compare
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.
another round of comments. apologies for making it iterative.
5e80ebe
to
2bf3eab
Compare
src/sdk/object_sdk.js
Outdated
req.once('aborted', () => { | ||
dbg.log0('request aborted', req.url); | ||
this.abort_controller.abort(); | ||
}); |
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.
So I just noticed that the aborted event is being deprecated since nodejs 16 (we are still on 14).
https://nodejs.org/dist/latest-v16.x/docs/api/http.html#event-aborted
And the docs recommend listening for the close
event. LOL.
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.
Seems like we need to do this:
req.once('close', () => {
if (req.destroyed) {
dbg.log0('request aborted', req.url);
this.abort_controller.abort(new Error('request aborted ' + req.url));
}
});
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, so this is a gap for moving to node 16, because on node 14 we can't check the req.destroyed state yet.
FYI @liranmauda and @nimrod-becker - we need to track this so that once we switch to node 16 we make sure to include this fix, which @romayalon left as a comment in the code.
@romayalon I resolved the previous comments, but the deprecation of the abort event on the request is probably best to fix... |
Signed-off-by: Romy <[email protected]>
77f4403
to
777c722
Compare
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.
Great work @romayalon. Super artistic and accurate as usual! Godspeed.
@guymguym Thank you for your great help and profound code review! |
Signed-off-by: Romy [email protected]
Explain the changes
1.1. setup_abort_controller() - setup event listeners on the req/res that will call abort() of the abort_controller.
1.2. throw_if_aborted() - will check if the abort_controller got the aborted signal and will throw error when true.
2.1. Called setup_abort_controller() in order to hang the event listeners on each get_object request/response.
2.2. Removed the previous event listeners code.
3.1. Added a call to object_sdk.throw_if_aborted() after each await in read_object_stream(). (see Note)
3.2. Passed object_sdk.abort_controller.signal parameter to wait_finished() and to wait_drain().
NOTE -
The whole purpose of this new abort_controller functionality was to fix the following situation -
Issues: Fixed #xxx / Gap #xxx
Testing Instructions: