test: add a test for "tls.Socket" with "StreamWrap" and "allowHalfOpen" by oyyd · Pull Request #23866 · nodejs/node · GitHub

A tls client socket using StreamWrap with allowHalfOpen option might hang instead of exiting automatically because the 'drain' event may not be emitted. This commit corrects it by making StreamWrap try doShutdown in finishWrite.

Before this commit, running the following test will make processes hang.

This test ensures that a tls client socket using StreamWrap with allowHalfOpen option won't hang.

Refs: #23654

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Once again, nice work!

@oyyd Actually … do you know why there are a write request and a shutdown request simultaneously in that code? In net.js, shutdownSocket() is only triggered in _final(); and I thought that always happened after all writes are finished… is that wrong?

@addaleax this.finishWrite() and this.finishShutdown() inside of doClose() are likely to be no-ops. But in theory, there are some places doing handle.close() outside of _final(), such as this one:

node/lib/net.js

Line 1236 in 6223236

handle.close();

As handle.close() trigger doClose() and some of them are outside of _final(), it looks possible that this.finishWrite() and this.finishShutdown() would take effect.

But, again, handle.close() outside of _final() never get called when wrapping net.Socket in StreamWrap. We don't use StreamWrap for server sockets and StreamWrap is even not a public API in our docs. At least, I tried hard but failed to create a possible scenario and removing them didn't break any tests on my Mac.

Maybe some of our old code used them somehow, but IDK. The usage of StreamWrap in http2 is similar.

If you are suggesting removing them, I would agree with that.

@oyyd As you say, the line you’re pointing to is for server sockets, so I don’t think that’s an issue…

The thing is, generally there should only be 1 ShutdownWrap or 1 WriteWrap be active for a certain stream, ever… it’s okay that doClose() can be called at any time, but if I understand correctly, this PR fixes the situation when there is a ShutdownWrap and a WriteWrap? And I’m wondering why that situation occurrs in the first place…

generally there should only be 1 ShutdownWrap or 1 WriteWrap be active for a certain stream

I agree.

this PR fixes the situation when there is a ShutdownWrap and a WriteWrap?

I'm not totally sure but I guess it doesn't, or my intention is different. My original intention is that I noticed that some stream won't emit drain(or they emit earlier than we bind to drain) so that we won't call this.stream.end() correctly:

node/lib/internal/wrap_js_stream.js

Lines 118 to 119 in 6223236

    
           if (this[kCurrentWriteRequest] !== null) 
        
             return this.once('drain', () => this.doShutdown(req));

But it works well because when a ReadableStream ends, they call end() on WritableStream, except that this.allowHalfOpen is true. And calling end() later than expected might cause other issues.

For those writing operations don't emit drain, I try to call kDoShutdown after WriteReq so that the streams will end correctly.

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

Test failed: parallel/test-https-host-headers

log

test https server listening on port 41235
/home/iojs/build/workspace/node-test-commit-arm/nodes/centos7-arm64-gcc6/test/parallel/test-https-host-headers.js:31
throw er;
^

Error: 4395995780336:error:1408F119:SSL routines:ssl3_get_record:decryption failed or bad record mac:../deps/openssl/openssl/ssl/record/ssl3_record.c:469:

parallel/test-https-host-headers seems to be flaky as it could also fail before this commit and the error logs are just like other flaky tls tests (like: #23913)

@oyyd Not to spoil this for you, but … the test file here seems to be passing on Node 11.0.0 (but not 10.13.0) – bisecting actually points to your #23654 as the fix :)

@addaleax Oops, I guess I was taking Node 10.x as my "control group" by mistake.. But I still have some doubt about the reason. Let me dig into this a bit.

Not to spoil this for you, but … the test file here seems to be passing on Node 11.0.0 (but not 10.13.0) – bisecting actually points to your #23654 as the fix :)

Yes. Therefore I prefer not to modify the code if we couldn't find clear goodness it could bring.

But I think it's fine to keep the test for #23654. I have modified the title and description of this PR.

BTW, another place that is confusing to me is that if we don't call tlsSocket.end(), i.e. comment this line:

https://github.com/nodejs/node/blob/9fa47cf6d3b059b1ac612b5f8ec4cd96ba6d4ad2/test/parallel/test-tls-net-socket-keepalive.js#L25

Then the sockets won't hang anymore on Node 10.13.0. This needs more investigation but we could discuss it elsewhere.

This test ensures that a tls client socket using `StreamWrap` with `allowHalfOpen` option won't hang.

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

Still very much LGTM :)

Landed in aa82a21, thanks for the PR! 🎉

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>

Partially revert b7e6ccd because it broke a test that was added since its last CI run. Refs: nodejs#24075 Refs: nodejs#23866

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>

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>

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>

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>

jasnell requested a review from addaleax October 25, 2018 14:37

jasnell added tls Issues and PRs related to the tls subsystem. stream Issues and PRs related to the stream subsystem. labels Oct 25, 2018

addaleax approved these changes Oct 25, 2018

View reviewed changes

jasnell approved these changes Oct 29, 2018

View reviewed changes

oyyd force-pushed the tls-improve branch from f8507ae to 9fa47cf Compare November 4, 2018 15:39

oyyd changed the title tls: revamp StreamWrap to avoid some edge cases test: add a test for "tls.Socket" with "StreamWrap" and "allowHalfOpen" Nov 4, 2018

test: add a test for tls.Socket with allowHalfOpen

0f4ddef

This test ensures that a tls client socket using `StreamWrap` with `allowHalfOpen` option won't hang.

oyyd force-pushed the tls-improve branch from 9fa47cf to 0f4ddef Compare November 5, 2018 02:31

addaleax approved these changes Nov 6, 2018

View reviewed changes

addaleax removed the stream Issues and PRs related to the stream subsystem. label Nov 6, 2018

addaleax closed this Nov 6, 2018

addaleax added a commit to addaleax/node that referenced this pull request Nov 10, 2018

net: partially revert "simplify Socket.prototype._final"

ac1f56c

Partially revert b7e6ccd because it broke a test that was added since its last CI run. Refs: nodejs#24075 Refs: nodejs#23866

addaleax mentioned this pull request Nov 10, 2018

net: partially revert "simplify Socket.prototype._final" #24288

Closed

2 tasks

BridgeAR mentioned this pull request Nov 14, 2018

Release proposal: v11.2 #24350

Merged

MylesBorins added the land-on-v10.x label 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

test: add a test for "tls.Socket" with "StreamWrap" and "allowHalfOpen" by oyyd · Pull Request #23866 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

test: add a test for "tls.Socket" with "StreamWrap" and "allowHalfOpen"#23866oyyd wants to merge 1 commit intonodejs:masternodejs/node:masterfrom oyyd:tls-improveoyyd/node:tls-improveCopy head branch name to clipboard

Conversation

oyyd commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 25, 2018

Uh oh!

oyyd commented Oct 26, 2018

Uh oh!

addaleax commented Oct 26, 2018

Uh oh!

oyyd commented Oct 26, 2018

Uh oh!

danbev commented Oct 29, 2018

Uh oh!

oyyd commented Oct 30, 2018

Uh oh!

oyyd commented Oct 30, 2018

Uh oh!

addaleax commented Nov 3, 2018

Uh oh!

oyyd commented Nov 4, 2018

Uh oh!

oyyd commented Nov 4, 2018

Uh oh!

Trott commented Nov 6, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Nov 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

test: add a test for "tls.Socket" with "StreamWrap" and "allowHalfOpen"#23866
oyyd wants to merge 1 commit into
nodejs:masterfrom
oyyd:tls-improve

oyyd commented Oct 25, 2018 •
edited
Loading