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

net: simplify Socket.prototype._final #24075

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 4, 2018

Remove conditions that should be irrelevant since we started
using _final, as well as an extra defaultTriggerAsyncIdScope()
call which is unnecessary because there is an equivalent
scope already present on the native side.

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

Remove conditions that should be irrelevant since we started
using `_final`, as well as an extra `defaultTriggerAsyncIdScope()`
call which is unnecessary because there is an equivalent
scope already present on the native side.
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Nov 4, 2018
@addaleax
Copy link
Member Author

addaleax commented Nov 4, 2018

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Nov 5, 2018
@addaleax
Copy link
Member Author

addaleax commented Nov 5, 2018

Sigh … looks like Windows isn’t happy with this change. I’ll look into it.

@addaleax
Copy link
Member Author

addaleax commented Nov 5, 2018

Trying something...

CI: https://ci.nodejs.org/job/node-test-pull-request/18338/

@addaleax addaleax removed the wip Issues and PRs that are still a work in progress. label Nov 5, 2018
@addaleax
Copy link
Member Author

addaleax commented Nov 7, 2018

Any other thoughts/reviewers?

@addaleax
Copy link
Member Author

Landed in b7e6ccd

@addaleax addaleax closed this Nov 10, 2018
@addaleax addaleax deleted the net-simplify-final branch November 10, 2018 18:40
addaleax added a commit that referenced this pull request Nov 10, 2018
Remove conditions that should be irrelevant since we started
using `_final`, as well as an extra `defaultTriggerAsyncIdScope()`
call which is unnecessary because there is an equivalent
scope already present on the native side.

PR-URL: #24075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Nov 10, 2018
Partially revert b7e6ccd
because it broke a test that was added since its last CI run.

Refs: nodejs#24075
Refs: nodejs#23866
addaleax added a commit that referenced this pull request Nov 10, 2018
Partially revert b7e6ccd
because it broke a test that was added since its last CI run.

Refs: #24075
Refs: #23866

PR-URL: #24288
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Nov 10, 2018
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Remove conditions that should be irrelevant since we started
using `_final`, as well as an extra `defaultTriggerAsyncIdScope()`
call which is unnecessary because there is an equivalent
scope already present on the native side.

PR-URL: #24075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
Partially revert b7e6ccd
because it broke a test that was added since its last CI run.

Refs: #24075
Refs: #23866

PR-URL: #24288
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Remove conditions that should be irrelevant since we started
using `_final`, as well as an extra `defaultTriggerAsyncIdScope()`
call which is unnecessary because there is an equivalent
scope already present on the native side.

PR-URL: nodejs#24075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Partially revert b7e6ccd
because it broke a test that was added since its last CI run.

Refs: nodejs#24075
Refs: nodejs#23866

PR-URL: nodejs#24288
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
addaleax added a commit that referenced this pull request Nov 16, 2018
`'drain'` event handlers may not be invoked if the stream
is currently finishing. Instead, use the fact that we know
when writes are active or not, and invoke the delayed shutdown
handler from our own after-write callback.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
addaleax added a commit that referenced this pull request Nov 16, 2018
Previously, there was no mechanism in place that would
have destroyed the TLS socket once the underlying socket
had been closed.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
addaleax added a commit that referenced this pull request Nov 16, 2018
This reverts commit ac1f56c.

Refs: #24288
Refs: #24075

PR-URL: #24290
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
targos pushed a commit that referenced this pull request Nov 18, 2018
`'drain'` event handlers may not be invoked if the stream
is currently finishing. Instead, use the fact that we know
when writes are active or not, and invoke the delayed shutdown
handler from our own after-write callback.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
targos pushed a commit that referenced this pull request Nov 18, 2018
Previously, there was no mechanism in place that would
have destroyed the TLS socket once the underlying socket
had been closed.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
targos pushed a commit that referenced this pull request Nov 18, 2018
This reverts commit ac1f56c.

Refs: #24288
Refs: #24075

PR-URL: #24290
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
`'drain'` event handlers may not be invoked if the stream
is currently finishing. Instead, use the fact that we know
when writes are active or not, and invoke the delayed shutdown
handler from our own after-write callback.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Previously, there was no mechanism in place that would
have destroyed the TLS socket once the underlying socket
had been closed.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
This reverts commit ac1f56c.

Refs: #24288
Refs: #24075

PR-URL: #24290
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
codebytere pushed a commit that referenced this pull request Dec 14, 2018
Remove conditions that should be irrelevant since we started
using `_final`, as well as an extra `defaultTriggerAsyncIdScope()`
call which is unnecessary because there is an equivalent
scope already present on the native side.

PR-URL: #24075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Dec 14, 2018
Partially revert b7e6ccd
because it broke a test that was added since its last CI run.

Refs: #24075
Refs: #23866

PR-URL: #24288
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
Remove conditions that should be irrelevant since we started
using `_final`, as well as an extra `defaultTriggerAsyncIdScope()`
call which is unnecessary because there is an equivalent
scope already present on the native side.

PR-URL: #24075
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
Partially revert b7e6ccd
because it broke a test that was added since its last CI run.

Refs: #24075
Refs: #23866

PR-URL: #24288
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@codebytere codebytere mentioned this pull request Jan 4, 2019
codebytere pushed a commit that referenced this pull request Jan 12, 2019
`'drain'` event handlers may not be invoked if the stream
is currently finishing. Instead, use the fact that we know
when writes are active or not, and invoke the delayed shutdown
handler from our own after-write callback.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
codebytere pushed a commit that referenced this pull request Jan 12, 2019
Previously, there was no mechanism in place that would
have destroyed the TLS socket once the underlying socket
had been closed.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
codebytere pushed a commit that referenced this pull request Jan 12, 2019
This reverts commit ac1f56c.

Refs: #24288
Refs: #24075

PR-URL: #24290
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
codebytere pushed a commit that referenced this pull request Jan 29, 2019
`'drain'` event handlers may not be invoked if the stream
is currently finishing. Instead, use the fact that we know
when writes are active or not, and invoke the delayed shutdown
handler from our own after-write callback.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
codebytere pushed a commit that referenced this pull request Jan 29, 2019
Previously, there was no mechanism in place that would
have destroyed the TLS socket once the underlying socket
had been closed.

PR-URL: #24290
Refs: #24288
Refs: #24075
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
codebytere pushed a commit that referenced this pull request Jan 29, 2019
This reverts commit ac1f56c.

Refs: #24288
Refs: #24075

PR-URL: #24290
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Ouyang Yadong <oyydoibh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants