[v12.x] tls: reset secureConnecting on client socket

[addaleax]

secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

PR-URL: #33209
Reviewed-By: Luigi Pinca [email protected]
Reviewed-By: Sam Roberts [email protected]

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/32881/

[szmarczak]

Friendly ping :) Can this be addressed quick please? Or is there some extra work to do?

[addaleax]

@szmarczak CI wasn’t passing here, but failing with what looked like related failures. I don’t necessarily have the time to look into those, so feel free to take over :) I’ve rebased and kick off another CI, let’s see how that goes.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/33237/

[lpinca]

It seems the session is destroyed before calling session.close() but I don't why.

diff --git a/test/parallel/test-http2-connect.js b/test/parallel/test-http2-connect.js
index 9ee2e4347f..7267956222 100644
--- a/test/parallel/test-http2-connect.js
+++ b/test/parallel/test-http2-connect.js
@@ -90,7 +90,7 @@ const { connect: tlsConnect } = require('tls');
     };
 
     const onSessionConnect = (session) => {
-      session.close();
+      // session.close();
       server.close();
     };

The error is

events.js:291
      throw er; // Unhandled 'error' event
      ^

Error [ERR_HTTP2_ERROR]: Protocol error
    at Http2Session.onSessionInternalError (internal/http2/core.js:766:26)
Emitted 'error' event on ClientHttp2Session instance at:
    at emitClose (internal/http2/core.js:1020:10)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  code: 'ERR_HTTP2_ERROR',
  errno: -505
}

I wonder if this depends on another backport. There is a comment above Http2Session#onSessionInternalError() which says

// When an error occurs internally at the binding level, immediately
// destroy the session.

[lpinca]

The same error (Error [ERR_HTTP2_ERROR]: Protocol error) also happens on master but for some reasons (probably due to stream.destroy() changes) it is swallowed on master if session.close() is called.

I think we can work around the issue by adding a noop listener for the 'error' event or fixing the test so that no Protocol error is emitted.

cc: @davedoesdev

[lpinca]

I think the simplest way to fix the issue is

diff --git a/test/parallel/test-http2-connect.js b/test/parallel/test-http2-connect.js
index 9ee2e4347f..18cf07b041 100644
--- a/test/parallel/test-http2-connect.js
+++ b/test/parallel/test-http2-connect.js
@@ -96,7 +96,8 @@ const { connect: tlsConnect } = require('tls');
 
     const clientOptions = {
       port,
-      rejectUnauthorized: false
+      rejectUnauthorized: false,
+      ALPNProtocols: ['h2']
     };
     const socket = tlsConnect(clientOptions, mustCall(onSocketConnect));
   }));

[davedoesdev]

@lpinca seems a reasonable workaround.
What's the cause of the protocol error? The connection is closed before the handshakes complete?

[lpinca]

The TLS handshake completes, but without the ALPNProtocols option the Http2Session emits that protocol error.

[davedoesdev]

The TLS handshake completes, but without the ALPNProtocols option the Http2Session emits that protocol error.

Might be some state not updated in the HTTP/2 library. Out of interest, does the issue still occur with

    const onSessionConnect = (session) => process.nextTick(() => {
      session.close();
      server.close();
    });

(or setImmediate)

[lpinca]

@davedoesdev yes the test fails with a read ECONNRESET error emitted on the TLSSocket with both process.nextTick() and setImmediate() because it tries to write to a destroyed socket.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/33492/

[MylesBorins]

landed in 3b55adc...9ab53b4