refactor(streams): Refactor response stream handling. #465

haadcode · 2016-12-05T10:17:51Z

This PR will:

Refactor the response stream handling.
Add dagnode-stream to transform file results to DAGNode objects.
Add stream-to-value to convert a response stream to a single value.
Refactor request-api so that chunked JSON objects are not buffered anymore. This touches a bunch of the files pushing the transform function down to the individual commands.

The general idea in this PR is to cleanup the architecture and implementation regards to how the http responses are handled. Previously some of it was done in request-api.js and some in the individual commands making request-api coupled with the commands' response type. I've moved the response transform logic down to the individual commands so that request-api is agnostic to what is returned to the consumer of the commands. I've also refactored individual types of transformations to their own files to make it clear, clean and much more reusable (we can consider moving them to their own modules, but not in this PR).

dignifiedquire

While we are at it, could you please use pump instead of .pipe for combining streams, as that will handle errors and cleanup streams on failure. For more details see https://github.com/mafintosh/pump

dignifiedquire · 2016-12-05T10:24:44Z

src/dagnode-stream.js

+    this._send = opts.send
+  }
+
+  static streamToValue (send, inputStream, callback) {


is static available in Node4?

Is it? Is it not? And if so, why is that important? Provide context and solutions or guidance.

Looks good: https://kangax.github.io/compat-table/es6/#test-class_static_methods

Always usable is

class MyClass {} MyClass.mystaticmethod = function () { }

daviddias

Some modifications are not clear to me

daviddias · 2016-12-06T18:20:18Z

src/api/add.js

               isStream.isReadable(files) ||
               Array.isArray(files)

-    if (!good) {
-      callback(new Error('"files" must be a buffer, readable stream, or array of objects'))
+    if (!ok) {


unnecessary change

Don't you think ok is more descriptive and intuitive than good?

daviddias · 2016-12-06T18:20:27Z

src/api/add.js

-    if (!good) {
-      callback(new Error('"files" must be a buffer, readable stream, or array of objects'))
+    if (!ok) {
+      return callback(new Error('"files" must be a buffer, readable stream, or array of objects'))


daviddias · 2016-12-06T18:23:21Z

src/tar-stream-to-objects.js

+    callback(null, outputStream)
+  }
+
+  _read () {}


A Class with a constructor and a static method? What is the goal here?

daviddias · 2016-12-06T18:28:46Z

src/dagnode-stream.js

+  Output object format:
+  {
+    path: '/path/to/file/foo.txt',
+    hash: 'Qma4hjFTnCasJ8PVp3mZbZK5g2vGDT4LByLJ7m8ciyRFZP',


hash or multihash?

This "Output object format" is not a DAGNode Transforms a stream of objects to DAGNodes and outputs them as objects., what am I missing?

Changed the description. Does that make sense now?

Here: 1566751

@diasdavid I'm not sure about the multihash vs. hash. It's a breaking change again and I'd be hesitant to change it in this PR. Have we made the change elsewhere in the code already? Does anything that returns DAGNode-like information return .multihash instead of .hash? If we have, I can change it so that we're consistent, but if not, I would leave it for later.

This https://github.com/ipld/js-ipld-dag-pb/blob/master/src/dag-node/index.js#L22-L27 has been there for a while now, that is why I proposed it to be multihash

daviddias · 2016-12-06T18:29:58Z

src/request-api.js

    if (typeof options !== 'object') {
      return callback(new Error('no options were passed'))
    }

    return requestAPI(config, options, callback)
  }

-  // Wraps the 'send' function such that an asynchronous
-  // transform may be applied to its result before
-  // passing it on to either its callback or promise.


can you preserve the comment please?

This comment doesn't make sense, and it doesn't hold true for the new implementation.

It was trying to convey that before the response is returned, either through the callback or promise, we transform the response from go-ipfs to match interface-ipfs-core definition.

Added the description back and trying to describe as simple as I can what it does. What do you think?

Here: 1566751

daviddias · 2016-12-06T18:31:33Z

src/stream-to-value.js

+  res.pipe(concat((data) => callback(null, data)))
+}
+
+module.exports = streamToValue


In case of error on the stream (connection dropped) this fails silently

Is this why you had all the once('error') calls there previously?

Not knowing what is all the, but yes, if you don't capture the error, you don't know it failed

daviddias · 2016-12-06T18:32:31Z

src/stringlist-to-array.js

+  cb(null, res.Strings || [])
+}
+
+module.exports = stringlistToArray


What is a string list? Is there a better name in this context? Does it has to be a separate file to do this?

stringList is a Go type (that's what it's called in Go) that the go-ipfs API sends back for some requests. It adds an additional "Strings" field to the response that contains a list of strings, so "{ Strings: [...] }" instead of "[...]".

The correct solution here would be to fix it on go-ipfs side to return an array instead of what it does now, and this is a workaround for that: a type to another type conversion function to isolate it and make it clear that another type gets converted to another.

@whyrusleeping wanna comment?

We shouldn't change this on go-ipfs side for this PR. If we do that, it's an API breaking change that we need to handle with proper care. That's why I put it in one file, so that it's wrapped and isolated and if/once we change the return values, we only need to change change it in this file and whichever command is using the current API doesn't need changes. Obviously once you do that, we don't need the stringlist-to-array.js conversion function and can get rid of the file completely, but I recommend we keep it as such now so that we isolate the data conversion to one place. Does that make sense?

Understood :)

haadcode · 2016-12-07T07:03:07Z

Thanks for the review @dignifiedquire and @diasdavid. Will work on this soon, mainly change it to use pump as well as address other CR pieces.

daviddias · 2016-12-18T09:33:47Z

src/dagnode-stream.js

+        path: obj.Name,
+        hash: obj.Hash,
+        size: node.size
+      }


Not really a dag, it is just a json representation.

daviddias · 2016-12-18T09:36:04Z

src/tar-stream-to-objects.js

+    let outputStream = new TarStreamToObjects()
+
+    inputStream
+      .pipe(tar.extract())


better to use pump

haadcode · 2016-12-19T08:07:02Z

Will update this today and address the CR feedback.

haadcode · 2016-12-19T11:42:37Z

I've refactored the pipes to use pump. I wasn't sure if the pump is needed in dagnode-stream.js since streamToValue internally uses pump (so I take it would handle the errors), but I added it just in case. The tests are not running in the browser atm, but run fully on node (will look into why the browser tests are not running).

I also reworked the comments for the two functions @diasdavid had a confusion about, hope they're now more descriptive. I refactored the TarStreamToObjects according to @diasdavid's question, perhaps now the intent is clearer. I couldn't get the ReadableStream to work when created directly with new Readable() and adding _read() = () => {}` function to it like it's normally done, but by wrapping it in a class it works. If you have an idea how to make it work without the class, please do refactor it.

There's also a problem with browser version now as we're using pump because its dependency on fs. @dignifiedquire suggested we fix it there: mafintosh/pump#6. If you know how, please do.

So while this is still WIP, can you take a look and see if there's anything else to address in this one?

dignifiedquire · 2016-12-19T12:16:12Z

@haadcode pump fix is here: mafintosh/pump#13 and the issue with cors was correct, you changed them, now they are back: b2c09b2

haadcode · 2016-12-20T14:04:40Z

Should we wait for the PR to get merged or can we use the PR commit as our dep in the meanwhile?

the issue with cors was correct, you changed them, now they are back: b2c09b2

Interesting. I had to change it in the first place because it wasn't working. Good that it's fixed now 👍

dignifiedquire · 2016-12-20T14:14:07Z

Should we wait for the PR to get merged or can we use the PR commit as our dep in the meanwhile?

Pinged @mafintosh on irc, if it's not merged when we want to ship this lets switch to the fork.

dignifiedquire · 2016-12-20T14:19:10Z

package.json

@@ -43,6 +42,7 @@
    "peer-id": "^0.8.1",
    "peer-info": "^0.8.1",
    "promisify-es6": "^1.0.2",
+    "pump": "^1.0.1",


lets bump this to 1.0.2 so we are sure to have the fix in

dignifiedquire · 2016-12-20T14:22:41Z

src/dagnode-stream.js

+
+  static streamToValue (send, inputStream, callback) {
+    const outputStream = pump(inputStream, new DAGNodeStream({ send: send }), (err) => {
+      if (err) callback(err)


@diasdavid will want this to be

if (err) { callback(err) }

daviddias

Other than the hash vs multihash thing, it LGTM. I'm fine if Files API uses hash, although I would prefer consistency.

Add dagnode-stream to transform file results to DAGNode objects. Add stream-to-value to convert a response stream to a single value. Refactor request-api so that chunked JSON objects are not buffered anymore. This touches a bunch of the files pushing the transform function down to the individual commands.

haadcode · 2016-12-21T10:40:03Z

I fixed the style issues, updated pump to 1.0.2 and rebased on master.

Other than the hash vs multihash thing, it LGTM. I'm fine if Files API uses hash, although I would prefer consistency.

@diasdavid would you be ok if we leave the change from .hash to .multihash to a separate PR? This is a large PR in itself and I'm afraid of the consequences the aforementioned change would have on users (being an API change). Perhaps you have more confidence to do it as you know the codebase throughout, but I would prefer doing it in one, separate PR where we update all functions (that return .hash currently) at the same time. Let me know what you think.

daviddias · 2016-12-21T15:01:13Z

@diasdavid would you be ok if we leave the change from .hash to .multihash to a separate PR?

sounds good :)

haadcode · 2017-01-02T21:08:09Z

Didn't even notice this got merged. Thank you! 😄 🎉

haadcode added the in progress label Dec 5, 2016

dignifiedquire suggested changes Dec 5, 2016

View reviewed changes

haadcode mentioned this pull request Dec 5, 2016

feature: add pubsub (tests and API Spec) ipfs-inactive/interface-js-ipfs-core#101

Merged

haadcode added the needs review label Dec 5, 2016

haadcode self-assigned this Dec 5, 2016

daviddias suggested changes Dec 6, 2016

View reviewed changes

daviddias removed the needs review label Dec 6, 2016

haadcode mentioned this pull request Dec 6, 2016

(WIP) feature: floodsub #470

Merged

daviddias mentioned this pull request Dec 6, 2016

WIP: feat/pubsub #471

Closed

haadcode added ready and removed in progress labels Dec 12, 2016

daviddias reviewed Dec 18, 2016

View reviewed changes

haadcode added in progress and removed ready labels Dec 19, 2016

dignifiedquire reviewed Dec 20, 2016

View reviewed changes

dignifiedquire approved these changes Dec 20, 2016

View reviewed changes

dignifiedquire added the needs review label Dec 20, 2016

dignifiedquire assigned daviddias Dec 20, 2016

daviddias removed the needs review label Dec 21, 2016

daviddias approved these changes Dec 21, 2016

View reviewed changes

haadcode added 2 commits December 21, 2016 11:19

Improve function descriptions

ecdbf6a

haadcode and others added 6 commits December 21, 2016 11:19

Change variable name to be more descriptive in dagnode-stream

738056f

Refactor tar-stream-to-objects to better show intent

eb88227

Use stream-to-value in Block command instead of bl

25061cf

test: fix daemon config setting

c71eb58

Fix deps after rebase and use pump 1.0.2

2031362

Add curlies around error callback

da14a60

daviddias merged commit 753d19d into ipfs-inactive:master Dec 21, 2016

daviddias removed the in progress label Dec 21, 2016

refactor(streams): Refactor response stream handling. #465

refactor(streams): Refactor response stream handling. #465

Conversation

haadcode commented Dec 5, 2016 • edited by dignifiedquire Loading

dignifiedquire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haadcode Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a 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

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

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

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

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

haadcode commented Dec 7, 2016

daviddias Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haadcode commented Dec 19, 2016

haadcode commented Dec 19, 2016

dignifiedquire commented Dec 19, 2016

haadcode commented Dec 20, 2016

dignifiedquire commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias left a comment

Choose a reason for hiding this comment

haadcode commented Dec 21, 2016

daviddias commented Dec 21, 2016

haadcode commented Jan 2, 2017

haadcode commented Dec 5, 2016 •

edited by dignifiedquire

Loading

haadcode Dec 5, 2016 •

edited

Loading

daviddias Dec 18, 2016 •

edited

Loading