Skip to content

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

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

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

Conversation

@ronag

@ronag ronag commented Jul 18, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

ronag commented Jul 23, 2019

Copy link
Copy Markdown
Member Author

ping @benjamingr

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

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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?

Comment thread lib/internal/streams/end-of-stream.js Outdated
Comment thread lib/internal/streams/end-of-stream.js Outdated
@ronag ronag force-pushed the stream-legacy-listener branch 11 times, most recently from 9482e59 to 176ef57 Compare July 30, 2019 19:44
@ronag

ronag commented Aug 2, 2019

Copy link
Copy Markdown
Member Author

@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

mcollina commented Aug 2, 2019

Copy link
Copy Markdown
Member

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

@ronag

ronag commented Aug 2, 2019

Copy link
Copy Markdown
Member Author

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

@mcollina

mcollina commented Aug 3, 2019

Copy link
Copy Markdown
Member

the tests are in test-stream-finished.js

This was referenced Aug 6, 2019
Closed
Closed
@jasnell

jasnell commented Aug 21, 2019

Copy link
Copy Markdown
Member

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

ronag commented Aug 21, 2019

Copy link
Copy Markdown
Member Author

@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

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

ronag commented Aug 22, 2019

Copy link
Copy Markdown
Member Author

this is supposed to work with users streams as well.

Any particular concern I can try to address?

@mcollina

mcollina commented Aug 22, 2019

Copy link
Copy Markdown
Member

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

ronag commented Sep 23, 2019

Copy link
Copy Markdown
Member Author

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