tls.connect() emitting twice an error event

[user447463]

With certain hosts (and the particular cipher settings used below) tls.connect() appears to emit an error event twice.

var host='85.158.224.100';

var options = { rejectUnauthorized: false,
    servername: host,
    ciphers: 'EXPORT' };

require('tls').connect(443, host, options, function() {
    console.log('success');
}).on('error', function(error) {
    console.log(error);
    console.log();
});

Actual Result

{ [Error: socket hang up] code: 'ECONNRESET' }

[Error: 101057795:error:14094410:SSL routines:SSL3_READ_BYTES:sslv3 alert handsh
ake failure:openssl\ssl\s3_pkt.c:1293:SSL alert number 40
101057795:error:140780E5:SSL routines:SSL23_READ:ssl handshake failure:openssl\s
sl\s23_lib.c:138:
]

Expected Result

[Error: 101057795:error:14094410:SSL routines:SSL3_READ_BYTES:sslv3 alert handsh
ake failure:openssl\ssl\s3_pkt.c:1293:SSL alert number 40
101057795:error:140780E5:SSL routines:SSL23_READ:ssl handshake failure:openssl\s
sl\s23_lib.c:138:
]

Quick assumption would be it has something to do with SNI, as it does not occur if servername is not set.

[user447463]

Update: It is not necessarily SNI related, as it also happens without servername being given.

var host='204.11.109.195';

var options = { rejectUnauthorized: false,
    ciphers: 'EXPORT' };

require('tls').connect(443, host, options, function() {
    console.log('success');
}).on('error', function(error) {
    console.log(error);
    console.log();
});

[skenqbx]

The socket hangup is only emitted when socket._hadError === false and the ssl error is only emitted when socket._writableState.errorEmitted === false. Therefore both get emitted.

The following patch lets onHangUp() check socket._writableState.errorEmitted in the next tick to ensure the ssl error is emitted, since the socket hangup does have less useful information.

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index fcc216b..5f93270 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -947,10 +947,17 @@ exports.connect = function(/* [port, host], options, cb */) {
     // NOTE: This logic is shared with _http_client.js
     if (!socket._hadError) {
       socket._hadError = true;
-      var error = new Error('socket hang up');
-      error.code = 'ECONNRESET';
-      socket.destroy();
-      socket.emit('error', error);
+
+      setImmediate(function() {
+        if (socket._writableState.errorEmitted)
+          return;
+        socket._writableState.errorEmitted = true;
+
+        var error = new Error('socket hang up');
+        error.code = 'ECONNRESET';
+        socket.destroy();
+        socket.emit('error', error);
+      });
     }
   }
   socket.once('end', onHangUp);

Another issue arose while checking on('close', function(hadError) {}); hadError is always false. Not sure why yet.

[Fishrock123]

cc @indutny?

[indutny]

Looking.

[indutny]

@skenqbx this change looks good to me. May I ask you to send a PR for it? I'll send a PR for fixing the 'close' event's argument in TLS.

Many thanks!

[indutny]

@skenqbx here it is #1711

[skenqbx]

Will do, give me until end of the week.

[indutny]

Errors are now properly reported as of 80342f6

[silverwind]

sorry, didn't see there's still a task pending here at first.

[skenqbx]

The double emission of errors seems to be handled by #1711.

[skenqbx]

@silverwind This can be closed now, thank you.