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
fix: wait a macrotick to resume without pipelining #1465
Conversation
This change makes the Client wait for a full macrotick before executing up the next request if pipelining is disabled. This is to account for socket errors events that might be waiting to be processed in the event queue. This is the expected behavior without pipelining. This will slow us down a bit without pipelinig. Fixes: #1415 Signed-off-by: Matteo Collina <hello@matteocollina.com>
Codecov Report
@@ Coverage Diff @@
## main #1465 +/- ##
==========================================
+ Coverage 94.51% 94.54% +0.02%
==========================================
Files 49 49
Lines 4269 4271 +2
==========================================
+ Hits 4035 4038 +3
+ Misses 234 233 -1
Continue to review full report at Codecov.
|
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.
Awesome, thanks @mcollina! Yes, this solves my issue and looks sensible to me 👍
} else if (client[kPipelining] === 1) { | ||
// We must wait a full event loop cycle to reuse this socket to make sure | ||
// that non-spec compliant servers are not closing the connection even if they | ||
// said they won't. |
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.
Just nitpicking this comment: I think closing the connection without warning is actually spec-complaint for the server. Servers are free to close connections on a whim, no matter what. Persistence is encouraged where possible, but not strictly required at all times.
From https://httpwg.org/specs/rfc7230.html#rfc.section.6.5:
A client, server, or proxy MAY close the transport connection at any time.
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.
The server in the text/example close the connection while sending Connection: Keep-Alive
header, which is in contrast with https://datatracker.ietf.org/doc/html/rfc7230#section-6.3/. Basically the server is communicating to the client that it supports persistent connections while it does not as it closes every incoming connection immediately.
Implementing the behavior you want in a spec-compliant way would be to set a Connection: close
header. This is already covered in undici.
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.
@ronag wdyt?
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 agree with @mcollina
* Wait a macrotick to resume without pipelining This change makes the Client wait for a full macrotick before executing up the next request if pipelining is disabled. This is to account for socket errors events that might be waiting to be processed in the event queue. This is the expected behavior without pipelining. This will slow us down a bit without pipelinig. Fixes: nodejs#1415 Signed-off-by: Matteo Collina <hello@matteocollina.com> * fixup Signed-off-by: Matteo Collina <hello@matteocollina.com>
* Wait a macrotick to resume without pipelining This change makes the Client wait for a full macrotick before executing up the next request if pipelining is disabled. This is to account for socket errors events that might be waiting to be processed in the event queue. This is the expected behavior without pipelining. This will slow us down a bit without pipelinig. Fixes: nodejs#1415 Signed-off-by: Matteo Collina <hello@matteocollina.com> * fixup Signed-off-by: Matteo Collina <hello@matteocollina.com>
* Wait a macrotick to resume without pipelining This change makes the Client wait for a full macrotick before executing up the next request if pipelining is disabled. This is to account for socket errors events that might be waiting to be processed in the event queue. This is the expected behavior without pipelining. This will slow us down a bit without pipelinig. Fixes: nodejs#1415 Signed-off-by: Matteo Collina <hello@matteocollina.com> * fixup Signed-off-by: Matteo Collina <hello@matteocollina.com>
This change makes the Client wait for a full macrotick before
executing up the next request if pipelining is disabled.
This is to account for socket errors events that might be waiting
to be processed in the event queue. This is the expected behavior
without pipelining.
This will slow us down a bit without pipelinig.
Fixes: #1415
Signed-off-by: Matteo Collina hello@matteocollina.com