Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: make StreamWrap work correctly when waiting for "drain" events in doShutdown #23294

Closed
wants to merge 1 commit into from

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Oct 6, 2018

When an instance of StreamWrap is shutting down and a "drain" event is emitted, the process will abort as its this[kCurrentShutdownRequest] is already set by doShutdown itself.

The following test will fail before this commit.

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

The following test will fail before this commit.

Is there a way to reach this error through public API? If so, can we use that for the test?

@@ -114,7 +111,9 @@ class JSStreamWrap extends Socket {

if (this[kCurrentWriteRequest] !== null)
return this.on('drain', () => this.doShutdown(req));
assert.strictEqual(this[kCurrentWriteRequest], null);
Copy link
Member

Choose a reason for hiding this comment

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

This assertion can stay, right? Or is it “too obvious” that it’s true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. We can't be too careful here.

@@ -114,7 +111,9 @@ class JSStreamWrap extends Socket {

if (this[kCurrentWriteRequest] !== null)
return this.on('drain', () => this.doShutdown(req));
Copy link
Member

Choose a reason for hiding this comment

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

Aside: This should be .once(), I think?

@oyyd
Copy link
Contributor Author

oyyd commented Oct 7, 2018

I reached this error when I tried to use StreamWrap to wrap a server-side socket which, IMHO, might be a solution to issues like #10871.

In fact, I can't produce this through public API as doShutdown are always called by functions from stream.Writable instead of onshutdown from JSStream, IDK. This test is an imitation of this one. Therefore, feel free to oppose this PR.

As the situations that would reach this line of code seem to be rare, removing the listener to drain might come into consideration? (I'm not sure)

When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.
@oyyd
Copy link
Contributor Author

oyyd commented Oct 7, 2018

The travis ci is green now. I didn't notice the core-validate-commit there.

@addaleax
Copy link
Member

addaleax commented Oct 7, 2018

@addaleax addaleax added tls Issues and PRs related to the tls subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Oct 7, 2018
@danbev
Copy link
Contributor

danbev commented Oct 12, 2018

Landed in e4dea40.

@danbev danbev closed this Oct 12, 2018
danbev pushed a commit that referenced this pull request Oct 12, 2018
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: #23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Trott pushed a commit that referenced this pull request Oct 13, 2018
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: #23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
targos pushed a commit that referenced this pull request Oct 13, 2018
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: #23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: #23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 30, 2018
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: #23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: #23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: #23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants