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

oyyd · 2018-10-14T12:30:25Z

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

addaleax

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/

jasnell

/cc @mcollina

addaleax · 2018-10-21T06:57:43Z

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

oyyd · 2018-10-21T07:17:35Z

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2018-11-26T17:14:34Z

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 intonodejs: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

Uh oh!

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