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

Set timeout for HTTP requests #7143

Merged
merged 2 commits into from Jun 21, 2016

Conversation

Projects
None yet
8 participants
@mitar
Collaborator

mitar commented May 31, 2016

While debugging #7090 I discovered that there is no timeout specified for many HTTP requests Meteor is making. This means that when packaging server (or CDN) is badly behaving, client is waiting indefinitely instead of throwing an error.

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 1, 2016

This might sounds too short for people in China, as in most cases it spends more than 30s. We should have a ENV to disable timeout.

@abernix

This comment has been minimized.

Member

abernix commented Jun 1, 2016

@laosb The TIMEOUT_SCALE_FACTOR env variable exists already and you can set it to a higher multiplier.

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 1, 2016

Didn't realize it. Was that env used in other places?

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 1, 2016

Also, timeout is not to download a file, but more or less timeout between TCP packets. I do not think that you have 15 seconds between packets even in China. :-)

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 1, 2016

Ah, my mistake, sorry!

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 1, 2016

I hoped that my comment in the code is conveying this. :-( Should I improve it?

@abernix

This comment has been minimized.

Member

abernix commented Jun 1, 2016

I thought it was fine – in general when talking about HTTP "timeout" this is what is implied – it's never the overall duration of the download, but instead the max time between blocks of data.

My only concern is if this is an appropriate use of timeoutScaleFactor – as that's only really used in selftest (usually in test.waitSecs) for increasing the delay when awaiting the next command to output to the log. I don't think it's currently purposed for networking stuff.

@abernix

This comment has been minimized.

Member

abernix commented Jun 1, 2016

I also think all future ENV variables should be prefixed with METEOR_, but that's a separate issue and not something you've caused with this PR. :)

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 1, 2016

And we should have a cheatsheet for ENVs.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 1, 2016

I do not know, I can remove it. I do not see really a reason for anyone to increase it to more than 15 seconds anyway.

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 1, 2016

We should not remove it, as nobody knows who will need it.

@abernix

This comment has been minimized.

Member

abernix commented Jun 1, 2016

I think this timeout is important. I might actually make it 60 seconds just to cover the worst cases. A 15 second transmission timeout can absolutely happen – especially on a wifi or mobile network when switching modes – but is still recoverable in most all cases. After a minute, you're very likely down though. I guess 60 seconds of waiting might be obnoxious in some environments though – maybe 30 is an okay compromise though?

I'd probably wait to see about using timeoutScaleFactor based on what core says.

Based on the networking problems I've seen so far (including CDN problems, etc.) I think a procedure for HTTP request retry strategies is the next thing that might need some attention.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 1, 2016

I think we should merge it with 15 seconds, and provide it with a release candidate. And then see what people give feedback on.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 1, 2016

But yes, it could also be longer. I just do not want cases like now where it just hangs indefinitely.

@abernix

This comment has been minimized.

Member

abernix commented Jun 1, 2016

Definitely don't want infinite hanging. My vote would be 60 – too short of a timeout and this could actually increase the number of package install problems by terminating them when they still might succeed if left to wait.

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 14, 2016

Shall we ask our friend Math.random?

> Math.round(Math.random() * 45 + 15)
35

In all seriousness, I'm subjectively comfortable with 30 seconds.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 14, 2016

And should I just remove the scaling and make it 30s and this is it?

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 14, 2016

Yeah let's make it 30 seconds, and keep the scaling factor unless there's a good reason to remove it.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 14, 2016

Others said that until now scaling factor is used only for testing.

@zol

This comment has been minimized.

Contributor

zol commented Jun 21, 2016

Lgtm, thanks @mitar

@zol zol merged commit 8f86d08 into meteor:devel Jun 21, 2016

3 checks passed

CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjamn benjamn referenced this pull request Jun 22, 2016

Merged

Release 1.3.4 #7263

6 of 6 tasks complete
@jamiter

This comment has been minimized.

Contributor

jamiter commented Jun 23, 2016

I'm getting the following error when I try to deploy to Galaxy with 1.3.4

Error deploying application: Connection error (ETIMEDOUT)

Could this be related or is Galaxy simply having issues?

Edit: Setting TIMEOUT_SCALE_FACTOR to 10 fixed it (just picked a number), or Galaxy is back online again.

@benjamn benjamn referenced this pull request Jun 23, 2016

Merged

Release 1.3.4.1 #7285

@jamiter

This comment has been minimized.

Contributor

jamiter commented Jun 23, 2016

For who is reading the above comment; It has been comfirmed that this caused the deploy issue. A fix will be released soon. See this thread for details:

https://forums.meteor.com/t/1-3-4-breaks-galaxy-deployment-etimedout/25383/7

@fdiazsmith

This comment has been minimized.

fdiazsmith commented Jun 24, 2016

Can some explain how to change the TIMEOUT_SCALE_FACTOR. Is this a variable stored somewhere, can it be done from the terminal?

@benjamn

This comment has been minimized.

Member

benjamn commented Jun 24, 2016

On Mac/Linux:

TIMEOUT_SCALE_FACTOR=10 meteor deploy ...

On Windows (PowerShell):

$env:TIMEOUT_SCALE_FACTOR = "10"
meteor deploy ...
@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 25, 2016

Unfortunately, even 60s got an exceed in China. Changing TIMEOUT_SCALE_FACTOR...

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 25, 2016

For downloading or uploading?

I think the solution is to restart. Not to extend timeout. With such long delays between TCP packets, TCP connection might not recover anyway.

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 25, 2016

Downloading. Retried several times.

@laosb

This comment has been minimized.

Collaborator

laosb commented Jun 25, 2016

An auto-retry-and-resume strategy would make Meteor usable in China, I think.

@mitar

This comment has been minimized.

Collaborator

mitar commented Jun 25, 2016

Care of trying to add this feature to request library?

@adamgins

This comment has been minimized.

adamgins commented Oct 5, 2016

I seems to be getting a similar issue with Meteor 1.3.5.1 I have tried TIMEOUT_SCALE_FACTOR = "10" but I cannot consistently deploy to Galaxy

ghybs added a commit to runisland/meteor-static-assets that referenced this pull request Nov 28, 2016

Bump patch version to 0.1.4
due to successive tries to publish the package onto Atmosphere.

Turns out there was a simple timeout problem, fixed by increasing that timeout with command `TIMEOUT_SCALE_FACTOR=10 meteor publish`. See meteor/meteor#7143 and https://forums.meteor.com/t/1-3-4-breaks-galaxy-deployment-etimedout/25383/17
But in the first place, it is VERY strange that Meteor needs to build and upload a few MB for such a simple package… mys:fonts package does not need to download such a big payload when installed on a project. To be investigated later on. Might be a change in Meteor behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment