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.

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

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

Closed
wants to merge 1 commit into from

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Oct 25, 2018

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

@jasnell jasnell requested a review from addaleax October 25, 2018 14:37
@jasnell jasnell added tls Issues and PRs related to the tls subsystem. stream Issues and PRs related to the stream subsystem. labels Oct 25, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Once again, nice work!

@addaleax
Copy link
Member

@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?

@oyyd
Copy link
Contributor Author

oyyd commented Oct 26, 2018

@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.

@addaleax
Copy link
Member

@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…

@oyyd
Copy link
Contributor Author

oyyd commented Oct 26, 2018

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:

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.

@danbev
Copy link
Contributor

danbev commented Oct 29, 2018

@oyyd
Copy link
Contributor Author

oyyd commented Oct 30, 2018

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:

@oyyd
Copy link
Contributor Author

oyyd commented Oct 30, 2018

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)

@addaleax
Copy link
Member

addaleax commented Nov 3, 2018

@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 :)

@oyyd
Copy link
Contributor Author

oyyd commented Nov 4, 2018

@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.

@oyyd 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
@oyyd
Copy link
Contributor Author

oyyd commented Nov 4, 2018

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.
@Trott
Copy link
Member

Trott commented Nov 6, 2018

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Still very much LGTM :)

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

addaleax commented Nov 6, 2018

Landed in aa82a21, thanks for the PR! 🎉

@addaleax addaleax closed this Nov 6, 2018
addaleax pushed a commit that referenced this pull request Nov 6, 2018
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]>
targos pushed a commit that referenced this pull request Nov 6, 2018
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]>
addaleax added a commit to addaleax/node that referenced this pull request Nov 10, 2018
Partially revert b7e6ccd
because it broke a test that was added since its last CI run.

Refs: nodejs#24075
Refs: nodejs#23866
addaleax added a commit that referenced this pull request Nov 10, 2018
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 <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
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 <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
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 <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 26, 2018
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]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
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]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
codebytere pushed a commit that referenced this pull request Dec 14, 2018
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 <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
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 <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants