Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
http: always call response.write() callback#27709
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
Ensure that the callback of `OutgoingMessage.prototype.write()` is called even when writing empty chunks. Fixes: nodejs#22066
nodejs-github-bot commented May 15, 2019
lpinca commented May 15, 2019
This is an attempt to fix #22066.
The downside is that internal empty writes like Line 714 in 53bef42
socket.write() call which in turn calls into C++. This could have an impact on performance if writev is not supported. A possible way to mitigate the issue is to not call writeGeneric() if the chunk is empty here Line 696 in 53bef42
If we don't care about the callbacks order, we can simplify everything and just call the callback of |
addaleax left a comment
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 think this is my preferred approach as well 👍
nodejs-github-bot commented May 18, 2019
nodejs-github-bot commented May 19, 2019
nodejs-github-bot commented May 19, 2019
nodejs-github-bot commented May 19, 2019
addaleax commented May 19, 2019
Landed in 0df581c |
Ensure that the callback of `OutgoingMessage.prototype.write()` is called even when writing empty chunks. Fixes: #22066 PR-URL: #27709 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
Ensure that the callback of `OutgoingMessage.prototype.write()` is called when `outgoingMessage._hasBody` is `false` (HEAD method, 204 status code, etc.). Refs: nodejs#27709
Ensure that the callback of `OutgoingMessage.prototype.write()` is called even when writing empty chunks. Fixes: #22066 PR-URL: #27709 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]>
Ensure that the callback of `OutgoingMessage.prototype.write()` is called when `outgoingMessage._hasBody` is `false` (HEAD method, 204 status code, etc.). Refs: nodejs#27709 PR-URL: nodejs#27777 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Ensure that the callback of `OutgoingMessage.prototype.write()` is called when `outgoingMessage._hasBody` is `false` (HEAD method, 204 status code, etc.). Refs: #27709 PR-URL: #27777 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]>
awwright commented Sep 24, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
What's the test case that this fixes? This PR seemed to introduce a bug where the callback for OutgoingMessage#end is not always called, since the callback for Writable#write doesn't always call if passed an empty string (where as before this PR, OutgoingMessage#_writeRaw did guarantee this). But this PR was supposed to fix such an issue, so I'm not sure what's going on here. |
lpinca commented Sep 24, 2019
@awwright see #22066 (comment) and included tests. |
awwright commented Sep 26, 2019
But #22066 (comment) is incorrect, Stream#write has no guarantee in the code that the callback will always be called, see #29649 for an example |
Ensure that the callback of
OutgoingMessage.prototype.write()iscalled even when writing empty chunks.
Fixes: #22066
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes