tls: TLSSocket emits 'error' on handshake failure #9742
Conversation
Removes branch that would make TLSSocket emit '_tlsError' event if error occured on handshake and control was not released, as it was never happening. Added test for tls.Server to ensure it still emits 'tlsClientError' as expected. Note that 'tlsClientError' does not exist in the v4.x branch so this back-port emits 'clientError' instead. See also pull request nodejs#4557. Fixes: nodejs#8803 PR-URL: nodejs#8805 Refs: nodejs#4557 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com>
|
Running CI one more time: https://ci.nodejs.org/job/node-test-pull-request/5387/ |
MylesBorins
force-pushed
the
v4.x-staging
branch
from
d860f6e
to
c77ba8c
Compare
MylesBorins
force-pushed
the
v4.x-staging
branch
from
f5c57c7
to
735119c
Compare
MylesBorins
force-pushed
the
v4.x-staging
branch
from
bba3dcf
to
6fa3c73
Compare
MylesBorins
force-pushed
the
v4.x-staging
branch
2 times, most recently
from
004f6b0
to
dcbc1b4
Compare
| @@ -0,0 +1,36 @@ | |||
| 'use strict'; | |||
| const common = require('../common'); | |||
|
|
|||
There was a problem hiding this comment.
missing test description: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#lines-4-5
| } | ||
| const tls = require('tls'); | ||
| const net = require('net'); | ||
| const assert = require('assert'); |
| const net = require('net'); | ||
| const assert = require('assert'); | ||
|
|
||
| const bonkers = Buffer.alloc(1024, 42); |
There was a problem hiding this comment.
maybe a comment, "Construct a buffer that is not a valid TLS handshake.", or just change var name -
s/bonkers/invalidTlsHandshake/?
| setTimeout(function() { | ||
| server.close(); | ||
|
|
||
| assert.ok(clientErrorEmited, 'clientError should be emited'); |
| @@ -0,0 +1,38 @@ | |||
| 'use strict'; | |||
| const common = require('../common'); | |||
|
|
|||
|
|
||
| const server = net.createServer(function(c) { | ||
| setTimeout(function() { | ||
| const s = new tls.TLSSocket(c, { |
There was a problem hiding this comment.
this version of the test exists because the error handling code paths under test differ in behaviour depending on how the server is created, or just to be very thorough?
|
@sam-github It's a back-port of an already merged pull request, I'm not going to make big changes to it. If you want those changes in, apply them to master first. |
|
@bnoordhuis any chance you could prefix a |
|
I can do that but it's right under the PR title:
|
|
landed in b6941b0 |
Note that 'tlsClientError' does not exist in the v4.x branch so this back-port emits 'clientError' instead. See also pull request #4557.
Original pull request: #8805
CI: https://ci.nodejs.org/job/node-test-pull-request/4942/