Skip to content

stream: don't read when paused#34106

Closed
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:readable-paused-read
Closed

stream: don't read when paused#34106
ronag wants to merge 2 commits into
nodejs:masterfrom
nxtedition:readable-paused-read

Conversation

@ronag

@ronag ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member

Readable should not read when paused. Noticed while working
on further streamifying net with _construct.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Readable should not read when paused. Noticed while working
on further streamifying net with _construct.
@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jun 28, 2020
@ronag

ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member Author

@nodejs/streams

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@lpinca

lpinca commented Jun 28, 2020

Copy link
Copy Markdown
Member

What happens if there is buffered data and the stream is destroyed after pause? Is it possible to read the buffered data?

It seems this does not touch Readable.prototype.read() so it should still work, just want to be sure as a lot changed on streams and I did not follow everything closely.

@ronag

ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member Author

What happens if there is buffered data and the stream is destroyed after pause? Is it possible to read the buffered data?

I'm not sure I follow the question. maybeReadMore is only invoked when pushing more chunks, which I don't think you can do after destroying the stream. So I don't think it's relevant in the context you are referring to.

just want to be sure as a lot changed on streams and I did not follow everything closely.

Yea, I can't say I'm an expert on the readable side of things either.

@ronag

This comment has been minimized.

@ronag

This comment has been minimized.

@ronag

This comment has been minimized.

@ronag

ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member Author

There is something really weird going on here with js_stream_socket in relation to the original commit in this PR.

@ronag ronag requested a review from addaleax June 28, 2020 20:12
@ronag ronag force-pushed the readable-paused-read branch 2 times, most recently from 93c43da to d5960f5 Compare June 28, 2020 20:17
Comment thread test/parallel/test-stream2-unpipe-leak.js Outdated
@ronag ronag force-pushed the readable-paused-read branch from 3af0084 to 8da065a Compare June 28, 2020 20:24
@ronag

ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member Author

Another problem here is that pipeOnDrain (not sure if related) doesn't call resume but instead does flowing =true, even though pipe uses pause().

This is quite a rabbit hole. I won't be able to sleep if I continue digging 😨.

@ronag

ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member Author

I think it's best to leave this in its current and simplest form.

There are a lot of issues here that need to be resolved to fix this properly. A bit overwhelming at the moment.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronag ronag mentioned this pull request Jun 28, 2020
25 tasks
@ronag

ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member Author

Even simplest form fails. Closing this for now.

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

Labels

stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants