Upgrade the files branch (#323) to work with pull-streams #469

dignifiedquire · 2016-09-09T18:25:10Z

Some tests are still breaking, need to figure out why.

Needs:

feat(exporter): return file sizes ipfs-inactive/js-ipfs-unixfs-engine#71

TODO:

rebased add onto master
started using pull-streams in the file operations
made files get actually stream the file content, using streams + size args
fix tests:
- http: files .cat streams a large file
- http: files .get a large file
- http: files .get directory

dignifiedquire · 2016-09-09T21:07:17Z

Quick update: I managed to find the issue, it is a double read of the first chunk of the file, which happens due to a race condition inside pull-file. I am not certain how to best fix it though.

victorb · 2016-09-09T21:07:49Z

src/http-api/resources/files.js

+      )
+    })
+
+    function handleError (err) {


Might not be a great idea to create this function every time a request happens. And in general, handleError should probably be somewhere global for the lib instead of specific to get.

dignifiedquire · 2016-09-10T11:10:36Z

Fix for pull-file is up: pull-stream/pull-file#4

daviddias · 2016-09-10T12:33:44Z

@dignifiedquire awesome! Shall we use the fork meanwhile?

dignifiedquire · 2016-09-10T12:41:30Z

@diasdavid still working out some issues :/

daviddias · 2016-09-11T06:47:13Z

@jbenet fixed all the things :D (except pull-file, that still requires the PR to be merged)

dignifiedquire · 2016-09-12T09:28:54Z

Whoo a wild @jbenet appears and fixes my code 😹

The other issue I fixed in ipfs/js-ipfs-repo#85

daviddias · 2016-09-12T13:29:44Z

@dignifiedquire could you describe how we went from: 'pull-file has a race condition that emits the same chunk twice' to 'putting a lock that buffers the whole block is a fix'?

daviddias · 2016-09-12T14:11:40Z

src/core/ipfs/files.js

@@ -53,6 +53,7 @@ module.exports = function files (self) {
      pull(
        pull.values([hash]),
        pull.asyncMap(self._dagS.get.bind(self._dagS)),
+        pull.take(1),


Can't these be replaced just with one call to dagService to get the DAGNode?

it could, but I like it this way :P

Why would we have something in 3 lines when we can just use one? It adds unnecessary complexity, memory overhead and superfluous ops.

no it doesn't add memory overhead, because the implementation of .get in the dagService is literally the same thing

sorry, nvm what I'm doing here is stupid -.-

This is absurd. It's serious fancy abstraction creep. Careful

On Mon, Sep 12, 2016 at 10:53 AM Friedel Ziegelmayer <
notifications@github.com> wrote:

In src/core/ipfs/files.js
#469 (comment):

@@ -53,6 +53,7 @@ module.exports = function files (self) {
pull(
pull.values([hash]),
pull.asyncMap(self._dagS.get.bind(self._dagS)),

pull.take(1),

sorry, nvm what I'm doing here is stupid -.-

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ipfs/js-ipfs/pull/469/files/06ee44fb061954f440d6c80df94609400dc8f12e#r78388308,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAIcoW2OqPNNILjFBhda_sNfyhs_gxARks5qpWd6gaJpZM4J5Uvl
.

dignifiedquire · 2016-09-12T14:17:48Z

@dignifiedquire could you describe how we went from: 'pull-file has a race condition that emits the same chunk twice' to 'putting a lock that buffers the whole block is a fix'?

I filed a PR on pull-file as mentioned above, where @domenictarr was so nice to point out that calling the file stream twice without waiting for the previous cb to finish was a bug in the sink stream, not in pull-file, as pull-streams should always wait for the previous one to be finished. So I started to dig in and found that the reason why the source stream was called twice, was that we were not properly locking gets to the same keys in the blockstore (we were doing this before we transitioned to pull-streams, I simply forgot to put it back when doing the refactor).

daviddias · 2016-09-12T14:53:09Z

src/http-api/resources/files.js

+    }
+
+    const ipfs = request.server.app.ipfs
+    // TODO: make pull-multipart


Track this in a issue. @xicombd you probably would like to get that working, since you did the first multipart parser :)

daviddias · 2016-09-12T14:58:23Z

@dignifiedquire interesting, so reading the same file twice causes an issue? That sounds weird.

Also, CR'ed this PR

dignifiedquire · 2016-09-12T15:05:39Z

@dignifiedquire interesting, so reading the same file twice causes an issue? That sounds weird.

No reusing the same stream to read the file can cause issues

https://greenkeeper.io/

Add tests for making sure .cat shows right output and fix that test by using the right argument from cli. Ref: issue #476

using ipfs-api

dignifiedquire · 2016-09-12T17:09:33Z

@diasdavid lets squash this into a single commit when merging please.

daviddias · 2016-09-12T17:11:50Z

@dignifiedquire I've squashed and reworded, but not all into a single commit, because that loses the contributions from the different participants.

That being said, thank you everyone @nginnever @noffle @victorbjelkholm @dignifiedquire @jbenet that took part in this PR, js-ipfs is at least 40% more awesome and robust just for having all of these tests passing :D thank you! 👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏👏🎉👏

victorb · 2016-09-12T17:23:28Z

jbenet · 2016-09-12T18:55:03Z

History is important too. Squash responsibly
On Mon, Sep 12, 2016 at 1:23 PM ᴠɪᴄᴛᴏʀ ʙᴊᴇʟᴋʜᴏʟᴍ notifications@github.com
wrote:

!()[http://i.giphy.com/l46CvRFB1GqPYAOis.gif]

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#469 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIcoeTW4S9j7j0h3ulqKNqoahecuuqJks5qpYqRgaJpZM4J5Uvl
.

dignifiedquire added the status/in-progress In progress label Sep 9, 2016

This was referenced Sep 9, 2016

feat(exporter): return file sizes ipfs-inactive/js-ipfs-unixfs-engine#71

Merged

.add/files.add http-api endpoint #323

Closed

daviddias changed the title ~~Make add branch work with pull streams~~ Upgrade the files branch (#323) to work with pull-streams Sep 9, 2016

daviddias added the milestone 2 label Sep 9, 2016

victorb reviewed Sep 9, 2016
View reviewed changes

victorb mentioned this pull request Sep 9, 2016

Use interface-ipfs-core tests for files API over http #438

Closed

2 tasks

jbenet force-pushed the pull-add branch 2 times, most recently from aca631a to 72d35c0 Compare September 11, 2016 06:35

dignifiedquire mentioned this pull request Sep 12, 2016

fix(blockstore: lock getStream to avoid race issues ipfs/js-ipfs-repo#85

Merged

dignifiedquire force-pushed the pull-add branch from 72d35c0 to 06ee44f Compare September 12, 2016 09:52

daviddias reviewed Sep 12, 2016
View reviewed changes

dignifiedquire mentioned this pull request Sep 12, 2016

Create pull-multipart module #479

Closed

chore(package): update ipfs-unixfs-engine to version 0.11.3

76959ac

https://greenkeeper.io/

victorb and others added 2 commits September 12, 2016 13:02

fix(cli): use right argument for cli .cat

2bf49ea

Add tests for making sure .cat shows right output and fix that test by using the right argument from cli. Ref: issue #476

feat(add): add the http endpoint for files.add

e29f429

daviddias force-pushed the pull-add branch from 641554f to e5ad85f Compare September 12, 2016 17:05

victorb and others added 5 commits September 12, 2016 13:08

feat(files): get interface-ipfs-core files tests pass through http-api

11cb4ca

using ipfs-api

feat(files): interface-ipfs-core tests over ipfs-api

001a6eb

feat(tests): waste less time generating keys

cb10ab7

fix(http): get handler reads the stream

b0a6db9

chore: cleanup, apply CR

4050195

daviddias force-pushed the pull-add branch from e5ad85f to 4050195 Compare September 12, 2016 17:08

daviddias merged commit e0b69a8 into master Sep 12, 2016

daviddias deleted the pull-add branch September 12, 2016 19:43

daviddias removed status/in-progress In progress milestone 2 labels Sep 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade the files branch (#323) to work with pull-streams #469

Upgrade the files branch (#323) to work with pull-streams #469

dignifiedquire commented Sep 9, 2016 •

edited

Loading

dignifiedquire commented Sep 9, 2016

victorb Sep 9, 2016

dignifiedquire commented Sep 10, 2016

daviddias commented Sep 10, 2016

dignifiedquire commented Sep 10, 2016

daviddias commented Sep 11, 2016

dignifiedquire commented Sep 12, 2016

daviddias commented Sep 12, 2016

daviddias Sep 12, 2016

dignifiedquire Sep 12, 2016

daviddias Sep 12, 2016

dignifiedquire Sep 12, 2016

dignifiedquire Sep 12, 2016

jbenet Sep 12, 2016

dignifiedquire commented Sep 12, 2016

daviddias Sep 12, 2016

dignifiedquire Sep 12, 2016

daviddias commented Sep 12, 2016

dignifiedquire commented Sep 12, 2016

dignifiedquire commented Sep 12, 2016

daviddias commented Sep 12, 2016

victorb commented Sep 12, 2016 •

edited by daviddias

Loading

jbenet commented Sep 12, 2016

Upgrade the files branch (#323) to work with pull-streams #469

Upgrade the files branch (#323) to work with pull-streams #469

Conversation

dignifiedquire commented Sep 9, 2016 • edited Loading

dignifiedquire commented Sep 9, 2016

Choose a reason for hiding this comment

dignifiedquire commented Sep 10, 2016

daviddias commented Sep 10, 2016

dignifiedquire commented Sep 10, 2016

daviddias commented Sep 11, 2016

dignifiedquire commented Sep 12, 2016

daviddias commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Sep 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Sep 12, 2016

dignifiedquire commented Sep 12, 2016

dignifiedquire commented Sep 12, 2016

daviddias commented Sep 12, 2016

victorb commented Sep 12, 2016 • edited by daviddias Loading

jbenet commented Sep 12, 2016

dignifiedquire commented Sep 9, 2016 •

edited

Loading

victorb commented Sep 12, 2016 •

edited by daviddias

Loading