http: add reusedSocket property on client request #29715

themez · 2019-09-26T08:18:17Z

Currently It's hard to handle keep-alive connection closes at unfortunate time. This PR set ClientRequest.reusedSocket property when reusing socket for request, so user can handle retry base on wether the request is reusing a socket. Similar to what chromium did

Refs:

Request fail to retry: retry on ECONNREST broken request/request#3131
Discussion in golang for the same issue: net/http: Client returns errors on POST if keep-alive connection closes at unfortunate time golang/go#22158

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

jasnell · 2019-09-26T18:55:38Z

Not addressing the merits of the change yet, but this would need docs and unit tests. Also, can you expand on why this is helpful

themez · 2019-09-27T03:01:25Z

@jasnell Thanks for replying, I'm not sure if it's a good solution. Let me elaborate the problem first:

When doing http RPC, we normally keep the connection alive, but it's often when server closed a connection due to idle timeout, which is 5s by default, client send a request before receiving the closed event, then it would end in ECONNRESET err. Here's an example to reproduce that.

const http = require("http");
const agent = new http.Agent({ keepAlive: true });

// server has a 5 seconds default idle timeout
http
  .createServer((req, res) => {
    res.write("foo\n");
    res.end();
  })
  .listen(3000);

setInterval(() => {
  // using a keep-alive agent
  http.get("http://localhost:3000", { agent }, res => {
    res.on("data", data => {
      //ignore
    });
    res.on("end", () => {
      console.log("success");
    });
  })
}, 5000); // sending request on 5s interval so it's easy to hit idle timeout

Wait for about 20s you will get an Error: read ECONNRESET.

A possible solution is retry on such kind of unexpected connection close. But we need to determine it's a reused socket closing, or a real server reject(maybe server is not running at all). So here I add a reusedSocket property to mark a request is reusing an exists socket, which is how the request library did when using forever agent

The simplified retry checking:

const req = http
  .get("http://localhost:3000", { agent }, res => {
    // ...
  })
  .on("error", err => {
    if (req.reusedSocket && err.code === "ECONNRESET") { // check if retry is needed
      // retry
    }
  });

jasnell · 2019-09-27T14:19:51Z

@nodejs/http ... thoughts?

Trott · 2019-10-01T01:58:23Z

@nodejs/collaborators

mcollina

Would you mind adding some tests for this?

mcollina · 2019-10-01T07:46:38Z

When doing http RPC, we normally keep the connection alive, but it's often when server closed a connection due to idle timeout, which is 5s by default, client send a request before receiving the closed event, then it would end in ECONNRESET err.

I would prefer if we tracked the “closing” state in the Agent, and avoid sending a request on a socket we are closing.

themez · 2019-10-01T08:17:14Z

@mcollina No problem, I'll add some tests and docs.

I would prefer if we tracked the “closing” state in the Agent, and avoid sending a request on a socket we are closing.

We already have closing handling in Agent, but the problem is client could send out request in the time between server socket closing and client receiving closing event. So retry is inevitable in this situation.

Event sequence would be like:

server closing socket due to keep alive timeout.
client send out request on existing connection(which is just being closed on server side).
client receiving socket closed event, removing socket from agent.
client got request error.

themez · 2019-10-01T09:16:35Z

doc/api/http.md

+### request.reusedSocket
+
+<!-- YAML
+added: v12.12.0


I'm not sure what version should be put here.

It should be added: REPLACEME. The next time a release is made, tooling will replace it with the correct version number.

Trott · 2019-10-04T02:24:49Z

@nodejs/http This could use some reviews.

mcollina

LGTM

nodejs-github-bot · 2019-10-05T18:42:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25863/

themez · 2019-10-07T03:03:52Z

Shall I do something to make CI pass? @mcollina

nodejs-github-bot · 2019-10-07T03:06:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25887/

ronag · 2019-10-07T19:00:22Z

This is a bit weird to me. Eventually you will end up with every socket reused. So what’s the difference between that state and just having the flag true on every socket from the beginning?

~~I don’t think we have an idle timeout. Is this intended for the agent-keepalive package?~~ I think you can achieve the same thing in user space.

ronag · 2019-10-07T22:11:32Z

doc/api/http.md

+    })
+    .on('error', (err) => {
+      // Check if retry is needed
+      if (req.reusedSocket && err.code === 'ECONNRESET') {


This could retry infinitely depending on what causes the econnreset?

Can probably be resolved by retrying without keep alive agent and only applying this logic in the keep alive case.

It wont be infinitely retrying，the client would finally received socket closes event and remove the broken socket，next connection ‘s first request would be non-reused socket

but the next socket could have the same problem? But yea, I guess it would run out of pooled sockets eventually.

ronag · 2019-10-07T22:15:42Z

doc/api/http.md

+    })
+    .on('error', (err) => {
+      // Check if retry is needed
+      if (req.reusedSocket && err.code === 'ECONNRESET') {


Isn't req.agent.keepAlive enough here, instead of req.reusedSocket?

The difference is the first request of a keep-alive agent is based on a new created socket, which is not reused. If the first request failed with ECONNRESET, we would know the server is not listening, there's no need to retry.

I don't think it hurts to retry in that edge case but yes, I can't think of a good way to accomplish avoiding the retry without the property you suggest.

You are right. Disregard my comment.

ronag · 2019-10-07T22:22:05Z

I think this achieves what this PR intends without modifying core:

const agent = new http.Agent({ keepAlive: true });
function retriableRequest(url) {
  return new Promise((resolve, reject) => http
    .get(url, { agent }, resolve)
    .on('error', (err) => {
      // Check if retry is needed.
      // Retry if:
      // - Connection was reset.
      // - We have not received a response.
      // - Socket might have been re-used through keep alive agent.
      if (err.code === 'ECONNRESET' && !req.res && req.agent.keepAlive) {
         // Retry without keep alive to avoid infinite loop.
         http
           .get(url, { agent: null }, resolve)
           .on('error', reject);
      } else {
        reject(err);
      }
    }
  );
}

There is one case that neither this PR or my suggestion addresses... what if it was a non idempotent request and the server actually did receive and act on it before ECONNRESET? Is it still safe to retry?

e.g.

async function handle(req, res) {
  if (req.method === 'POST') {
     await createResource(req);
     // everything fine so far
     // oops... server crash or various other reasons that might cause a ECONNRESET
  }
}

Which begs the question. Should keepAlive be used for non-idempotent methods?

themez · 2019-10-08T02:17:22Z

@ronag I agree with you on the idempotent point, it's not totally safe to retry on an ECONNRESET. That's why I only add a reusedSocket in the core, user can do retry base on the use case.

For example, the popular request library thought it's OK in practical to retry on ECONNRESET even there's a chance to duplicate non-idempotent transaction.

Or user can just retry on GET request.

if (err.code === 'ECONNRESET' && req.reusedSocket && req.method === 'GET')

Which is better but not totally safe also.

ronag · 2019-10-08T06:39:55Z

Node http collaborators. This is kind of a data race in regards to keep alive connections. Does the spec say anything about this? The way it is now it seems like the server should never close any socket that might be in a client's agent pool.

ronag · 2019-10-08T06:58:19Z

~~I'm not convinced about the merits of adding this but I can't think of another way to avoid a potentially unnecessary retry. +0.~~

ronag · 2019-10-08T07:00:58Z

ronag · 2019-10-08T07:10:39Z

doc/api/http.md

+      // ...
+    })
+    .on('error', (err) => {
+      // Check if retry is needed


Should probably include a note about idempotent methods.

ronag · 2019-10-08T07:11:54Z

doc/api/http.md

+    })
+    .on('error', (err) => {
+      // Check if retry is needed
+      if (req.reusedSocket && err.code === 'ECONNRESET') {


I think this example should at least also check for !req.res and not retry without keep alive in case the ECONNRESET is caused by something else.

I don't think !req.res would be non-nil when got an ECONNRESET error, and for a non-keep-alive agent, req.reusedSocket would never be true.

I don't think !req.res would be non-nil when got an ECONNRESET error,

That's an implementation detail that might change in the future. There are PR's that might or might not be merged that changes this.

themez · 2019-10-08T09:09:12Z

@ronag the http spec said something about the "data race" but didn't give a solution.

A client, server, or proxy MAY close the transport connection at any
time. For example, a client might have started to send a new request
at the same time that the server has decided to close the "idle"
connection. From the server's point of view, the connection is being
closed while it was idle, but from the client's point of view, a
request is in progress.

The reusedSocket solution is not perfect but I think it might be practical in the current situation.
While golang have another aproach to solve this problem by respecting an x-idempotency-key header, which I think could be considered also.

ronag · 2019-10-08T09:23:53Z

@ronag the http spec said something about the "data race" but didn't give a solution.

That's unfortunate.

Chromium seems to assume that a 408 reply is sent before the socket is closed, is that a possible solution? i.e. if the server is about to close the socket due to timeout, first push out a 408 timed out on the wire which the client would detect in the case of this race condition?

themez · 2019-10-08T10:01:16Z

@ronag I think 408 and ECONNRESET are different cases, see how chromium handle connection reset

ronag · 2019-10-08T12:02:14Z

see how chromium handle connection reset

Interesting, they seem to always retry regardless of idempotency?

ronag · 2019-10-08T12:05:53Z

@mcollina: I would consider if it might be appropriate to handle this race condition edge case in the Node core client code? I don't think most users consider this and I would hope there is at least something we could do to help users here, e.g. similar to @themez example retry automatically in certain situations?

starkwang · 2019-10-12T02:15:51Z

@themez This PR is about to land. User name and email address in these commits seems not be linked to your Github account. Would you like to change the name/email in commits, or add the email to your Github email list ?

You can take a look at: https://github.com/nodejs/node/blob/99b1ada/doc/guides/contributing/pull-requests.md#step-1-fork

Set ClientRequest.reusedSocket property when reusing socket for request, so user can handle retry base on wether the request is reusing a socket. Refs: request/request#3131

themez · 2019-10-12T02:54:06Z

@starkwang Looks I was using an old invalid git config by mistake, however, I've changed the email address in commits now, except the first commit looks odd, do you think it is normal?

starkwang · 2019-10-12T02:58:37Z

@themez That's fine.

nodejs-github-bot · 2019-10-12T03:00:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/25978/

Set ClientRequest.reusedSocket property when reusing socket for request, so user can handle retry base on wether the request is reusing a socket. Refs: request/request#3131 PR-URL: #29715 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com>

starkwang · 2019-10-12T03:57:59Z

Landed in 8915b15

Set ClientRequest.reusedSocket property when reusing socket for request, so user can handle retry base on wether the request is reusing a socket. Refs: request/request#3131 PR-URL: #29715 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Sep 26, 2019

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 26, 2019

mcollina reviewed Oct 1, 2019

View reviewed changes

themez commented Oct 1, 2019

View reviewed changes

mcollina approved these changes Oct 4, 2019

View reviewed changes

benjamingr approved these changes Oct 4, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 5, 2019

ronag reviewed Oct 7, 2019

View reviewed changes

ronag reviewed Oct 8, 2019

View reviewed changes

ronag approved these changes Oct 9, 2019

View reviewed changes

starkwang approved these changes Oct 11, 2019

View reviewed changes

themez added 4 commits October 12, 2019 10:26

http: add reusedSocket property on client request

21ef016

Set ClientRequest.reusedSocket property when reusing socket for request, so user can handle retry base on wether the request is reusing a socket. Refs: request/request#3131

add tests and docs

f198794

fix lint

ebf41bf

fix doc version

cd3ed01

starkwang closed this Oct 12, 2019

starkwang mentioned this pull request Oct 12, 2019

Add reusedSocket property on client request node-modules/agentkeepalive#82

Merged

themez mentioned this pull request Oct 14, 2019

retry on ECONNREST broken request/request#3131

Closed

BethGriggs mentioned this pull request Oct 21, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

http: add reusedSocket property on client request #29715

http: add reusedSocket property on client request #29715

Conversation

themez commented Sep 26, 2019 • edited Loading

Checklist

jasnell commented Sep 26, 2019

themez commented Sep 27, 2019 • edited Loading

jasnell commented Sep 27, 2019

Trott commented Oct 1, 2019

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Oct 1, 2019

themez commented Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Oct 4, 2019

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 5, 2019

themez commented Oct 7, 2019

nodejs-github-bot commented Oct 7, 2019

ronag commented Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

ronag Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themez Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronag commented Oct 7, 2019 • edited Loading

themez commented Oct 8, 2019 • edited Loading

ronag commented Oct 8, 2019

ronag commented Oct 8, 2019 • edited Loading

ronag commented Oct 8, 2019 • edited Loading

ronag Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

ronag Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themez commented Oct 8, 2019

ronag commented Oct 8, 2019

themez commented Oct 8, 2019

ronag commented Oct 8, 2019

ronag commented Oct 8, 2019 • edited Loading

starkwang commented Oct 12, 2019

themez commented Oct 12, 2019

starkwang commented Oct 12, 2019

nodejs-github-bot commented Oct 12, 2019

starkwang commented Oct 12, 2019

themez commented Sep 26, 2019 •

edited

Loading

themez commented Sep 27, 2019 •

edited

Loading

themez commented Oct 1, 2019 •

edited

Loading

ronag commented Oct 7, 2019 •

edited

Loading

ronag Oct 7, 2019 •

edited

Loading

themez Oct 8, 2019 •

edited

Loading

ronag commented Oct 7, 2019 •

edited

Loading

themez commented Oct 8, 2019 •

edited

Loading

ronag commented Oct 8, 2019 •

edited

Loading

ronag commented Oct 8, 2019 •

edited

Loading

ronag Oct 8, 2019 •

edited

Loading

ronag Oct 8, 2019 •

edited

Loading

ronag commented Oct 8, 2019 •

edited

Loading