★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/25026Nouvelle recherche | Portail wanayoo
Skip to content

/node

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: check for close on stream, not parent #25026

Closed
wants to merge 1 commit into from

Conversation

@davedoesdev
Copy link
Contributor

commented Dec 13, 2018 •
edited

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 
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

Fixes: #24984
Loading status checks…
e9597e9

@nodejs-github-bot nodejs-github-bot added the net label Dec 13, 2018

@lpinca
lpinca reviewed Dec 14, 2018
View changes
lib/net.js
@@ -371,8 +371,8 @@ Socket.prototype._final = function(cb) {
};


function afterShutdown(status, handle) {

This comment has been minimized.

Sign in to view
Copy link
@lpinca

lpinca Dec 14, 2018

Member

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

This comment has been minimized.

Sign in to view
Copy link
@davedoesdev

davedoesdev Dec 14, 2018

Author Contributor

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.

This comment has been minimized.

Sign in to view
Copy link
@lpinca

lpinca Dec 14, 2018

Member

Thanks for the explanation.
@lpinca
lpinca approved these changes Dec 14, 2018
View changes

@lpinca lpinca added the tls label Dec 14, 2018

@lpinca

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 14, 2018 •
edited by Trott

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

@Trott Trott added the author ready label Dec 14, 2018

@addaleax
addaleax approved these changes Dec 15, 2018
View changes

@addaleax addaleax added this to To do in StreamBase refactor via automation Dec 15, 2018

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 15, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Dec 19, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 20, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

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 
'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>
86e2ec4
targos added a commit that referenced this pull request Jan 1, 2019
@davedoesdev @targos
net: check for close on stream, not parent 
'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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
9ac8d41
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
@davedoesdev @refack
net: check for close on stream, not parent 
'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>
27a3c3d
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
Merged
@MylesBorins MylesBorins referenced 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
Loading status checks…
96f1bed
BethGriggs added a commit that referenced this pull request Apr 17, 2019
@davedoesdev @BethGriggs
net: check for close on stream, not parent 
'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>
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
85880cb
BethGriggs added a commit that referenced this pull request Apr 28, 2019
@davedoesdev @BethGriggs
net: check for close on stream, not parent 
'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>
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
4598ed7
@BethGriggs BethGriggs referenced this pull request May 1, 2019
Merged
BethGriggs added a commit that referenced this pull request May 10, 2019
@davedoesdev @BethGriggs
net: check for close on stream, not parent 
'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>
Verified
This commit was signed with a verified signature.
BethGriggs Bethany Nicolle Griggs
GPG key ID: D7062848A1AB005C Learn about signing commits
958b97f
MylesBorins added a commit that referenced this pull request May 16, 2019
@davedoesdev @MylesBorins
net: check for close on stream, not parent 
'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>
Verified
This commit was signed with a verified signature.
MylesBorins Myles Borins
GPG key ID: 933B01F40B5CA946 Learn about signing commits
f17b61e
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
@beevelop beevelop referenced 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

@lpinca lpinca

Assignees
No one assigned
Labels
author ready net tls
Projects
  
Done in StreamBase refactor
Milestone
No milestone
6 participants
@davedoesdev @lpinca @addaleax @danbev @Trott @nodejs-github-bot
You can’t perform that action at this time.