-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
if (req.reusedSocket && err.code === 'ECONNRESET') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could retry infinitely depending on what causes the econnreset? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right. Disregard my comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this example should at least also check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 | ||
|
There was a problem hiding this comment.
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.