-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
Call "end" after the "write" pipeline #197
Call "end" after the "write" pipeline #197
Conversation
kettanaito
commented
Apr 20, 2022
- Fixes Do not call ".end()" in the callback of ".write" #195
Please allow me to test this version of the library before merging. While this should fix the issue I'm tackling, I need to verify that in tests. I will post updates below. |
I can confirm that this change fixes the issue by running automated tests in the reproduction repository: PASS src/example.test.js
✓ works because post data is not defined (27 ms)
# The next one used to fail
✓ does not work because there is a post data (7 ms) 🎉 |
index.js
Outdated
currentRequest.end(null, null, callback); | ||
this._ending = 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.
Perhaps swap those two lines actually?
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.
Yes, definitely need to be swapped, or we can write after end in the callback
(let's add a test).
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.
You're talking about swapping _ending
and _ended
right?
this.write(data, encoding, function () {
this._ending = true;
});
self._ended = true;
currentRequest.end(null, null, callback);
This makes sense. I think I've overlooked that.
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.
@RubenVerborgh, I've swapped the order of ending/ended state.
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.
or we can write after end in the callback (let's add a test).
Not sure what you mean here. We can't call write
in the callback of end
, if that's what you're referring to. That'd throw an exception (cannot write, already ended).
Hey, @RubenVerborgh. Could you please take a look at these changes once again? |
Thanks |
#195 has been closed, so this PR is no longer needed |