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.

Sign up for GitHub

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

Jump to bottom

net: check for close on stream, not parent #25026

Closed
wants to merge 1 commit into from
Closed

net: check for close on stream, not parent #25026

wants to merge 1 commit into from

Conversation

davedoesdev
Copy link
Contributor

@davedoesdev davedoesdev commented Dec 13, 2018 •
edited
Loading

afterShutdown was checking parent stream rather than TLS stream
Fixes #24984

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

@davedoesdev
net: check for close on stream, not parent
e9597e9 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

Fixes: #24984
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Dec 13, 2018
lpinca
lpinca reviewed Dec 14, 2018
View reviewed changes
lib/net.js
@@ -371,8 +371,8 @@ Socket.prototype._final = function(cb) {
};


function afterShutdown(status, handle) {
Copy link
Member

Choose a reason for hiding this comment

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

Do you know why this gets called with the wrong handle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from here: https://github.com/nodejs/node/blob/v11.4.0/src/stream_base.cc#L383
This in turn is set by https://github.com/nodejs/node/blob/v11.4.0/src/stream_base-inl.h#L86
which is called by https://github.com/nodejs/node/blob/v11.4.0/src/tls_wrap.cc#L73

TLSWrap's stream is the external stream from here: https://github.com/nodejs/node/blob/v11.4.0/lib/_tls_wrap.js#L423

which is the unencrypted socket passed into the constructor: https://github.com/nodejs/node/blob/v11.4.0/lib/_tls_wrap.js#311

In summary, it gets the underlying socket's stream rather than the encrypted stream (TLSSocket). Node stream operations appear to be performed on TLSSocket only.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation.

@lpinca lpinca added the tls Issues and PRs related to the tls subsystem. label Dec 14, 2018
@lpinca
Copy link
Member

lpinca commented Dec 14, 2018 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/19512/ ✔️

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2018
@addaleax addaleax added this to To do in StreamBase refactor via automation Dec 15, 2018
@addaleax
Copy link
Member

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1690/

@danbev
Copy link
Contributor

danbev commented Dec 19, 2018

Re-run of CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1694/

@davedoesdev
Copy link
Contributor Author

Do I need to do anything about the CITGM? They look like compile failures.

@lpinca
Copy link
Member

lpinca commented Dec 26, 2018

Landed in 86e2ec4.

@lpinca lpinca closed this Dec 26, 2018
StreamBase refactor automation moved this from To do to Done Dec 26, 2018
pull bot pushed a commit to shakir-abdo/node that referenced this pull request Dec 26, 2018
@davedoesdev @lpinca
net: check for close on stream, not parent
86e2ec4 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: nodejs#25026
Fixes: nodejs#24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Jan 1, 2019
@davedoesdev @targos
net: check for close on stream, not parent
9ac8d41 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@davedoesdev @refack
net: check for close on stream, not parent
27a3c3d 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: nodejs#25026
Fixes: nodejs#24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
davedoesdev added a commit to davedoesdev/centro that referenced this pull request Feb 6, 2019
@davedoesdev
Rev deps, use latest Node with nodejs/node#25026
96f1bed
BethGriggs pushed a commit that referenced this pull request Apr 17, 2019
@davedoesdev @BethGriggs
net: check for close on stream, not parent
85880cb 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@davedoesdev @BethGriggs
net: check for close on stream, not parent
4598ed7 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@davedoesdev @BethGriggs
net: check for close on stream, not parent
958b97f 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@davedoesdev @MylesBorins
net: check for close on stream, not parent
f17b61e 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
@beevelop beevelop mentioned this pull request May 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. net Issues and PRs related to the net subsystem. tls Issues and PRs related to the tls subsystem.
Projects
No open projects
StreamBase refactor
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

close event emitted on net socket but not tls
6 participants
@davedoesdev @lpinca @addaleax @danbev @Trott @nodejs-github-bot