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

send files HTTP request should stream #629

Merged
merged 5 commits into from Nov 20, 2017

Conversation

Projects
None yet
2 participants
@pgte
Copy link

pgte commented Nov 17, 2017

The HTTP multipart implementation provides a streaming interface but doesn't allow adding files mid-flight.
This PR does that and bases all the APIs that need to send files on that.
Should fix #628 (and, by association, ipfs/js-ipfs#823).

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 17, 2017

Codecov Report

Merging #629 into master will decrease coverage by 0.01%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #629      +/-   ##
==========================================
- Coverage   83.86%   83.85%   -0.02%     
==========================================
  Files         112      108       -4     
  Lines        1624     1604      -20     
==========================================
- Hits         1362     1345      -17     
+ Misses        262      259       -3
Impacted Files Coverage Δ
src/files/create-add-stream.js 100% <100%> (ø)
src/index.js 88.88% <100%> (ø) ⬆️
src/utils/converter.js 71.42% <100%> (-20.24%) ⬇️
src/utils/prepare-file.js 92.3% <100%> (ø)
src/object/appendData.js 85.71% <100%> (+3.36%) ⬆️
src/util/url-add.js 82.75% <100%> (-1.62%) ⬇️
src/util/fs-add.js 77.77% <100%> (-1.17%) ⬇️
src/object/setData.js 38.09% <57.14%> (+8.68%) ⬆️
src/utils/module-config.js 60% <66.66%> (ø) ⬆️
src/utils/send-one-file.js 80% <80%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4b8ed1...7403afc. Read the comment docs.

@wafflebot wafflebot bot assigned pgte Nov 17, 2017

@wafflebot wafflebot bot added the in progress label Nov 17, 2017

@pgte pgte changed the title WIP: send files HTTP request streams send files HTTP request should stream Nov 18, 2017

@pgte pgte requested a review from daviddias Nov 18, 2017

@daviddias
Copy link
Member

daviddias left a comment

Please rebase master onto this branch, otherwise LGTM :)

// This does not work
// this.once('drain', () => {
// callback()
// })

This comment has been minimized.

@daviddias

daviddias Nov 18, 2017

Member

No needed anymore?

This comment has been minimized.

@pgte

pgte Nov 18, 2017

We do back-pressure per file item, but once inside a file we don't. So it's optimised for a lot of files, but not for large files. If that comes to be a problem, I think we should open a different issue to tackle it..

This comment has been minimized.

@daviddias

daviddias Nov 19, 2017

Member

Mind writing this note as the comment as well?

This comment has been minimized.

@pgte

pgte Nov 20, 2017

Decided to fix individual file back-pressure in this PR: 782beb9

This comment has been minimized.

@pgte

pgte Nov 20, 2017

Had to limit it to Node only though, because a browser HTTP request does not stream..
7403afc

callback(null, ds)
})
}
module.exports = (send) => SendFilesStream(send, 'add')

This comment has been minimized.

@daviddias

daviddias Nov 18, 2017

Member

This file doesn't exist anymore, you need to rebase master onto this branch

qs.hash = opts.hash
} else if (opts.hashAlg != null) {
qs.hash = opts.hashAlg
}

This comment has been minimized.

@daviddias

daviddias Nov 18, 2017

Member

Where did all of this options went?


send.andTransform(request, (response, cb) => {
converter(ProgressStream.fromStream(opts.progress, response), cb)

This comment has been minimized.

@daviddias

daviddias Nov 18, 2017

Member

Is this still doing Progress reports?

@@ -6,7 +6,6 @@ const isNode = require('detect-node')
const ndjson = require('ndjson')

This comment has been minimized.

@daviddias

daviddias Nov 18, 2017

Member

While you are at it, wanna rename this file to send-request?

This comment has been minimized.

@pgte

@pgte pgte force-pushed the fix/add-stream branch from 6f25c3f to 6583fcf Nov 20, 2017

@pgte

This comment has been minimized.

Copy link

pgte commented Nov 20, 2017

@diasdavid rebased.

@daviddias daviddias merged commit dae62cb into master Nov 20, 2017

3 of 5 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
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

@daviddias daviddias deleted the fix/add-stream branch Nov 20, 2017

@wafflebot wafflebot bot removed the in progress label Nov 20, 2017

@daviddias

This comment has been minimized.

Copy link
Member

daviddias commented Nov 20, 2017

This is fantastic @pgte !

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