http: Adding doc and debug for calling empty string on write function #22118
Conversation
doc/api/http.md
Outdated
| @@ -750,6 +750,9 @@ Returns `true` if the entire data was flushed successfully to the kernel | |||
| buffer. Returns `false` if all or part of the data was queued in user memory. | |||
| `'drain'` will be emitted when the buffer is free again. | |||
|
|
|||
| Note that when `write` is called with empty string or buffer, it does nothing and | |||
There was a problem hiding this comment.
Linter issue: 80 characters per line limit exceeded.
|
@nodejs/http |
|
@jasnell I guess its worth to add the same for |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/16604/
@antsmartian Yes in compatibility mode. |
|
Oh, sorry, missed the earlier ping. Yes, what @mcollina said :-) |
doc/api/http.md
Outdated
| @@ -750,6 +750,9 @@ Returns `true` if the entire data was flushed successfully to the kernel | |||
| buffer. Returns `false` if all or part of the data was queued in user memory. | |||
| `'drain'` will be emitted when the buffer is free again. | |||
|
|
|||
| Note that when `write` is called with empty string or buffer, it does | |||
There was a problem hiding this comment.
Nit, you can ignore this, but my mild preference: Remove Note that
There was a problem hiding this comment.
Thanks, taken care.
|
This PR needs a rebase against master to avoid the git failure in the CI. |
|
@joyeecheung Taken care. |
|
Unrelated CI failures are currently keeping this from landing. It's ready to go otherwise. |
@jasnell Needs a rebase against master to get past the CI issues. The jinja LICENSE thing is the giveaway: 12:18:15 Changes not staged for commit:
12:18:15 (use "git add <file>..." to update what will be committed)
12:18:15 (use "git checkout -- <file>..." to discard changes in working directory)
12:18:15
12:18:15 modified: deps/v8/third_party/jinja2/LICENSE
12:18:15
12:18:15 Untracked files:
12:18:15 (use "git add <file>..." to include in what will be committed)
12:18:15
12:18:15 env.propertiesOP can rebase, or any Collaborator can rebase and force push on their behalf. Start another CI and all will hopefully be better... |
|
Rebased, force-pushed. New CI: https://ci.nodejs.org/job/node-test-pull-request/16666/ |
|
Landed in 413a7e1 |
PR-URL: #22118 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #22118 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #22118 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Addresses the issue : #22066
Removing the empty check would be a breaking change, however I'm not sure why such check is in first place. May be need input from the community.
In the meantime, I thought its worth to add documentation and at the same time to add
debuglog, so that when users feelwritewith empty string or buffer hangs, they can run in debug mode to find out the reason.If the content fits for the documentation, I guess we need to add the same to
http2as well.Open to feedbacks and thoughts.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes