-
-
Notifications
You must be signed in to change notification settings - Fork 935
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
Customize timeouts and generally improve the whole thing #534
Conversation
- Refactor timed-out to optimistically defer errors until after the poll phase of the current event loop tick. - Timeouts begin and end when their respective phase of the lifecycle begins and ends. - Timeouts result in a `got.TimeoutError`
@@ -188,7 +188,13 @@ Type: `number` `Object` | |||
|
|||
Milliseconds to wait for the server to end the response before aborting request with `ETIMEDOUT` error (a.k.a. `request` property). By default there's no timeout. | |||
|
|||
This also accepts an object with separate `connect`, `socket`, and `request` fields for connection, socket, and entire request timeouts. | |||
This also accepts an object with separate `lookup`, `connect`, `socket`, `response` and `request` fields to specify granular timeouts for each phase of the request. |
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.
Being able to set timeouts around each phase is more useful that timing each phase from the beginning of the request.
A concrete example is being able to time the response
phase (TTFB) when the target is a load-balanced java service which may be undergoing garbage collection. We often want to set a very aggressive response timeout in this case, so that we can retry the request immediately against another instance.
@@ -119,23 +117,23 @@ module.exports = (options = {}) => { | |||
return; | |||
} | |||
|
|||
const err = new RequestError(error, options); | |||
emitter.emit('retry', err, retried => { | |||
if (!(error instanceof GotError)) { |
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.
I wasn't sure that this was the precise change needed. My objective was to expose the TimeoutError to a client using the Promise interface.
readme.md
Outdated
- `connect` starts when `lookup` completes (or when the socket is assigned if lookup does not apply to the request) and ends when the socket is connected. | ||
- `socket` starts when the socket is connected. See [request.setTimeout](https://nodejs.org/api/http.html#http_request_settimeout_timeout_callback). | ||
- `response` starts when the socket is connected and ends when the response headers are received. | ||
- `request` starts when the request is initiated and ends when the response's end event fires. |
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.
I'm curious about the naming of response
timeout and request
timeout.
Should be these names swapped (as the timeouts say how much time can pass until it reaches the event)?
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.
I think the names should be as transparent as possible. lookup, connect and response are nice because they map directly to the event that ends the phase.
request and socket are the outliers, but I left them as they were to maximize backwards compatibility.
If we want to forego backwards compatibility, I'd suggest renaming request
to overall
or total
. socket
may be more expressive as idleSocket
, but I think it's too pedantic to justify the incompatibility.
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.
I'd suggest renaming request to overall or total.
👍
socket may be more expressive as idleSocket, but I think it's too pedantic to justify the incompatibility.
Agreed. socket
is enough IMO.
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.
@sindresorhus what do you think about changing request
to overall
or total
?
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.
Following up on this, I realized that the response timer would start too soon if the request spent significant time writing to the socket, so I implemented a fix for that by emitting a custom event on the request when the body has been written.
This also enabled the addition of a send
timer.
If we were starting from scratch I would name the send
timer request
and I would name the request
timer overall
. But, I worry that changing the meaning of those names is subtle enough to go overlooked by users, leading to bugs that can be avoided by just keeping the imperfect names.
getResponse(response, options, emitter, redirects); | ||
} catch (error) { | ||
emitter.emit('error', error); | ||
} |
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.
Can we leave this unchanged as this is being discussed in #525? Try not to duplicate PRs.
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.
I agree, this should not have been included here, but since I'm gonna accept #525, I'll allow it.
source/timed-out.js
Outdated
}); | ||
} else { | ||
req.once('response', timeResponse()); | ||
} |
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.
Love the new timed-out.js
. ❤️ 🙌
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.
Excellent piece of work. 😺
@szmarczak thanks for the flattering review :-) |
test/timeout.js
Outdated
}); | ||
|
||
test('response timeout (slow upload)', async t => { | ||
const body = fs.createReadStream('/dev/urandom', { |
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.
Ugh, I forgot about AppVeyor. I'll look for an alternative. Suggestions welcome.
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.
I made https:/sindresorhus/random-bytes-readable-stream, but I see you already solved the problem :)
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.
That looks handy :-)
- add response timeout tests for slow upload and download - add send test with keepalive - increase timeouts to mitigate intermittent false negatives
@jstewmon This is great! Thanks for doing such a thorough investigating into timeouts. 🙌 |
phase of the current event loop tick.
begins and ends.
got.TimeoutError
This includes the controversial change discussed in #525: removal of
setImmediate
around response handling and the addition ofsetImmediate
in the timeout handling. Those change are not strictly required and can be reverted if necessary and everything in this PR will still work.