Skip to content

Conversation

ronag
Copy link
Member

@ronag ronag commented Sep 7, 2019

Stop flowing once destroyed.

Currently stream implementation MUST check for destroyed inside _read.

Found this while investigating a suspicious destroyed conditional in fs streams.

This is semver major unfortunately. The existing behaviour is to flush the readable and invoke end after destroy(). See updated tests.

If preferable I can make a less breaking version of this (instead of or as an intermediate?) that just makes sure _read is not called after destroy(). Though it does not fully "correct" the behaviour.

Refs: #29477

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Sep 7, 2019
@ronag ronag force-pushed the read-after-destroy branch from 9c3f707 to 995f432 Compare September 7, 2019 21:56
Copy link
Member Author

Choose a reason for hiding this comment

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

This test doesn't seem to be testing correct behaviour. We should not have either 'end' nor 'finish' after destroy().

@ronag ronag force-pushed the read-after-destroy branch 3 times, most recently from 58d1e79 to 7083dc8 Compare September 7, 2019 22:02
@ronag ronag force-pushed the read-after-destroy branch from 7083dc8 to 593e6b2 Compare September 7, 2019 22:02
@ronag
Copy link
Member Author

ronag commented Sep 8, 2019

This needs more work. Simpler version on the way.

@ronag ronag closed this Sep 8, 2019
@ronag ronag changed the title stream: don't call _read after destroy() [x] stream: don't call _read after destroy() Sep 8, 2019
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.

2 participants