Skip to content
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

Do not send empty TCP packet in _http_outgoing.js #3947

Closed
indutny opened this issue Nov 20, 2015 · 10 comments
Closed

Do not send empty TCP packet in _http_outgoing.js #3947

indutny opened this issue Nov 20, 2015 · 10 comments
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem.

Comments

@indutny
Copy link
Member

indutny commented Nov 20, 2015

After 99943e1 we send empty TCP packets on res.end() in quite many cases. I don't think that it is a serious thing, but it is definitely worth fixing.

@indutny
Copy link
Member Author

indutny commented Nov 20, 2015

See #3803 for reference

@indutny indutny added http Issues or PRs related to the http subsystem. feature request Issues that request new features to be added to Node.js. labels Nov 20, 2015
@jasnell
Copy link
Member

jasnell commented Nov 21, 2015

+1 I'd noticed that before too but hadn't managed to get around to fixing it.
/cc @nodejs/http

@indutny
Copy link
Member Author

indutny commented Nov 21, 2015

Yeah, it is kind of a way to emit finish event only after completion of all previous writes.

@trevnorris
Copy link
Contributor

Is this a feature request or a bug?

@indutny
Copy link
Member Author

indutny commented Nov 23, 2015

More like a feature request with the elements of bug fixing.

@Trott
Copy link
Member

Trott commented Jun 7, 2016

Is there a way to reproduce this not-exactly-a-bug reliably?

@indutny
Copy link
Member Author

indutny commented Jun 8, 2016

I think yes.

@Trott
Copy link
Member

Trott commented Jul 7, 2017

Probably don't want to close this, right? But no one's actively working on it either? Is someone available to mentor an interested potential-contributor who might want to try to implement this?

@bnoordhuis
Copy link
Member

Has this been fixed? I added a judicious assert (see diff) and I can't reproduce.

diff --git a/deps/uv/src/unix/stream.c b/deps/uv/src/unix/stream.c
index 7b23d16..80805e5 100644
--- a/deps/uv/src/unix/stream.c
+++ b/deps/uv/src/unix/stream.c
@@ -1444,6 +1444,7 @@ int uv_write2(uv_write_t* req,
   memcpy(req->bufs, bufs, nbufs * sizeof(bufs[0]));
   req->nbufs = nbufs;
   req->write_index = 0;
+  assert(0 != uv__count_bufs(bufs, nbufs));
   stream->write_queue_size += uv__count_bufs(bufs, nbufs);
 
   /* Append the request to write_queue. */

(It does break some REPL and child process tests but that's expected. The salient part is that no zero-sized TCP writes seem to take place.)

@apapirovski
Copy link
Member

This seems to be addressed from what I can tell. Please feel free to re-open if you think I'm wrong or this needs to remain open to tackle some edge cases.

This was referenced Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants