tls: call `destroy()` on StreamWrap and their streams by oyyd · Pull Request #23654 · nodejs/node · GitHub

When sockets of the "net" module destroyed, they will call this._handle.close() which will also emit EOF if not emitted before.

This feature makes sockets on the other side emit "end" and "close" even though we haven't called end(). As stream of StreamWrap are likely to be instances of net.Socket, calling destroy() manually will avoid issues that don't properly close wrapped connections.

Fixes: #14605

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

When sockets of the "net" module destroyed, they will call `this._handle.close()` which will also emit EOF if not emitted before. This feature makes sockets on the other side emit "end" and "close" even though we haven't called `end()`. As `stream` of `StreamWrap` are likely to be instances of `net.Socket`, calling `destroy()` manually will avoid issues that don't properly close wrapped connections. Fixes: nodejs#14605

Thanks for digging through this, awesome PR!

CI: https://ci.nodejs.org/job/node-test-pull-request/17851/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1587/

/cc @mcollina

Landed in 517955a, thanks for the PR again!

If you don’t see any activity on one of your PRs for a few days, feel free to ping somebody about that. :)

When sockets of the "net" module destroyed, they will call `this._handle.close()` which will also emit EOF if not emitted before. This feature makes sockets on the other side emit "end" and "close" even though we haven't called `end()`. As `stream` of `StreamWrap` are likely to be instances of `net.Socket`, calling `destroy()` manually will avoid issues that don't properly close wrapped connections. Fixes: #14605 PR-URL: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

I see. Thanks 😉

When sockets of the "net" module destroyed, they will call `this._handle.close()` which will also emit EOF if not emitted before. This feature makes sockets on the other side emit "end" and "close" even though we haven't called `end()`. As `stream` of `StreamWrap` are likely to be instances of `net.Socket`, calling `destroy()` manually will avoid issues that don't properly close wrapped connections. Fixes: #14605 PR-URL: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

This test ensures that a tls client socket using `StreamWrap` with `allowHalfOpen` option won't hang. PR-URL: #23866 Refs: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

When sockets of the "net" module destroyed, they will call `this._handle.close()` which will also emit EOF if not emitted before. This feature makes sockets on the other side emit "end" and "close" even though we haven't called `end()`. As `stream` of `StreamWrap` are likely to be instances of `net.Socket`, calling `destroy()` manually will avoid issues that don't properly close wrapped connections. Fixes: #14605 PR-URL: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

this lands cleanly on 10.x but has test failure on 8.x

would someone be willing to backport and figure out what is going on?

=== release test-tls-destroy-stream ===
Path: parallel/test-tls-destroy-stream
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/mborins/code/node/v8.x/test/common/index.js:439:10)
    at Socket.conn.once.common.mustCall (/Users/mborins/code/node/v8.x/test/parallel/test-tls-destroy-stream.js:36:35)
    at Socket.<anonymous> (/Users/mborins/code/node/v8.x/test/common/index.js:479:15)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:116:13)
    at Socket.emit (events.js:211:7)
    at addChunk (_stream_readable.js:263:12)
    at readableAddChunk (_stream_readable.js:250:11)
    at Socket.Readable.push (_stream_readable.js:208:10)
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/mborins/code/node/v8.x/test/common/index.js:439:10)
    at Socket.conn.once.common.mustCall (/Users/mborins/code/node/v8.x/test/parallel/test-tls-destroy-stream.js:45:35)
    at Socket.<anonymous> (/Users/mborins/code/node/v8.x/test/common/index.js:479:15)
    at Object.onceWrapper (events.js:315:30)
    at emitOne (events.js:116:13)
    at Socket.emit (events.js:211:7)
    at addChunk (_stream_readable.js:263:12)
    at readableAddChunk (_stream_readable.js:250:11)
    at Socket.Readable.push (_stream_readable.js:208:10)
Command: out/Release/node /Users/mborins/code/node/v8.x/test/parallel/test-tls-destroy-stream.js
=== release test-wrap-js-stream-exceptions ===
Path: parallel/test-wrap-js-stream-exceptions
Mismatched noop function calls. Expected exactly 1, actual 0.
    at Object.exports.mustCall (/Users/mborins/code/node/v8.x/test/common/index.js:439:10)
    at Object.<anonymous> (/Users/mborins/code/node/v8.x/test/parallel/test-wrap-js-stream-exceptions.js:13:16)
    at Module._compile (module.js:653:30)
    at Object.Module._extensions..js (module.js:664:10)
    at Module.load (module.js:566:32)
    at tryModuleLoad (module.js:506:12)
    at Function.Module._load (module.js:498:3)
    at Function.Module.runMain (module.js:694:10)
    at startup (bootstrap_node.js:204:16)
Command: out/Release/node --expose-internals /Users/mborins/code/node/v8.x/test/parallel/test-wrap-js-stream-exceptions.js

This test ensures that a tls client socket using `StreamWrap` with `allowHalfOpen` option won't hang. PR-URL: #23866 Refs: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

When sockets of the "net" module destroyed, they will call `this._handle.close()` which will also emit EOF if not emitted before. This feature makes sockets on the other side emit "end" and "close" even though we haven't called `end()`. As `stream` of `StreamWrap` are likely to be instances of `net.Socket`, calling `destroy()` manually will avoid issues that don't properly close wrapped connections. Fixes: #14605 PR-URL: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

This test ensures that a tls client socket using `StreamWrap` with `allowHalfOpen` option won't hang. PR-URL: #23866 Refs: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

When sockets of the "net" module destroyed, they will call `this._handle.close()` which will also emit EOF if not emitted before. This feature makes sockets on the other side emit "end" and "close" even though we haven't called `end()`. As `stream` of `StreamWrap` are likely to be instances of `net.Socket`, calling `destroy()` manually will avoid issues that don't properly close wrapped connections. Fixes: #14605 PR-URL: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

This test ensures that a tls client socket using `StreamWrap` with `allowHalfOpen` option won't hang. PR-URL: #23866 Refs: #23654 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

oyyd changed the title tls: close StreamWrap and its stream correctly tls: call destroy() on StreamWrap and their streams Oct 14, 2018

addaleax approved these changes Oct 14, 2018

View reviewed changes

jasnell approved these changes Oct 14, 2018

View reviewed changes

addaleax added tls Issues and PRs related to the tls subsystem. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Oct 21, 2018

addaleax closed this Oct 21, 2018

addaleax mentioned this pull request Nov 3, 2018

test: add a test for "tls.Socket" with "StreamWrap" and "allowHalfOpen" #23866

Closed

3 tasks

MylesBorins added land-on-v10.x labels Nov 26, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

smile21 mentioned this pull request Mar 26, 2019

fix grpc bugs sofastack/sofa-rpc-node#40

Merged

tls: call `destroy()` on StreamWrap and their streams by oyyd · Pull Request #23654 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: call destroy() on StreamWrap and their streams - #23654

tls: call destroy() on StreamWrap and their streams#23654
oyyd wants to merge 1 commit into
nodejs:masterfrom
oyyd:tls-stream-wrap-close

oyyd commented Oct 14, 2018

Uh oh!

addaleax left a comment

Uh oh!

jasnell left a comment

Uh oh!

addaleax commented Oct 21, 2018

Uh oh!

oyyd commented Oct 21, 2018

Uh oh!

MylesBorins commented Nov 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tls: call `destroy()` on StreamWrap and their streams by oyyd · Pull Request #23654 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: call destroy() on StreamWrap and their streams - #23654

tls: call destroy() on StreamWrap and their streams#23654oyyd wants to merge 1 commit intonodejs:masternodejs/node:masterfrom oyyd:tls-stream-wrap-closeoyyd/node:tls-stream-wrap-closeCopy head branch name to clipboard

Conversation

oyyd commented Oct 14, 2018

Checklist

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 21, 2018

Uh oh!

oyyd commented Oct 21, 2018

Uh oh!

MylesBorins commented Nov 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tls: call destroy() on StreamWrap and their streams#23654
oyyd wants to merge 1 commit into
nodejs:masterfrom
oyyd:tls-stream-wrap-close