You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Network request fails after event loop is blocked for a few seconds (regression in v20) #54293
Darwin MacBook.my.network 23.6.0 Darwin Kernel Version 23.6.0: Fri Jul 5 17:54:52 PDT 2024; root:xnu-10063.141.1~2/RELEASE_ARM64_T8103 arm64
Subsystem
http
What steps will reproduce the bug?
Block the event loop for 10 seconds via a long-running operation
Make a network request via http.request()
Code example:
consthttp=require("node:http");constmakePostRequest=()=>newPromise((resolve,reject)=>{constonResponse=(res)=>{letdata="";res.on("data",(chunk)=>{data+=chunk;});res.on("end",()=>resolve(data));};constreq=http.request("http://127.0.0.1:3000/",{method: "POST"},onResponse,);req.on("error",(e)=>reject(e));req.end();});constmakeRequests=async()=>{awaitmakePostRequest();console.log("First request worked");consttimestamp=Date.now();// Block the event loop for 10 secondswhile(Date.now()<timestamp+10000);// Enable the following line and it will work:// await new Promise((resolve) => setTimeout(resolve, 10));awaitmakePostRequest();console.log("It crashes before it gets here");};constserver=http.createServer((req,res)=>{console.log("Request received");res.statusCode=200;res.setHeader("Content-Type","text/plain");res.end("Hello world!\n");});server.listen(3000,"127.0.0.1",()=>{console.log("Server up");makeRequests();});
How often does it reproduce? Is there a required condition?
Reproduces always.
What is the expected behavior? Why is that the expected behavior?
The network request works.
What do you see instead?
The network request fails with ECONNRESET.
The network request does not reach the server.
Log from running the code above:
Server up
Request received
First request worked
node:internal/process/promises:391
triggerUncaughtException(err, true /* fromPromise */);
^
Error: read ECONNRESET
at TCP.onStreamRead (node:internal/stream_base_commons:218:20) {
errno: -54,
code: 'ECONNRESET',
syscall: 'read'
}
Node.js v20.16.0
Process finished with exit code 1
Additional information
This is a regression. The code works on Node.js 18. It fails on Node.js 20.
If we wait for 10ms before running the network request, it works.
// repro.jsconsthttp=require("node:http");constmakePostRequest=()=>newPromise((resolve,reject)=>{constonResponse=(res)=>{letdata="";res.on("data",(chunk)=>{data+=chunk;});res.on("end",()=>resolve(data));};constreq=http.request("http://127.0.0.1:3000/",{method: "POST"},onResponse,);req.on("error",(e)=>reject(e));req.end();});constmakeRequests=async()=>{awaitmakePostRequest();console.log("First request worked");consttimestamp=Date.now();// Block the event loop for 10 secondswhile(Date.now()<timestamp+10000);// Enable the following line and it will work:// await new Promise((resolve) => setTimeout(resolve, 10));awaitmakePostRequest();console.log("It crashes before it gets here");};constserver=http.createServer((req,res)=>{console.log("Request received");res.statusCode=200;res.setHeader("Content-Type","text/plain");res.end("Hello world!\n");});server.listen(3000,"127.0.0.1",()=>{console.log("Server up");makeRequests();});
This is caused by keepAlive, which is true by default after v18. I think this PR made that globally true for httpAgent and this issue surfaced.
If you reduce the while loop to less than 5 seconds, or add a header connection: 'close', the problem goes away.
Since the event loop was blocked and the socket destroy actions were scheduled in the next tick, your next request would be assigned to the socket to be destroyed.
IMHO, the best fix is probably about how to detect this at the request stage
Not suggesting Connection: close as a proper workaround, as it does not persist sockets.
Overall, I think this is an limitation in keepAlive or underlying socket implementation. As according to RFC 9112 standard, socket should not be closed immediately.
To avoid the TCP reset problem, servers typically close a connection in stages. First, the server performs a half-close by closing only the write side of the read/write connection. The server then continues to read from the connection until it receives a corresponding close by the client, or until the server is reasonably certain that its own TCP stack has received the client's acknowledgement of the packet(s) containing the server's last response. Finally, the server fully closes the connection.
Jason (@jazelly) and myself have spent some time to dig into this issue further more.
The reason why the second request failed is because the socket is no longer kept alive after 5 seconds and server tries to destroy the socket (this is asynchronous) however client side does not know the socket is being destroyed and still tries to send an request by reusing the same socket.
What happens next is that the request was assigned to the old socket by http agent at client side while the server is about to close the socket. The server then closed the socket and the request failed. connection: close in the header to force to open a new socket is one way to fix but not the optimal way as open up a socket is more costly, another workaround is always retrying the request.
You can find the similar issue here, hope it helps and please let us know if you have any other questions.
Collaborators with write permissions can help run CI for the PR. I don't have the permission but I believe once the CI is passed it will be landed soon.
Activity
avivkeller commentedon Aug 9, 2024
trivikr commentedon Aug 10, 2024
Reproducible in
v20.0.0Not reproducible in
v18.20.4jazelly commentedon Aug 10, 2024
This is caused by
keepAlive, which is true by default after v18. I think this PR made that globally true forhttpAgentand this issue surfaced.If you reduce the while loop to less than 5 seconds, or add a header
connection: 'close', the problem goes away.Since the event loop was blocked and the socket destroy actions were scheduled in the next tick, your next request would be assigned to the socket to be destroyed.
IMHO, the best fix is probably about how to detect this at the request stage
nikwen commentedon Aug 11, 2024
Thank you so much for the information, @jazelly! I fixed the issue in my application by disabling
keepAlivefor now.Agree that your suggested approach sounds good:
jazelly commentedon Aug 22, 2024
Not suggesting
Connection: closeas a proper workaround, as it does not persist sockets.Overall, I think this is an limitation in
keepAliveor underlying socket implementation. As according to RFC 9112 standard, socket should not be closed immediately.jakecastelli commentedon Aug 25, 2024
Jason (@jazelly) and myself have spent some time to dig into this issue further more.
The reason why the second request failed is because the socket is no longer kept alive after 5 seconds and server tries to destroy the socket (this is asynchronous) however client side does not know the socket is being destroyed and still tries to send an request by reusing the same socket.
What happens next is that the request was assigned to the old socket by http agent at client side while the server is about to close the socket. The server then closed the socket and the request failed.
connection: closein the header to force to open a new socket is one way to fix but not the optimal way as open up a socket is more costly, another workaround is always retrying the request.You can find the similar issue here, hope it helps and please let us know if you have any other questions.
nikwen commentedon Aug 26, 2024
Thanks for digging further into it, @jakecastelli and @jazelly!
Let me summarize:
ECONNRESET.Damn, that sucks. It means there's little we can do except for retrying.
Thanks for investigating and thanks for sharing! I appreciate the time you put into this. 🙌
jazelly commentedon Aug 28, 2024
I found this PR that will fix this issue. I verified it in my local env.
nikwen commentedon Aug 28, 2024
@jazelly That's awesome, thanks! Do you know the right process to bring attention back to the PR? It seems stale.
jazelly commentedon Aug 28, 2024
Collaborators with write permissions can help run CI for the PR. I don't have the permission but I believe once the CI is passed it will be landed soon.
nikwen commentedon Aug 28, 2024
Thanks, @jazelly, for getting the PR to the attention of the right people. :)
http: reduce likelihood of race conditions on keep-alive timeout
http: reduce likelihood of race conditions on keep-alive timeout
http: reduce likelihood of race conditions on keep-alive timeout
http: reduce likelihood of race conditions on keep-alive timeout
http: reduce likelihood of race conditions on keep-alive timeout