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

Adding a configurable timeout for each uplink #18

Merged
merged 1 commit into from
Dec 5, 2013

Conversation

iambrandonn
Copy link
Contributor

I added a timeout config value for the uplinks. Without this timeout I've found that the fallback to local cached copies of packages is much too slow, especially for packages with many dependencies (where the timeout applies to each dependency).

@rlidwka
Copy link
Owner

rlidwka commented Dec 5, 2013

Without this timeout I've found that the fallback to local cached copies of packages is much too slow, especially for packages with many dependencies

It's not actually a good way to solve it. If you got a timeout, it probably means that uplink is down, and it won't make sense to ask it again for a couple of minutes. I planned to do this, but not quite have time yet.

Also, it's a bad default, because for a people who has slow internet connection, perfectly legitimate request will hit timeout.

But if user needs to specify a timeout, he (oh my god, Joyent is gonna fire me) should be able to do that. So, I'm merging it, but I'll probably change a default later, so please set it in config if it matters to you.

rlidwka added a commit that referenced this pull request Dec 5, 2013
Adding a configurable timeout for each uplink
@rlidwka rlidwka merged commit ff52b00 into rlidwka:master Dec 5, 2013
@rlidwka
Copy link
Owner

rlidwka commented Dec 6, 2013

You suggest a default of 5 seconds, but it's too low for everybody. Default request.js timeout is 120 sec, which is in fact too much.

What do you think, provided that sinopia would try to connect just once (not once per package), what a good default timeout would be?

@iambrandonn
Copy link
Contributor Author

If sinopia only tries to connect once, and then knows to fall back for subsequent requests... I think a much higher default would be fine. In my testing however I found that it was checking for every package I tried to install.
Anyway... maybe 20 or 25 seconds?

@rlidwka
Copy link
Owner

rlidwka commented Dec 8, 2013

In my testing however I found that it was checking for every package I tried to install.

Yeah, this feature is on the roadmap, but not quite there yet.

rlidwka added a commit that referenced this pull request Dec 8, 2013
@rlidwka
Copy link
Owner

rlidwka commented Dec 11, 2013

In my testing however I found that it was checking for every package I tried to install.

Now it doesn't. Fixed by commit 96b336a, but not yet published in npm.

From now on when there's an error, sinopia don't send any more requests to that uplink for 2 minutes.

Not really sure it's a right behaviour, since npmjs registry can fail some of the requests, but answer to others. Maybe mark it as "down" if there are 2 consecutive failures, but it'll be a minute of waiting in this case.

dididada123 pushed a commit to dididada123/sinopia that referenced this pull request Aug 8, 2016
…-plugin

makes middlewares pluggable. i.e for custom oauth flows or analytics etc.
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.

2 participants