Skip to content

Conversation

@ronag
Copy link
Member

@ronagronag commented Apr 6, 2020

Slightly cleans up the destroyer logic. Make readable and writable use similar logic.

i.e.

if(!err&&reading&&!writing&&stream.writable){returncallback();}if(!err&&!reading&&writing&&stream.readable){returncallback();}
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

Slightly cleans up the destroyer logic.
@ronagronag added the stream Issues and PRs related to the stream subsystem. label Apr 6, 2020
@nodejs-github-bot
Copy link
Collaborator

destroyImpl.destroyer(stream,err);
if(!err&&!reading&&writing&&stream.readable){
returncallback();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe merge writable and readable.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

not sure I follow?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 13, 2020

Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronagronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 20, 2020

if(err||!final||!stream.readable){
destroyImpl.destroyer(stream,err);
if(!err&&!reading&&writing&&stream.readable){
Copy link
Member

Choose a reason for hiding this comment

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

if(!err){if(reading){if(!writing&&stream.writable){returncallback();}}elseif(writing&&stream.readable){returncallback();}}

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I suppose this is a nit?

@ronagronag added blocked PRs that are blocked by other issues or PRs. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 21, 2020
@ronag
Copy link
MemberAuthor

This might be redundant given #32966

@ronag
Copy link
MemberAuthor

closed in favor of #32968

@ronagronag closed this Apr 21, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blockedPRs that are blocked by other issues or PRs.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ronag@nodejs-github-bot@mcollina@BridgeAR@rickyes