Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parallel.test-tls-socket-close is flaky #49902

Closed
anonrig opened this issue Sep 27, 2023 · 6 comments · Fixed by #49980 or #53019
Closed

parallel.test-tls-socket-close is flaky #49902

anonrig opened this issue Sep 27, 2023 · 6 comments · Fixed by #49980 or #53019
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI.

Comments

@anonrig
Copy link
Member

anonrig commented Sep 27, 2023

Test

parallel.test-tls-socket-close

Platform

No response

Console output

not ok 2868 parallel/test-tls-socket-close
  ---
  duration_ms: 220.38800
  severity: fail
  exitcode: 1
  stack: |-
    node:assert:125
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
    
    false !== true
    
        at Immediate._onImmediate (/home/iojs/build/workspace/node-test-commit-linuxone/test/parallel/test-tls-socket-close.js:49:18)
        at process.processImmediate (node:internal/timers:478:21) {
      generatedMessage: true,
      code: 'ERR_ASSERTION',
      actual: false,
      expected: true,
      operator: 'strictEqual'
    }
    
    Node.js v21.0.0-pre
  ...

Build links

Additional information

No response

@anonrig anonrig added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Sep 27, 2023
@lpinca
Copy link
Member

lpinca commented Sep 27, 2023

It seems #49575 did not fully fix it.

@joyeecheung
Copy link
Member

I wonder if we really need to check the client socket in the immediate, perhaps it's already enough, say, checking it on process exit?

@joyeecheung
Copy link
Member

joyeecheung commented Sep 28, 2023

Not quite sure what the test was going for, perhaps we could also switch to process.nextTick...

EDIT: nope probably not useful

@joyeecheung
Copy link
Member

joyeecheung commented Sep 28, 2023

A dumb idea: maybe we can keep checking if the client socket is destroyed in setImmediate, when it's destroyed, log the count of immediates run, assert that the count should be no more then 1, then run a stress test on it to see if it goes beyond one and if it does, what exactly it is. I am fairly certain though that there probably is just no way you can be sure about this count, there's nothing in the API contract for you to tell how long you need to wait after the server socket is destroyed until the client socket is destroyed. So maybe again it's already enough to check on process exit..

@lpinca
Copy link
Member

lpinca commented Sep 29, 2023

Before Joyee's comments I tried to remove setImmediate() and rely on the 'close' events. If the 'close' event is emitted, then socket.destroy is true.

diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js
index 70af760d53..e661da94f0 100644
--- a/test/parallel/test-tls-socket-close.js
+++ b/test/parallel/test-tls-socket-close.js
@@ -6,6 +6,7 @@ if (!common.hasCrypto)
 const assert = require('assert');
 const tls = require('tls');
 const net = require('net');
+const Countdown = require('../common/countdown');
 const fixtures = require('../common/fixtures');
 
 const key = fixtures.readKey('agent2-key.pem');
@@ -27,6 +28,10 @@ const netServer = net.createServer((socket) => {
   connectClient(netServer);
 }));
 
+const countdown = new Countdown(2, () => {
+  netServer.close();
+});
+
 // A client that connects, sends one message, and closes the raw connection:
 function connectClient(server) {
   const clientTlsSocket = tls.connect({
@@ -42,17 +47,12 @@ function connectClient(server) {
 
       netSocket.destroy();
 
-      setImmediate(() => {
-        assert.strictEqual(netSocket.destroyed, true);
-
-        setImmediate(() => {
-          assert.strictEqual(clientTlsSocket.destroyed, true);
-          assert.strictEqual(serverTlsSocket.destroyed, true);
-
-          tlsServer.close();
-          netServer.close();
-        });
-      });
+      clientTlsSocket.on('close', dec);
+      serverTlsSocket.on('close', dec);
     }));
   }));
 }
+
+function dec() {
+  countdown.dec();
+}

However the 'close' event is not emitted by serverTlsSocket. I'm not sure if this is intentional. Older versions of Node.js have the same behavior but I find it surprising.

lpinca added a commit to lpinca/node that referenced this issue Sep 30, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Fixes: nodejs#49902
lpinca added a commit to lpinca/node that referenced this issue Sep 30, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: nodejs#49902 (comment)
Fixes: nodejs#49902
lpinca added a commit to lpinca/node that referenced this issue Sep 30, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: nodejs@048e0bec5147
Refs: nodejs#49902 (comment)
Fixes: nodejs#49902
lpinca added a commit to lpinca/node that referenced this issue Sep 30, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: nodejs@048e0bec5147
Refs: nodejs#49902 (comment)
Fixes: nodejs#49902
nodejs-github-bot pushed a commit that referenced this issue Oct 4, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: 048e0bec5147
Refs: #49902 (comment)
Fixes: #49902
PR-URL: #49980
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@anonrig anonrig reopened this Oct 18, 2023
@anonrig anonrig closed this as completed Oct 18, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this issue Nov 1, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: nodejs@048e0bec5147
Refs: nodejs#49902 (comment)
Fixes: nodejs#49902
PR-URL: nodejs#49980
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this issue Nov 11, 2023
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: 048e0bec5147
Refs: #49902 (comment)
Fixes: #49902
PR-URL: #49980
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
debadree25 pushed a commit to debadree25/node that referenced this issue Apr 15, 2024
Ensure that the `'close'` event is emitted on a `TLSSocket` when it is
created from an existing raw `net.Socket` and the raw socket is not
closed cleanly (destroyed).

Refs: nodejs@048e0bec5147
Refs: nodejs#49902 (comment)
Fixes: nodejs#49902
PR-URL: nodejs#49980
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos reopened this May 15, 2024
@targos
Copy link
Member

targos commented May 15, 2024

Still flaky:

https://ci.nodejs.org/job/node-test-commit-linux-containered/43411/nodes=ubuntu2204_sharedlibs_smallicu_x64/console

09:11:31 not ok 2947 parallel/test-tls-socket-close
09:11:31   ---
09:11:31   duration_ms: 383.31800
09:11:31   severity: fail
09:11:31   exitcode: 1
09:11:31   stack: |-
09:11:31     node:assert:408
09:11:31         throw err;
09:11:31         ^
09:11:31     
09:11:31     AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
09:11:31     
09:11:31       assert(serverTlsSocket)
09:11:31     
09:11:31         at Socket.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-tls-socket-close.js:51:7)
09:11:31         at Socket.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/test/common/index.js:470:15)
09:11:31         at Object.onceWrapper (node:events:634:28)
09:11:31         at Socket.emit (node:events:520:28)
09:11:31         at Socket._onTimeout (node:net:590:8)
09:11:31         at listOnTimeout (node:internal/timers:573:17)
09:11:31         at process.processTimers (node:internal/timers:514:7) {
09:11:31       generatedMessage: true,
09:11:31       code: 'ERR_ASSERTION',
09:11:31       actual: undefined,
09:11:31       expected: true,
09:11:31       operator: '=='
09:11:31     }
09:11:31     
09:11:31     Node.js v22.2.0
09:11:31   ...

lpinca added a commit to lpinca/node that referenced this issue May 16, 2024
lpinca added a commit to lpinca/node that referenced this issue May 16, 2024
nodejs-github-bot pushed a commit that referenced this issue May 23, 2024
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Jun 1, 2024
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this issue Jun 20, 2024
Fixes: nodejs#49902
PR-URL: nodejs#53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this issue Jun 22, 2024
Fixes: nodejs#49902
PR-URL: nodejs#53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
marco-ippolito pushed a commit that referenced this issue Jul 19, 2024
Fixes: #49902
PR-URL: #53019
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI.
Projects
None yet
4 participants