test: fix test-http2-socket-close.js

[huseyinacacak-janea]

The client receives a response from the server and then destroys the connection after waiting 10ms. This waiting time is sometimes not enough so the connection is closed without seeing UV_EOF.

In order to close the connection gracefully, I increased the waiting time to be on the safe side.
Note that this flakiness could have been fixed using end() instead of destroy(), but I didn't want to change the test logic.

Fixes: #54819

[lpinca]

Another option without increasing the test duration could be adding a noop listener for the 'error' event on the peer where the error is emitted. We have other cases like this. What do you think?

[lpinca]

This is what I mean

diff --git a/test/parallel/test-http2-socket-close.js b/test/parallel/test-http2-socket-close.js
index 02db77bcf8..ec6c8fe410 100644
--- a/test/parallel/test-http2-socket-close.js
+++ b/test/parallel/test-http2-socket-close.js
@@ -25,6 +25,7 @@ let serverH2Session;
 
 const netServer = net.createServer((socket) => {
   serverRawSocket = socket;
+  socket.on('error', () => {});
   h2Server.emit('connection', socket);
 });
 

FWIW, I can't reproduce the issue even if serverRawSocket.destroy() is called immediately (outside of the setTimeout() callback).

[huseyinacacak-janea]

Thank you for the review.
I can easily reproduce this by using the following command:
python tools\test.py --repeat=2000 parallel/test-http2-socket-close

I've applied the above patch locally, but the test is still flaky.

[lpinca]

Does still flaky means that ECONNRESET is still unhanded? If so I wonder where that error is emitted.

[huseyinacacak-janea]

Yes, the error is the same.
From what I've seen from the logs, the following throws the exception:

node/lib/internal/stream_base_commons.js

Line 216 in 6cc4d5f

stream.destroy(new ErrnoException(nread, 'read'));

[lpinca]

Yes, but it should not crash the process if there is a listener for the 'error' event.

[lpinca]

This should work

diff --git a/test/parallel/test-http2-socket-close.js b/test/parallel/test-http2-socket-close.js
index 02db77bcf8..d88c10ae18 100644
--- a/test/parallel/test-http2-socket-close.js
+++ b/test/parallel/test-http2-socket-close.js
@@ -42,6 +42,7 @@ netServer.listen(0, common.mustCall(() => {
     rejectUnauthorized: false
   });
 
+  proxyClient.on('error', () => {});
   proxyClient.on('close', common.mustCall(() => {
     netServer.close();
   }));
@@ -51,6 +52,7 @@ netServer.listen(0, common.mustCall(() => {
     ':path': '/'
   });
 
+  req.on('error', () => {});
   req.on('response', common.mustCall((response) => {
     assert.strictEqual(response[':status'], 200);
 

[huseyinacacak-janea]

OK, this seems to have fixed the issue (it didn't fail in 10k runs). I'll update the PR accordingly. Thanks.

[mhdawson]

@huseyinacacak-janea thanks for your work on helping to make the test more reliable.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.06%. Comparing base (9db6327) to head (3331a88).
Report is 31 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #54900   +/-   ##
=======================================
  Coverage   88.05%   88.06%           
=======================================
  Files         651      651           
  Lines      183405   183409    +4     
  Branches    35822    35823    +1     
=======================================
+ Hits       161499   161511   +12     
+ Misses      15159    15152    -7     
+ Partials     6747     6746    -1     

see 27 files with indirect coverage changes

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54900
✔  Done loading data for nodejs/node/pull/54900
----------------------------------- PR info ------------------------------------
Title      test: fix test-http2-socket-close.js (#54900)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     huseyinacacak-janea:huseyin-12529-flaky-test-http2-socket-close -> nodejs:main
Labels     test, author ready, needs-ci
Commits    1
 - test: fix test-http2-socket-close.js
Committers 1
 - Hüseyin Açacak <huseyin@janeasystems.com>
PR-URL: https://github.com/nodejs/node/pull/54900
Fixes: https://github.com/nodejs/node/issues/54819
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54900
Fixes: https://github.com/nodejs/node/issues/54819
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 12 Sep 2024 12:25:49 GMT
   ✔  Approvals: 1
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/54900#pullrequestreview-2302264140
   ✘  This PR needs to wait 119 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-09-13T10:01:26Z: https://ci.nodejs.org/job/node-test-pull-request/62395/
- Querying data for job/node-test-pull-request/62395/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/10862411224

[nodejs-github-bot]

Landed in c6f514c