Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

[WIP] Pull-streams #7

Merged
merged 5 commits into from
Sep 6, 2016
Merged

[WIP] Pull-streams #7

merged 5 commits into from
Sep 6, 2016

Conversation

dignifiedquire
Copy link
Member

No description provided.

@daviddias
Copy link
Member

daviddias commented Aug 19, 2016

Just to be sure that I see the same problems as you, can you describe the steps to run this one.

stream,
pull.collect((err, res) => {
expect(err).to.not.exist.mark()
expect(res).to.be.eql([]).mark()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dignifiedquire why would the collected values be empty arrays?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the other side does not send us anything

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See l32

Copy link
Member

@daviddias daviddias Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And empty array means end of the stream? I see the pull.empty, just confirming that is the right pattern

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pull.collect, drains the stream and collects all values into an array, or error into err. It will only be called when the stream ended.

@daviddias
Copy link
Member

daviddias commented Aug 26, 2016

Keeping up state, from me and @dignifiedquire debugging:

  • spdy on top of Node.js Streams and exposing Node.js Streams passes all the tests (even the stresses tests
  • spdy on top of Node.js Streams and exposing pull-streams passes all the tests (even the stresses tests, but with the caveat of running out of memory, needs to be throttled)
  • spdy with on top and exposing pull-streams fails, unless heavily throttled. all good now

@@ -17,7 +17,7 @@ module.exports = (common) => {
})

it('10000 messages of 10000 streams', (done) => {
spawn(muxer, 10000, 10000, done)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing values after the callback? Just for testing and not for merge?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callback is required, limit is optional, so limit comes after the callback :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is a really new pattern..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just to annoy you :trollface:

@daviddias
Copy link
Member

daviddias commented Sep 2, 2016

@dignifiedquire seeing ws tests failing:

      1) open a multiplex stream from dialer
      2) open a multiplex stream from listener

Is it missing your ws fix on pull-streams?

Other last things before the merge:

@dignifiedquire
Copy link
Member Author

@diasdavid yes ws will only pass with pull-stream/pull-stream-to-stream#4


In the Node.js case, if no callback is passed, stream will emit an 'ready' event when it is prepared or a 'error' event if it fails to establish the connection, until then, it will buffer the 'write' calls.
In the JavaScript case, if no callback is passed, stream will emit an 'ready' event when it is prepared or a 'error' event if it fails to establish the connection, until then, it will buffer the 'write' calls.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this holds true anymore, maybe it is not even needed. @dignifiedquire

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No dial will just return a pull-stream ready to be used

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • change this paragraph

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants