Skip to content
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: add reusedSocket property on client request #29715

Closed
wants to merge 4 commits into from
Closed

http: add reusedSocket property on client request #29715

wants to merge 4 commits into from

Conversation

themez
Copy link
Contributor

@themez themez commented Sep 26, 2019

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:

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Sep 26, 2019
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 26, 2019
@jasnell
Copy link
Member

jasnell commented Sep 26, 2019

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
Copy link
Contributor Author

themez commented Sep 27, 2019

@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
Copy link
Member

jasnell commented Sep 27, 2019

@nodejs/http ... thoughts?

@Trott
Copy link
Member

Trott commented Oct 1, 2019

@nodejs/collaborators

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding some tests for this?

@mcollina
Copy link
Member

mcollina commented Oct 1, 2019

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
Copy link
Contributor Author

themez commented Oct 1, 2019

@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:

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

doc/api/http.md Outdated
### request.reusedSocket

<!-- YAML
added: v12.12.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Trott
Copy link
Member

Trott commented Oct 4, 2019

@nodejs/http This could use some reviews.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott 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
@nodejs-github-bot
Copy link
Collaborator

@themez
Copy link
Contributor Author

themez commented Oct 7, 2019

Shall I do something to make CI pass? @mcollina

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Oct 7, 2019

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could retry infinitely depending on what causes the econnreset?

Copy link
Member

@ronag ronag Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@themez themez Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Disregard my comment.

@ronag
Copy link
Member

ronag commented Oct 7, 2019

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
Copy link
Contributor Author

themez commented Oct 8, 2019

@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
Copy link
Member

ronag commented Oct 8, 2019

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
Copy link
Member

ronag commented Oct 8, 2019

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.

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

@ronag ronag Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably include a note about idempotent methods.

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

@ronag ronag Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

themez commented Oct 8, 2019

@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
Copy link
Member

ronag commented Oct 8, 2019

@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
Copy link
Contributor Author

themez commented Oct 8, 2019

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

@ronag
Copy link
Member

ronag commented Oct 8, 2019

see how chromium handle connection reset

Interesting, they seem to always retry regardless of idempotency?

@ronag
Copy link
Member

ronag commented Oct 8, 2019

@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
Copy link
Contributor

@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
Copy link
Contributor Author

themez commented Oct 12, 2019

@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
Copy link
Contributor

@themez That's fine.

@nodejs-github-bot
Copy link
Collaborator

starkwang pushed a commit that referenced this pull request Oct 12, 2019
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
Copy link
Contributor

Landed in 8915b15

@starkwang starkwang closed this Oct 12, 2019
targos pushed a commit that referenced this pull request Jan 8, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants