/meteor

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

Package download request resuming #7399

Merged
merged 6 commits into from Jul 14, 2016

Conversation

Reviewers
No reviews
Assignees

@benjamn benjamn

@tmeasday tmeasday

Labels
None yet
Projects
None yet
Milestone
  Release 1.4
3 participants
@tmeasday @benjamn @abernix
@tmeasday
Contributor

tmeasday commented Jul 13, 2016 •
edited

For #7267

I wanted to get your thoughts on this approach @benjamn.

Todo:

  • Ensure it actually works with package downloading
  • Figure out if there's a way to make the test meaningful
  • Add a timeout to the retries
  • Better logging
  • Add stream-buffers to the dev bundle
@tmeasday
First pass at request resuming via buffering the `range` header 
This is still a WIP, but I wanted to get feedback at this point.
Will update the PR with outstanding TODOs.
Loading status checks…
756fb59

@tmeasday tmeasday assigned tmeasday and benjamn Jul 13, 2016

tools/utils/http-helpers.js Outdated
return httpHelpers.request({
outputStream,
headers,
...urlOrOptions,

This comment has been minimized.

Sign in to view
Copy link
@tmeasday

tmeasday Jul 13, 2016

Contributor

XXX: should be options

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016

Member

If you use options here, the options.headers property will override the headers property, so you might as well update options.headers above, and remove the headers, line here.

This comment has been minimized.

Sign in to view
Copy link
@tmeasday

tmeasday Jul 14, 2016

Contributor

👍
tools/utils/http-helpers.js Outdated
function attempt(triesRemaining) {
const headers = _.extend({}, urlOrOptions.headers, {
range: `bytes=${outputStream.size()}-`,
});

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016 •
edited

Member 
options.headers = {
  ...options.headers,
  range: `bytes=${outputStream.size()}-`
};

This comment has been minimized.

Sign in to view
Copy link
@tmeasday

tmeasday Jul 14, 2016

Contributor

👍
tools/utils/http-helpers.js Outdated
console.log(e);
if (triesRemaining > 0) {
console.log(`Request failed, retrying`);
return attempt(triesRemaining - 1);

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016

Member

What if the failure only counted against triesRemaining if the download made no progress (or less than some small amount of progress)?

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016

Member

Perhaps there should be a delay of a few seconds between attempts?

This comment has been minimized.

Sign in to view
Copy link
@tmeasday

tmeasday Jul 14, 2016

Contributor

That's part of the plan, yeah.
tools/tests/utils-tests.js Outdated
const url = 'http://warehouse.meteor.com/builds/Pr7L8f6PqXyqNJJn4/1443478653127/aRiirNrp4v/meteor-tool-1.1.9-os.osx.x86_64+web.browser+web.cordova.tgz';
const result = require('../utils/http-helpers').getUrlWithResuming({
timeout: 1000,

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016

Member

This should actually test resuming, perhaps by supporting a custom options.outputStream object, and passing a test object whose .write method throws an HTTP error after the first few chunks are written?

This comment has been minimized.

Sign in to view
Copy link
@tmeasday

tmeasday Jul 14, 2016

Contributor

Oooh, that might work, let me try it.
tools/utils/http-helpers.js
@@ -14,6 +14,7 @@ var release = require('../packaging/release.js');
var Console = require('../console/console.js').Console;
var timeoutScaleFactor = require('./utils.js').timeoutScaleFactor;
import { WritableStreamBuffer } from 'stream-buffers';

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016

Member

The stream-buffers package needs to be in dev_bundle/lib/node_modules.

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016

Member

Ah, sorry, I see that in your to-do list now.

This comment has been minimized.

Sign in to view
Copy link
@tmeasday

tmeasday Jul 14, 2016

Contributor

Yeah I just wanted to check with you that the concept of using a stream buffer to collect the body made sense before bothering to build a new bundle etc.
tools/utils/http-helpers.js Outdated
const href = response.request.href;
throw Error(
body ||
`Could not get ${href}; server returned [${response.statusCode}]`);

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016

Member

Maybe this was just for debugging, but I would use the same error message every time, regardless of whether body is defined, and then just return outputStream.getContents() below, if there was no error.

This comment has been minimized.

Sign in to view
Copy link
@tmeasday

tmeasday Jul 14, 2016

Contributor

Yeah, I was just re-implementing what getUrl does above, but I agree it's sort of weird.
@benjamn

This comment has been minimized.

Sign in to view
Member

benjamn commented Jul 13, 2016 •
edited

I like the approach of storing partial downloads in memory!
tools/utils/http-helpers.js Outdated
const MAX_ATTEMPTS = 5;
function attempt(triesRemaining) {
const headers = _.extend({}, urlOrOptions.headers, {
range: `bytes=${outputStream.size()}-`,

This comment has been minimized.

Sign in to view
Copy link
@benjamn

benjamn Jul 13, 2016

Member

Should range be capitalized here?

This comment has been minimized.

Sign in to view
Copy link
@tmeasday

tmeasday Jul 14, 2016

Contributor

👍

tmeasday added some commits Jul 14, 2016

@tmeasday
Made the test work in a sensible way
Loading status checks…
eeba73e
@tmeasday
Better logging and progress 
Using Console.debug to register messages about retries.
Loading status checks…
1ef12c7
@tmeasday
Added stream buffers to the dev bundle
Loading status checks…
c5cc1c7
@tmeasday
Bumped dev bundle version
Loading status checks…
14210b2
@tmeasday
Minor fixes + added a comment
Loading status checks…
6fe6875
@tmeasday

This comment has been minimized.

Sign in to view
Contributor

tmeasday commented Jul 14, 2016

@ben I think this should be good to go, awaiting tests

@benjamn benjamn merged commit 09ecd80 into release-1.4 Jul 14, 2016
4 checks passed

4 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
continuous-integration/travis-ci/push The Travis CI build passed
Details

benjamn added a commit that referenced this pull request Jul 14, 2016

@benjamn
Avoid exposing httpHelpers._currentRequest. 
Follow-up to #7399.
82dae55

benjamn added a commit that referenced this pull request Jul 14, 2016

@benjamn
Avoid fiberHelpers.bindEnvironment by returning Promise from attempt. 
Follow-up to #7399.
Loading status checks…
aeca680

@benjamn benjamn added this to the Release 1.4 milestone Jul 14, 2016

@benjamn benjamn referenced this pull request Jul 14, 2016

Closed

7267 use request retry #7349

benjamn added a commit that referenced this pull request Jul 14, 2016

@benjamn
Avoid exposing httpHelpers._currentRequest. 
Follow-up to #7399.
8bbe495

benjamn added a commit that referenced this pull request Jul 14, 2016

@benjamn
Avoid fiberHelpers.bindEnvironment by returning Promise from attempt. 
Follow-up to #7399.
89c91c6

benjamn added a commit that referenced this pull request Jul 14, 2016

@benjamn
Avoid exposing httpHelpers._currentRequest. 
Follow-up to #7399.
fbfb5b8

benjamn added a commit that referenced this pull request Jul 14, 2016

@benjamn
Avoid fiberHelpers.bindEnvironment by returning Promise from attempt. 
Follow-up to #7399.
0c43039

@benjamn benjamn referenced this pull request Jul 14, 2016

Merged

Release 1.3.5 #7407

benjamn added a commit that referenced this pull request Jul 14, 2016

@benjamn
Make download resumption test always run (and pass). 
This is a test that really needs to run and pass every time we run the
test suite, so I decided it shouldn't be --slow. If it takes too long, we
can always download a smaller test file.

Hard-coding the download length was a recipe for brittleness, so now I'm
downloading the file without interruptions in parallel with the
interrupted/resumed download, so that we can compare the two files when
both have finished downloading.

Follow-up to #7399.
91f2039
tools/packaging/tropohouse.js
@@ -304,7 +304,7 @@ _.extend(exports.Tropohouse.prototype, {
// it relies on extractTarGz being fast and not reporting any progress.

This comment has been minimized.

Sign in to view
Copy link
@abernix

abernix Jul 15, 2016

Member

It's just off the top of the diff here, but I think this XXX/TODO would be a huge benefit as it's getting hard to debug when something is stuck downloading or something is stuck extracting.

I'm getting the feeling that people are getting stuck after the actual download, especially on Windows.

@abernix abernix referenced this pull request Jul 15, 2016

Closed

Meteor not starting hang on meteor-tools after downloaded (OSX) #7283

@abernix abernix referenced this pull request Nov 4, 2016

Closed

Geographical CDN issue causing meteor & package install problem #6960

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment