async_wrap: add asyncReset to TLSWrap
#13092
Conversation
nodejs-github-bot
added
c++
|
CI: https://ci.nodejs.org/job/node-test-commit/9967/ |
refack
changed the title
asyncReset to TLSWrap
There was a problem hiding this comment.
Is there a reason to add this listener? If there is an error in the future I think it would be better to see that error than an AssertionError('must not throw') message.
There was a problem hiding this comment.
🤔 and put assert.ifError?
Ok.
There was a problem hiding this comment.
It will just throw by default, which will cause the test to fail.
There was a problem hiding this comment.
But then you don't get which line... And since there are two almost identical calls it's a PITA (just been there while writing this test)
There was a problem hiding this comment.
I see. I think the ifError approach is fine then.
cjihrig
left a comment
cjihrig
left a comment
There was a problem hiding this comment.
A couple of nits with the test, but mostly LGTM
There was a problem hiding this comment.
checks for the issue in -> Refs:
There was a problem hiding this comment.
Can you replace the uses of assert.ifError() with common.mustNotCall(). You can provide a custom message with the latter if you'd like.
There was a problem hiding this comment.
See #13092 (comment)
@AndreasMadsen suggested that we might want to see the actual error.
(Maybe we need mustNotErr like assert.doesNotThrow)
There was a problem hiding this comment.
OK, you might want to use assert.fail() instead. ifError() implies that there might not be an error, but in this case, we know that there is one.
There was a problem hiding this comment.
Added a factory method mustNotErr that calls assert.fail so the fail point will appear in the stack
There was a problem hiding this comment.
In a sense yes, I just wanted to be explicit about the pain point.
I could replace this with a comment.
There was a problem hiding this comment.
Couldn't you drop the try...catch completely, and if it happens to throw, it will fail the test.
There was a problem hiding this comment.
Ack. Replaced with a comment
There was a problem hiding this comment.
Sorry if my previous comment was unclear. I just meant to write this as:
Refs: #13045
There was a problem hiding this comment.
I don't understand why you need this. You can just pass assert.fail (without the parens) as the handler, everywhere you use mustNotErr().
There was a problem hiding this comment.
Tried it, then there is no frame for the line with the failing assert (Actually the factory is not a solution I need explicit (err) => assert.fail(err))
Generated error by setting port: port + 1
with (err) => assert.fail(err)
assert.js:92
throw new AssertionError({
^
AssertionError [ERR_ASSERTION]: Error: write EPROTO 101057795:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:openssl\ssl\s23_clnt.c:794:
at ClientRequest.req.on (D:\code\node-cur\test\parallel\test-async-wrap-GH13045.js:63:35)
at emitOne (events.js:115:13)
at ClientRequest.emit (events.js:210:7)
at TLSSocket.socketErrorListener (_http_client.js:397:9)
at emitOne (events.js:115:13)
at TLSSocket.emit (events.js:210:7)
at onwriteError (_stream_writable.js:359:10)
at onwrite (_stream_writable.js:377:5)
at fireErrorCallbacks (net.js:522:13)
at TLSSocket.Socket._destroy (net.js:563:3)with just assert.fail
assert.js:92
throw new AssertionError({
^
AssertionError [ERR_ASSERTION]: Error: write EPROTO 101057795:error:140770FC:SSL routines:SSL23_GET_SERVER_HELLO:unknown protocol:openssl\ssl\s23_clnt.c:794:
at emitOne (events.js:115:13)
at ClientRequest.emit (events.js:210:7)
at TLSSocket.socketErrorListener (_http_client.js:397:9)
at emitOne (events.js:115:13)
at TLSSocket.emit (events.js:210:7)
at onwriteError (_stream_writable.js:359:10)
at onwrite (_stream_writable.js:377:5)
at fireErrorCallbacks (net.js:522:13)
at TLSSocket.Socket._destroy (net.js:563:3)
at WriteWrap.afterWrite [as oncomplete] (net.js:857:10)
|
Sounds correct to me. |
|
Can we get a CITGM run? that has been the main blocker for me. |
There was a problem hiding this comment.
Isn't this more or less equivalent to just not adding an 'error' listener? Similarly with the 'error' handlers above.
There was a problem hiding this comment.
In that case, I'm -0 on it.
There was a problem hiding this comment.
Add the response handler as a second argument here for consistency?
There was a problem hiding this comment.
Ditto about switching to https.get().
There was a problem hiding this comment.
common.mustCall((req, res) => { ... }, 2) ?
There was a problem hiding this comment.
Are these last two options necessary?
There was a problem hiding this comment.
Just last one: AssertionError [ERR_ASSERTION]: Error: self signed certificate in certificate chain
There was a problem hiding this comment.
I think method: 'GET' could be dropped and .request() changed to .get() to avoid the need to req.end().
There was a problem hiding this comment.
This isn't needed, it's even the default.
|
Pre land CI:https://ci.nodejs.org/job/node-test-commit/10006/ Ping @trevnorris any comments? I would like to land this in a few hours. |
|
Landed in 6bfdeed |
|
Post land CI: https://ci.nodejs.org/job/node-test-commit/10031/ |
|
@refack Thanks for taking care of this. |
|
Thank you @refack. 🎉 |
|
This should likely be included in a larger async_wrap backport if it were to happen |
MylesBorins
added
the
baking-for-lts
MylesBorins
removed
the
baking-for-lts
9 participants
When using an Agent for HTTPS,
TLSSockets are reused and need tohave the ability to
asyncResetfrom JS.Fixes: #13045
(Not a complete solution. Does not solve custom Agent use case)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
async_wrap