Conversation
test/parallel/test-tls-js-stream.js
Outdated
There was a problem hiding this comment.
For reference this was added in 75930bb#diff-94e518e66d7d1b91e52af17401305431.
There was a problem hiding this comment.
It's placement was a deliberate regression test for destroy() in an SSL cb (specifically, from js code executed during a callback from an SSL C++ cb), so removing it loses the regression test. But, the general purpose of this test for js streams, not destroy-in-cb, and it looks like the two tests don't fit comfortably together anymore. It would be good to have a new test that triggers the old bug, but maybe at this point, with the bug fixed, we can afford to lose this line. If not, we likely need a new test specifcally for the destroy that doesn't depend on the rest of the test working.
test/parallel/test-tls-js-stream.js
Outdated
There was a problem hiding this comment.
I guess you don't need a common.mustCall here because the server has one on connection, and that won't occur if this doesn't run, but adding one might make less reasoning about the test flow necessary.
There was a problem hiding this comment.
Yes, I didn't add it on purpose for that reason. Will fix tomorrow.
lpinca
force-pushed
the
deflake/test-tls-js-stream
branch
from
7e57c87 to
8479ef4
Compare
`socket.destroy()` can destory the stream before the chunk to write with `socket.end()` is actually sent. Furthermore `socket.destroy()` destroys `p` and not the actual raw socket. As a result it is possible that the connection is left open. Remove `socket.destroy()` to ensure that the chunk is sent. Also use `common.mustCall()` to ensure that the `'secureConnection'` and `'secureConnect'` events are emitted exactly once. Fixes: nodejs#26938
lpinca
force-pushed
the
deflake/test-tls-js-stream
branch
from
8479ef4 to
c9e918b
Compare
|
Windows failures seem to be related. I will investigate. |
|
@Trott @sam-github PTAL. |
|
Landed in 8c4bd2a. |
`socket.destroy()` can destory the stream before the chunk to write with `socket.end()` is actually sent. Furthermore `socket.destroy()` destroys `p` and not the actual raw socket. As a result it is possible that the connection is left open. Remove `socket.destroy()` to ensure that the chunk is sent. Also use `common.mustCall()` to ensure that the `'secureConnection'` and `'secureConnect'` events are emitted exactly once. PR-URL: #27478 Fixes: #26938 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
`socket.destroy()` can destory the stream before the chunk to write with `socket.end()` is actually sent. Furthermore `socket.destroy()` destroys `p` and not the actual raw socket. As a result it is possible that the connection is left open. Remove `socket.destroy()` to ensure that the chunk is sent. Also use `common.mustCall()` to ensure that the `'secureConnection'` and `'secureConnect'` events are emitted exactly once. PR-URL: #27478 Fixes: #26938 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
5 participants
socket.destroy()can destory the stream before the chunk to writewith
socket.end()is actually sent. Furthermoresocket.destroy()destroys
pand not the actual raw socket. As a result it is possiblethat the connection is left open.
Remove
socket.destroy()to ensure that the chunk is sent. Also usecommon.mustCall()to ensure that the'secureConnection'and'secureConnect'events are emitted exactly once.Fixes: #26938
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes