-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Http connections aborted after 5s / keepAliveTimeout #13391
Comments
/cc @indutny @tshemsedinov @aqrln |
Thanks for the ping, I'll be able look into this in a few hours. |
Thanks for bringing this up @pbininda. We hit this issue as well when running requests longer than a few seconds. We also see multiple POSTs effecting all browsers on 8 ... reverted back to 7.10 and all is well. Seems like a pretty pernicious bug. We're using Koa 1.x middleware on Linux. |
Same behaviour here; request bit longer than usual and the browser kept resending the request. A general remark is that Firefox and Chrome did resend the request, but Safari didn't. Using Express server 4.15.2. Finally, we reverted to 7.10 and is working normally. |
Thanks for the note regarding Safari, I'll change the wording regarding "all major browsers" 😓 |
Ugh... sorry for the delay, I was more busy that I hoped for. Let's fix it today. Thanks a lot for a detailed report and reproduction! |
Confirmed issue still present in release 8.1 as well. |
@pbininda @lvpro @juanecabellob I'm very sorry for not making it in time for the 8.1 release. I took a look at the reproduction back then, but didn't have an opportunity to debug it until now. #13549 should fix it. |
Don't be sorry @aqrln! Thank you very much for getting this resolved! :) |
@aqrln No problem, I can keep the |
Fix the logic of resetting the socket timeout of keep-alive HTTP connections and add two tests: * `test-http-server-keep-alive-timeout-slow-server` is a regression test for nodejsGH-13391. It ensures that the server-side keep-alive timeout will not fire during processing of a request. * `test-http-server-keep-alive-timeout-slow-headers` ensures that the regular socket timeout is restored as soon as a client starts sending a new request, not as soon as the whole message is received, so that the keep-alive timeout will not fire while, e.g., the client is sending large cookies. Refs: nodejs#2534 Fixes: nodejs#13391
Fix the logic of resetting the socket timeout of keep-alive HTTP connections and add two tests: * `test-http-server-keep-alive-timeout-slow-server` is a regression test for GH-13391. It ensures that the server-side keep-alive timeout will not fire during processing of a request. * `test-http-server-keep-alive-timeout-slow-client-headers` ensures that the regular socket timeout is restored as soon as a client starts sending a new request, not as soon as the whole message is received, so that the keep-alive timeout will not fire while, e.g., the client is sending large cookies. Refs: #2534 Fixes: #13391 PR-URL: #13549 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
Fix the logic of resetting the socket timeout of keep-alive HTTP connections and add two tests: * `test-http-server-keep-alive-timeout-slow-server` is a regression test for GH-13391. It ensures that the server-side keep-alive timeout will not fire during processing of a request. * `test-http-server-keep-alive-timeout-slow-client-headers` ensures that the regular socket timeout is restored as soon as a client starts sending a new request, not as soon as the whole message is received, so that the keep-alive timeout will not fire while, e.g., the client is sending large cookies. Refs: #2534 Fixes: #13391 PR-URL: #13549 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
I have this issue too with webpack-dev-server. It somehow does not work. How would I pass the paratmeter keepAlive Timeout so that the default timeout is 20 secs? Above this is the example
I tried this:
It gets expanded to:
Unfortunately it does not have any effect. |
@MylesBorins You'll need the other commit: aaf2a1c — they probably should've landed as one or I should've indicated that the 2nd one depends on the first working properly. My bad. |
@apapirovski done landed in eeef06a |
Documenting the best way to imitate the old behavior saves time for people migrating from older versions. (E.g. for unexpected ECONNRESET) It isn't immediately obvious if earlier nodejs versions behaved the same way as nodejs 8 does with keepAliveTimeout = 0. From 0aa7ef5, it seems like they behave the same way. Related to issues such as #13391 that show up when migrating to node 8 PR-URL: #17660 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Documenting the best way to imitate the old behavior saves time for people migrating from older versions. (E.g. for unexpected ECONNRESET) It isn't immediately obvious if earlier nodejs versions behaved the same way as nodejs 8 does with keepAliveTimeout = 0. From 0aa7ef5, it seems like they behave the same way. Related to issues such as #13391 that show up when migrating to node 8 PR-URL: #17660 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Documenting the best way to imitate the old behavior saves time for people migrating from older versions. (E.g. for unexpected ECONNRESET) It isn't immediately obvious if earlier nodejs versions behaved the same way as nodejs 8 does with keepAliveTimeout = 0. From 0aa7ef5, it seems like they behave the same way. Related to issues such as #13391 that show up when migrating to node 8 PR-URL: #17660 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Documenting the best way to imitate the old behavior saves time for people migrating from older versions. (E.g. for unexpected ECONNRESET) It isn't immediately obvious if earlier nodejs versions behaved the same way as nodejs 8 does with keepAliveTimeout = 0. From 0aa7ef5, it seems like they behave the same way. Related to issues such as #13391 that show up when migrating to node 8 PR-URL: #17660 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Documenting the best way to imitate the old behavior saves time for people migrating from older versions. (E.g. for unexpected ECONNRESET) It isn't immediately obvious if earlier nodejs versions behaved the same way as nodejs 8 does with keepAliveTimeout = 0. From 0aa7ef5, it seems like they behave the same way. Related to issues such as #13391 that show up when migrating to node 8 PR-URL: #17660 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
details of bug are here: nodejs/node#13391 docs here: https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_server_keepalivetimeout tl;dr is node set a 5s idle timeout. this does what go and java both seem to do (have not checked python or ruby, though from what I know about python, we are not doing this either) and doesn't have an idle timeout. since in this case we do kinda trust that the client is using an idle timeout (it's fn), this seems like the right policy anyway (if fn dies is something to consider, but the least of our worries is fdk conns in that case, and we are killing fn spawned containers on startup too). it should also be noted the client (fn) is only using 1 conn per container.
details of bug are here: nodejs/node#13391 docs here: https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_server_keepalivetimeout tl;dr is node set a 5s idle timeout. this does what go and java both seem to do (have not checked python or ruby, though from what I know about python, we are not doing this either) and doesn't have an idle timeout. since in this case we do kinda trust that the client is using an idle timeout (it's fn), this seems like the right policy anyway (if fn dies is something to consider, but the least of our worries is fdk conns in that case, and we are killing fn spawned containers on startup too). it should also be noted the client (fn) is only using 1 conn per container.
Would this have been present in node 10.6.0? Test code above seems to pass. |
3 years and bug still exists? How's that possible? |
As with all things in Node.js, it requires someone free to work on it. Pull requests are always welcome. One thing that may be helpful is a reproduction of the issue in the form of a known_issue test that can be used to guide someone in making a fix. |
Not intended to sound rude. I'm trying to solve that riddle for hours now - maybe someone else has it as well:
My question would be: how is that possible? Seems like if send data is too big (?) it just freezes and then times out. I'm using Node 10.19.0. Thanks for your help. |
In case this helps - I think there's an additional use case which isn't related to the transmission size, but rather it's a race condition between the start / end of requests on the same connection.
Important: *I'm not 100% sure this is the correct flow - it's just my best educated-guess, based on some debugging Reproduce:
Tested Versions:
Naive Approach to a Fix
Here's a naive code listing of the second approach: // file: _http_server.js
function socketOnTimeout() {
// "this" is the socket
if (this.isInUse) {
return;
}
...
} |
Short Description
Node 8 introduced a change in the handling of http keep-alive connections. IMHO, this is (at least) a breaking change. When an http server does long-running requests (>5s), and the client requests a
Connection: keep-alive
connection, the http server closes the connection after 5s. This potentially causes browsers to re-send the request even if it is aPOST
request.To Reproduce
clone https://github.com/pbininda/node8keepAliveTimeout and
npm install
. ThenStarts a little express server (
server.js
) and a client./longpost
takes 10s).POST /longpost
with a preflightOPTIONS /longpost
.The test runs through fine on node 6 and node 7:
but fails on node 8 with
Browser Retries
It seems, most of the major browsers (Chrome, Firefox, Edge) implement https://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html#sec8.2.4. Since the server closes the connection on which it received the POST request before sending an answer, the Browsers re-send the POST. Note that you don't see the re-send in chrome dev tools but using Wireshark shows the retransmission. To have a look at this, run
which launches the server (
server.js
) and then loadbrowsertest.html
in chrome. This runsbrowsertest.js
in the browser which does a simple $.ajax request against the server. On the server side you will see:This shows, that the server received two POST requests the second one 5s after the first one, even though the browser client code only does one request.
Bug or Breaking Change?
I'm not sure if this is a bug or a breaking change. It probably got introduced through #2534. It only seems to happen when two connections are used (that's why the prefight OPTIONS is forced to happen in my code), so it may be that the wrong connection is being closed here.
Workaround
Setting the
keepAliveTimeout
(see https://nodejs.org/dist/latest-v8.x/docs/api/http.html#http_server_keepalivetimeout) of the http server to a value greater than the maximum duration of a request solves the problem. You can try this withand then in another terminal
The text was updated successfully, but these errors were encountered: