Skip to content

[x] stream: eos avoid listeners when possible#28751

Closed
ronag wants to merge 4 commits intonodejs:masterfrom
nxtedition:stream-legacy-listener
Closed

[x] stream: eos avoid listeners when possible#28751
ronag wants to merge 4 commits intonodejs:masterfrom
nxtedition:stream-legacy-listener

Conversation

@ronag
Copy link
Member

@ronag ronag commented Jul 18, 2019 •
edited
Loading

This slightly improves performance and maintainability by applying legacy compat logic only when required.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@ronag ronag changed the title streams: avoid listeners when not legacy compat streams: eos avoid listeners when not legacy compat Jul 18, 2019
@ronag ronag force-pushed the stream-legacy-listener branch 2 times, most recently from fe38aea to bd553ab Compare July 18, 2019 17:52
@lpinca lpinca added the stream Issues and PRs related to the stream subsystem. label Jul 19, 2019
@ronag ronag changed the title streams: eos avoid listeners when not legacy compat stream: eos avoid listeners when not legacy compat Jul 23, 2019
@ronag
Copy link
Member Author

ronag commented Jul 23, 2019

ping @benjamingr

@jasnell jasnell requested a review from mcollina July 29, 2019 15:22
mcollina
mcollina requested changes Jul 29, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Would you mind adding a unit test?

@ronag ronag force-pushed the stream-legacy-listener branch 11 times, most recently from 9482e59 to 176ef57 Compare July 30, 2019 19:44
@ronag
Copy link
Member Author

ronag commented Aug 2, 2019

@mcollina: Not sure I understand what to unit test here? Shouldn't existing unit tests already cover this? It's just an optimisation. Shouldn't be behavioural change.

@mcollina
Copy link
Member

mcollina commented Aug 2, 2019

You can test that those listeners are not added in that case.

@ronag
Copy link
Member Author

ronag commented Aug 2, 2019

@mcollina As far as I can see there are no tests for end-of-stream whatsoever? Or am I missing something?

@mcollina
Copy link
Member

mcollina commented Aug 3, 2019

the tests are in test-stream-finished.js

This was referenced Aug 6, 2019
Closed
Closed
@jasnell
Copy link
Member

jasnell commented Aug 21, 2019

This is still waiting on the addition of the tests.

@ronag ronag force-pushed the stream-legacy-listener branch from 176ef57 to e9655f8 Compare August 21, 2019 17:55
@ronag
Copy link
Member Author

ronag commented Aug 21, 2019

@jasnell: rebased, added test and added a few more conditions

@ronag ronag changed the title stream: eos avoid listeners when not legacy compat stream: eos avoid listeners when possible Aug 21, 2019
@ronag ronag force-pushed the stream-legacy-listener branch 2 times, most recently from 7d65c09 to aefa28d Compare August 21, 2019 17:58
@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 22, 2019
mcollina
mcollina reviewed Aug 22, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I'm flagging it semver-major out of caution, this is supposed to work with users streams as well.

@mcollina mcollina requested review from addaleax and mafintosh August 22, 2019 17:29
@ronag
Copy link
Member Author

ronag commented Aug 22, 2019

this is supposed to work with users streams as well.

Any particular concern I can try to address?

@mcollina
Copy link
Member

mcollina commented Aug 22, 2019

A review from @mafintosh would help.

@ronag ronag force-pushed the stream-legacy-listener branch from aefa28d to f4b5ddb Compare August 22, 2019 17:40
@ronag ronag force-pushed the stream-legacy-listener branch from f4b5ddb to edb7e47 Compare August 22, 2019 22:47
@ronag
Copy link
Member Author

ronag commented Sep 23, 2019

I'm putting this on ice for now. Will re-open later.

@ronag ronag closed this Sep 23, 2019
@ronag ronag changed the title stream: eos avoid listeners when possible [x] stream: eos avoid listeners when possible Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina requested changes

@addaleax addaleax Awaiting requested review from addaleax

@mafintosh mafintosh Awaiting requested review from mafintosh

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ronag @mcollina @jasnell @lpinca