Skip to content
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

Doc: stream becomes paused after unpipe. #5156

Closed
wants to merge 4 commits into from
Closed

Doc: stream becomes paused after unpipe. #5156

wants to merge 4 commits into from

Conversation

iliakan
Copy link

@iliakan iliakan commented Feb 9, 2016

Fixed the docs.
isPaused does not require explicit pause call in fact.

@mscdex mscdex added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Feb 9, 2016
@jasnell
Copy link
Member

jasnell commented Feb 10, 2016

LGTM

@silverwind
Copy link
Contributor

Please wrap the lines at 80 chars.

@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

@chrisdickinson

@jasnell
Copy link
Member

jasnell commented Apr 21, 2016

@nodejs/documentation ... can we please get some additional review on this one?
@iliakan ... can you rebase and update this?

@eljefedelrodeodeljefe
Copy link
Contributor

LGTM pending wrapping at 80 chars

@iliakan
Copy link
Author

iliakan commented Apr 22, 2016

did it

@estliberitas estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
This method returns whether or not the `readable` has been **explicitly**
paused by client code (using [`stream.pause()`][stream-pause] without a
corresponding [`stream.resume()`][stream-resume]).
This method returns whether or not the `readable` has been paused
Copy link
Member

Choose a reason for hiding this comment

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

nit: Trailing whitespace.

@addaleax
Copy link
Member

LGTM with a nit.

@iliakan It would be cool if you could rebase this, and ideally pick a commit message more with more resemblance to the PR title here. We can do that for you on landing if you prefer, though.

Also, these edits appears to have been made from the Github editor and have an @users.noreply.github.com email address set in the Author: field. Is that intentional? If you want to have it changed, you can do that when rebasing, or have us do it on landing, too.

@addaleax
Copy link
Member

This would also fix #4812, I think.

@iliakan
Copy link
Author

iliakan commented Apr 29, 2016

Actually, there are 3 state of the stream right now.
The docs do not say the truth:

  1. flowing
  2. paused
  3. explicitly paused :/

@addaleax
Copy link
Member

I think it would be awesome if you could update this PR with that information? And probably good to ping @nodejs/streams


* If there are no pipe destinations, by calling the
[`stream.pause()`][stream-pause] method.
* If there are pipe destinations, by removing any [`'data'`][] event
Copy link
Member

@mcollina mcollina Apr 29, 2016

Choose a reason for hiding this comment

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

all 'data' event handlers? Please check.

@mcollina
Copy link
Member

@iliakan I think the states are 3, but not the one you said:

  1. flowing
  2. not flowing
  3. explicitly paused (implies not flowing)

According to:
https://github.com/nodejs/node/blob/master/lib/_stream_readable.js#L139-L141

isPaused() means "not flowing".

@Trott
Copy link
Member

Trott commented Nov 3, 2016

This looks like it stalled awfully close to being land-able. @iliakan: Any chance you can rebase against master and pick this up again? (Or did this information end up in the docs already via a different pull request?)

@iliakan
Copy link
Author

iliakan commented Nov 3, 2016

The doc was rewritten since then, maybe it still needs adjusting.

Because we have (citing):

"The Readable can switch back to paused mode using one of the following:... If there are pipe destinations, by removing any 'data' event handlers
...
Note: For backwards compatibility reasons, removing 'data' event handlers will not automatically pause the stream."

These statements contradict to each other, what you think? It's really unclear what's going on.

@Trott
Copy link
Member

Trott commented Nov 4, 2016

These statements contradict to each other, what you think? It's really unclear what's going on.

Re-pinging @nodejs/streams for comment.

@mcollina
Copy link
Member

mcollina commented Nov 5, 2016

The two full sentences are:

If there are pipe destinations, by removing any 'data' event handlers, and removing all pipe destinations by calling the stream.unpipe() method.

and

For backwards compatibility reasons, removing 'data' event handlers will not automatically pause the stream. Also, if there are piped destinations, then calling stream.pause() will not guarantee that the stream will remain paused once those destinations drain and ask for more data.

If a stream is flowing, there are three situations:

a. there is a 'data' event handler
b. there are are piped destinations
c. there are multiple 'data' event handlers and piped destinations

To pause a stream:

a. call pause()
b. call unpipe()
c. remove all 'data' handlers, and unpipe().

We probably need to add some unit tests to cover all this cases anyway, if they are missing.

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jan 6, 2017
@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

@iliakan @mcollina ... ping. What's the status on this?

@mcollina
Copy link
Member

mcollina commented Jan 6, 2017

@jasnell it's stalled. Maybe someone else should take over.

@mcollina
Copy link
Member

@jasnell on second thoughts, I think this was fixed long ago in #6947. I think we can close this.

@jasnell jasnell closed this Jan 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stalled Issues and PRs that are stalled. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants