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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 56 additions & 0 deletions doc/api/http.md
Expand Up @@ -676,6 +676,62 @@ Removes a header that's already defined into headers object.
request.removeHeader('Content-Type');
```

### request.reusedSocket

<!-- YAML
added: REPLACEME
-->

* {boolean} Whether the request is send through a reused socket.

When sending request through a keep-alive enabled agent, the underlying socket
might be reused. But if server closes connection at unfortunate time, client
may run into a 'ECONNRESET' error.

```js
const http = require('http');

// Server has a 5 seconds keep-alive timeout by default
http
.createServer((req, res) => {
res.write('hello\n');
res.end();
})
.listen(3000);

setInterval(() => {
// Adapting a keep-alive agent
http.get('http://localhost:3000', { agent }, (res) => {
res.on('data', (data) => {
// Do nothing
});
});
}, 5000); // Sending request on 5s interval so it's easy to hit idle timeout
```

By marking a request whether it reused socket or not, we can do
automatic error retry base on it.

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

function retriableRequest() {
const req = http
.get('http://localhost:3000', { agent }, (res) => {
// ...
})
.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.

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.

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.

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.

retriableRequest();
}
});
}

retriableRequest();
```

### request.setHeader(name, value)
<!-- YAML
added: v1.6.0
Expand Down
1 change: 1 addition & 0 deletions lib/_http_agent.js
Expand Up @@ -341,6 +341,7 @@ Agent.prototype.keepSocketAlive = function keepSocketAlive(socket) {

Agent.prototype.reuseSocket = function reuseSocket(socket, req) {
debug('have free socket');
req.reusedSocket = true;
socket.ref();
};

Expand Down
1 change: 1 addition & 0 deletions lib/_http_client.js
Expand Up @@ -195,6 +195,7 @@ function ClientRequest(input, options, cb) {
this.upgradeOrConnect = false;
this.parser = null;
this.maxHeadersCount = null;
this.reusedSocket = false;

var called = false;

Expand Down
9 changes: 6 additions & 3 deletions test/parallel/test-http-agent-keepalive.js
Expand Up @@ -63,7 +63,8 @@ function checkDataAndSockets(body) {

function second() {
// Request second, use the same socket
get('/second', common.mustCall((res) => {
const req = get('/second', common.mustCall((res) => {
assert.strictEqual(req.reusedSocket, true);
assert.strictEqual(res.statusCode, 200);
res.on('data', checkDataAndSockets);
res.on('end', common.mustCall(() => {
Expand All @@ -80,7 +81,8 @@ function second() {

function remoteClose() {
// Mock remote server close the socket
get('/remote_close', common.mustCall((res) => {
const req = get('/remote_close', common.mustCall((res) => {
assert.deepStrictEqual(req.reusedSocket, true);
assert.deepStrictEqual(res.statusCode, 200);
res.on('data', checkDataAndSockets);
res.on('end', common.mustCall(() => {
Expand Down Expand Up @@ -120,7 +122,8 @@ function remoteError() {
server.listen(0, common.mustCall(() => {
name = `localhost:${server.address().port}:`;
// Request first, and keep alive
get('/first', common.mustCall((res) => {
const req = get('/first', common.mustCall((res) => {
assert.strictEqual(req.reusedSocket, false);
assert.strictEqual(res.statusCode, 200);
res.on('data', checkDataAndSockets);
res.on('end', common.mustCall(() => {
Expand Down