-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
"socket hang up" / ECONNRESET on consecutive requests with Node.js 19 and Node.js 20 #1735
"socket hang up" / ECONNRESET on consecutive requests with Node.js 19 and Node.js 20 #1735
Comments
Instead, we rely on the underlying http implementation in Node.js to handle this, as per the documentation at https://nodejs.org/api/http.html#new-agentoptions This fixes node-fetch#1735 and likely replaces node-fetch#1473 The original change introducing this provided no clear motivation for the override, and the implementation has since been changed to disable this header when an agent is provided, so I think there is sufficient evidence that removing this is the correct behaviour. node-fetch@af21ae6 node-fetch@7f68577
We're seeing this behavior in Axios v1.4.0 as well. I'm not sure it's specific to fetch. The "agent" workaround fixes it; the "setTimeout" one didn't help. |
This comment was marked as outdated.
This comment was marked as outdated.
@mbrevda - indeed it should! I've updated it, thanks. Good spot. |
Same problem here after updating node-fetch from version 2.6.7 to 2.6.11 and running newman collection tests in Azure pipelines |
Hi ✖ Updating... Any kind of help will be appreciated. |
This happens on the native node fetch too. I'm able to document it if it's of any help. I'm about to try if keep-alive will fix it. |
Instead, we rely on the underlying http implementation in Node.js to handle this, as per the documentation at https://nodejs.org/api/http.html#new-agentoptions This fixes #1735 and likely replaces #1473 The original change introducing this provided no clear motivation for the override, and the implementation has since been changed to disable this header when an agent is provided, so I think there is sufficient evidence that removing this is the correct behaviour. af21ae6 7f68577
🎉 This issue has been resolved in version 3.3.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
For those commenting on this issue cropping up in Axios and other libraries, this is due to the ticket here: nodejs/node#47130 (comment) - and poor handling of connection closing states in node itself. That is related to, but perhaps separate from the node-fetch issue, which can reliably trigger this poor handling of closing states - to make node-fetch sadly unusable in some situations. In other news - this has been merged and released in |
Instead, we rely on the underlying http implementation in Node.js to handle this, as per the documentation at https://nodejs.org/api/http.html#new-agentoptions This fixes node-fetch#1735 and likely replaces node-fetch#1473 The original change introducing this provided no clear motivation for the override, and the implementation has since been changed to disable this header when an agent is provided, so I think there is sufficient evidence that removing this is the correct behaviour. node-fetch@af21ae6 node-fetch@7f68577 This commit is backported to the v2 branch from node-fetch#1736 against v3.
Hi @dhedey , I encountered the same "socket hang up" error with node-fetch 2.7.0 and node 20.17. Do you know if this issue is fixed in any newer NodeJS versions ? |
@epretha I don't believe it's been fixed. As per this thread, you can use undici instead of the native http agents. |
@epretha It's not fixed, I just encountered this error myself and been battling with it for quite a while. Thanks @dhedey for the great context on it. We're not using node-fetch, so it must be solely node-related. What we did is a function that can retry the fetch if it fails, as we have a library that needs to run both in node and the browser. This made the trick and users know that they need to set 1 retry if they encounter this issue. |
Curious, why do you not just use native fetch? it does not have this issue |
Overview
When using
node-fetch
on Node.js 19+ to make two consecutive requests to the same web server, we always get the error:FetchError: request to <URL> failed, reason: socket hang up
Details
This error only arises with
node-fetch
and Node.js 19+ (tested using 19.9.0 and 20.0.0), and the web server we were using was Axum. We didn't see this issue when using a node-based proxy to help with debugging - I imagine it depends slightly on the order of TCP packet delivery and latency and the internal state machines of the HTTP servers - but it seems like many HTTP servers would see this issue.The error appears to only occur where the second request to the same server in the same round of the event loop as the first request was awaited, which gives the error:
Explanation
Root cause
We believe the following explains what we're seeing, and is backed up by logs at the end of this post:
fetch
sends aconnection: close
header in its first HTTP request...fetch
hands back to the caller.FIN
flag. This marks the end of the connection from the server's point of view. At this point, it does not expect to receive any further requests.Still in the same event loop, the caller calls fetch again to make its second HTTP request to the same origin.
conn reset
at the TCP layersocket hang up
And I believe this may be the cause:
close callbacks
phase)poll
stage, and haven't yet handled the socket close event...close callbacks
stage?), so attempts to send the HTTP request on the same (as yet unclosed) connection.fetch
raising thesocket hang
error.Basically it's a race condition between the server closing the connection and the client sending the second request... but it happens reproducibly 100% of the time, and is caused by buggy behaviour, possibly in the Node.js agent?
I would imagine that Node.js 19 changed some internal handling of socket closes which delayed the handling of the close when a connection close http request is sent, and introduced - but I'm not sure if it's Node.js or
node-fetch
at fault.Actions
Workarounds
The following both appeared to work - but for what it's worth, I don't think either of these is a "fix" because it's no longer an easy isomorphic drop in for the browser fetch.
await new Promise((resolve) => setTimeout(resolve, 0))
before every API call to try to ensure any connections are closed fixes the problem.keepalive
, eg:Suggestions for fixes
There is arguablably issues in both
node-fetch
and Node.js itself.However this is done, there are race conditions where a server can close a connection at the same time as you try to send another message on the connection - and this needs to be handled - I guess normally this is where applications implement retry logic (although it's an argument that libraries should have this - although maybe not
node-fetch
!)BUT there is a perfect storm that makes this race condition happen every time (100% reproducible for me and my colleagues):
node-fetch
is adding a header to close connections if no agent is specified.Previously, sending the
Connection: close
header explicitly didn't do anything wrong (even if it wasn't strictly needed). BUT sending a connection close down a keepalive connection isn't very nice behaviour - and trips up node's handling of the connection.The following are three key fixes that could be made - all of which would fix the issue to various degrees:
node-fetch
should not default to sending aConnection: Close
header if the default agent is set up to use keepalive... Maybe it shouldn't send the connection close header at all and should let Node.js handle it? (raised in fix: Remove the default connection close header. #1736 )Node.js
could fix itself so that if it receives aFIN
it immediately doesn't attempt to send more requests on the same connection. (mentioned in this comment: ECONNRESET while doing http 1.1 keep alive requests and server closes the connections nodejs/node#47130 as point 1)Node.js
could better pre-emptively handle the scenario where a caller sends aConnection: close
header (mentioned in this comment: ECONNRESET while doing http 1.1 keep alive requests and server closes the connections nodejs/node#47130 as point 2)Related to point the node-fetch point:
The node docs on http say this about the
keepalive
option on the agent:This suggests to me that Node.js has decent default handling, and we should be letting Node.js handle this and not take matters into our own hands by sending a confusing
Connection: close
header. By sending the header, we could be interfering with Node.js's ability to keep the socket alive if (for example) themaxSockets
are reached.Also - the fact we only set
Connection: close
on the request when a user doesn't provide an agent feels a little weird - either it's worth doing when the agent is set to keepalive or not - regardless of whether we use the default agent or not.-Finally - perhaps
node-fetch
could consider consider respecting thekeepalive
option offetch
itself and create the correct agent - with a cache - to prevent it from using thedefault
agent - or not even use thedefault
agent at all.-EDIT: Sorry - I'm wrong. The
keepalive
option on fetch is an entirely separate concept, to do with keeping the connection alive after the page ends in the browser (eg anavigator.sendBeacon
replacement) and not related to http keepalive - which makes sense - fetch is more high-level than that.Logs
TCP Dump
From running
tcpdump -A -ni lo tcp port 3333
to monitor the TCP messages between the server and thenode-fetch
client.The key parts are (see reference):
FIN
packet (seqRST
- reset connection packet.NODE_DEBUG=net
logsFrom running
NODE_DEBUG=net yarn test
The text was updated successfully, but these errors were encountered: