-
Notifications
You must be signed in to change notification settings - Fork 167
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
nginx: Enable resumable downloads for release files #36
Conversation
@silverwind this is now active on the server, can you confirm for me that it's working before I merge to master? thanks so much for this |
@rvagg Whatever you did in the server configuration, I recommend reverting. All tarballs now 404, both for 1.0.1 and 1.0.0. Edit: you forgot to add root inside the location {} scope. |
reverted |
|
||
location ~* (\.xz|\.gz|\.pkg|\.msi)$ { | ||
gzip off; | ||
} |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Sorry, haven't been around to test. It worked on my local nginx, but that one has a slightly different |
one guess is that I actually manually put this at the top of the |
Yeah, it shouldn't matter. What nginx version are you running, by the way? |
nginx/1.4.6 (Ubuntu) |
tried putting it last and get 404s still |
Anything in |
hard to tell with all the noise but this could have been during a turned on period:
so it's looking in /usr/share/nginx/html/ for dist, so it's overriding the proper |
You're right, I see the problem. The
|
As mentioned on IRC and slightly above, either add root within the location scope or move root outside of the other location scope to inherit it. I started doing a new branch with some other improvements (namely removing the www-check since it's executed on every request) - but that's for another day. |
Another possibilty would be to nest the regex location, but moving root outside and into the |
I'm open to changes here, my background is deeply with Apache and it's only the last 6 months or so that I've been really digging more deeply into nginx but I still consider myself a n00b with it |
Updated the PR. I moved the I'm available for some tests in the next hour, but almost certain that it'll work as expected now. |
LGTM. Lets add other improvements in other PR's -- but it's really heading into premature optimisation-ville :-) |
@ljharb could you please retest download resumption? While this change isn't live, I think this may actually have never been an issue in first place. |
If you do that default type I don't think you'll be able to download the files normally. |
@jbergstroem No, I meant the original issue of non-working resumption. Here's the correct
And here's a gzip response, supressing
I believe that nodejs/node#424 actually never was an issue. |
@Fishrock123 Already refactored that. It's just an fallback mime, and it's at its default value, so shouldn't have been an issue. |
@silverwind I still get |
I don't see an issue, the second curl finishes instantly. What's your
Edit: sorry, ran against nodejs first, but same result on iojs. |
|
And what do you get on this one?
|
Looks like exactly the same:
|
I'm using the exact same curl version here on OS X and don't get your error, so I'm really baffled. Could there be an proxy interfering on your side? Maybe your curl is aliased? Still wouldn't explain why nodejs.org works. |
Added some -vvv and I finally saw your 'error': On OS X (curl 7.37.1):
On Linux (curl 7.39.0):
As you see, it requests bytes beyond the Finally, I CTRL-C mid download, restarted, and it resumed fine on both Linux and OS X, so if there's an issue here, then it's with OS X's outdated curl. |
Thanks - so you're saying the |
@ljharb both servers respond correctly, it's just curl getting confused by the missing
|
Interesting - I'm not using Could |
Both support Go ahead and cancel a download at 50%, and you will see it work. Really nothing broken here. |
Cool, thanks for the exhaustive explanation. I'll just ignore the "error" output in nvm.sh |
Regarding the |
Disabling gzip for the release downloads enables support for the HTTP ranges mechanism, as seen by the advertized header on matching requests:
For further explaination on why nginx disables ranges on gzip, see here.