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

Request timeout #308

Merged
merged 8 commits into from
May 25, 2017
Merged

Conversation

djmadeira
Copy link
Contributor

Fixes #257.

Changes timeout option so that if a number is supplied, that is the timeout for the entire request to be completed; if an object is supplied, the keys connect, socket, and request set connection, socket, and entire request timeouts, respectively.

Open questions:

  • Is "request" the right name for the full request timeout option? Should it be "response" instead?

Copy link
Contributor

@floatdrop floatdrop left a comment

Choose a reason for hiding this comment

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

Problem of #257 is that we shadowing native timeout option and then copy it to gotTimeout here. Out option should be renamed to avoid this collision.

@floatdrop floatdrop dismissed their stale review May 18, 2017 08:09

Re-readed issue

index.js Outdated
false;

if (timeoutDuration) {
timeout = setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use p-timeout here instead?

await s.listen(s.port);
});

test('timeout option', async t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The rest of the tests tend to take this form:

	const err = await t.throws(got(`${s.url}/`, {
		timeout: 1,
		retries: 0
	}));
	t.is(err.code, 'ETIMEDOUT');

I'd be in favor of testing this way because:

  • it offloads worrying about if it throws to ava.
  • consistent style is helpful in reducing complexity.

@djmadeira
Copy link
Contributor Author

@floatdrop so should we just name the option gotTimeout?

@floatdrop
Copy link
Contributor

floatdrop commented May 18, 2017

@djmadeira nah, timeout is ok, I have fallen out of context a bit.

@djmadeira
Copy link
Contributor Author

@floatdrop @alextes addressed comments.

Copy link
Contributor

@floatdrop floatdrop left a comment

Choose a reason for hiding this comment

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

Does timeout option works with stream mode?

index.js Outdated
@@ -152,6 +162,7 @@ function asPromise(opts) {
throw new got.HTTPError(statusCode, res.headers, opts);
}

clearTimeout(timeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

Left over?

index.js Outdated

return (requestPromise => timeout ?
pTimeout(requestPromise, timeout, new got.RequestError({message: 'Request timed out', code: 'ETIMEDOUT'}, opts)) :
requestPromise
Copy link
Owner

Choose a reason for hiding this comment

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

This could be more readable. Can you move it (line 111 to 113) out into a separate variable?

readme.md Outdated

Option accepts `object` with separate `connect` and `socket` fields for connection and socket inactivity timeouts.
Option accepts `object` with separate `connect`, `socket`, and `request` fields for connection, socket, and entire request timeouts.
Copy link
Owner

@sindresorhus sindresorhus May 19, 2017

Choose a reason for hiding this comment

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

I know it's not from this change, but Option accepts object with separate => This also accepts an object with separate.

@djmadeira
Copy link
Contributor Author

@sindresorhus @floatdrop addressed comments.

Copy link
Owner

@sindresorhus sindresorhus left a comment

Choose a reason for hiding this comment

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

Looks great! 🌈

@sindresorhus
Copy link
Owner

@floatdrop @alextes :shipit: ?

@simoncpu
Copy link

Thank you for this contribution. I spent a few hour struggling to find out why I wasn't catching the timeout errors, so I resorted to reading the code. Lo and behold, a fix was merged on v7.0.0 so all I had to do was update my version of got. Thanks, man! =)

@Flackus
Copy link

Flackus commented Jul 11, 2017

-Milliseconds to wait for a server to send response headers before aborting request with `ETIMEDOUT` error.
--
+Milliseconds to wait for the server to end the response before aborting request with `ETIMEDOUT` error.

It should have been a major release probably :(
We now have (although already fixed) an issue with our module, which is designed for downloding big chunks of data, and it was handling "connection" timeout via got and the whole "download" timeout separately on it's side.
After 6.7.0 got's timeout: 2000 suddenly became the timeout for the whole request and everything went crazy.

@jstewmon jstewmon mentioned this pull request Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants