test: resolve race condition in test-net-write-fully-async-*

[mcollina]

No description provided.

[mcollina]

This removes the flakiness on my Mac OS X machine.

[lpinca]

I don't understand. The connection was just established, why we need to wait for data from the server?

[mcollina]

Because the active while loop above was starving the event loop to the point that the server was closed before it had the chance to run the JS code in the listener.

[lpinca]

Sorry but I still don't understand, what listener?

[mcollina]

The server listener was never executed in some cases because the server was shut down early in https://github.com/nodejs/node/pull/57022/files#diff-b2324542b7f8149fd690cc0f9c263464d266e54d4e6b7740a241ab107e662ed1R21.

This test failed 1/10000 on my mac before this.

[lpinca]

That's odd, the server is closed after 20 'drain' events. Are you sure that the connection is established when the test fails?

[lpinca]

I can reproduce only with this

diff --git a/test/parallel/test-net-write-fully-async-buffer.js b/test/parallel/test-net-write-fully-async-buffer.js
index 3a3426ba70..6f6123731c 100644
--- a/test/parallel/test-net-write-fully-async-buffer.js
+++ b/test/parallel/test-net-write-fully-async-buffer.js
@@ -16,7 +16,7 @@ const server = net.createServer(common.mustCall(function(conn) {
     let count = 0;
 
     function writeLoop() {
-      if (count++ === 200) {
+      if (count++ === 1) {
         conn.destroy();
         server.close();
         return;

but that is expected (1 'drain' event vs 200).

Can you please also apply the patch from #57022 (comment)? I get some 'read ECONNRESET' errors without it when testing

diff --git a/test/parallel/test-net-write-fully-async-buffer.js b/test/parallel/test-net-write-fully-async-buffer.js
index 3a3426ba70..e3652dcfcc 100644
--- a/test/parallel/test-net-write-fully-async-buffer.js
+++ b/test/parallel/test-net-write-fully-async-buffer.js
@@ -16,8 +16,8 @@ const server = net.createServer(common.mustCall(function(conn) {
     let count = 0;
 
     function writeLoop() {
-      if (count++ === 200) {
-        conn.destroy();
+      if (count++ === 1) {
+        conn.end();
         server.close();
         return;
       }

which is also expected.

[mcollina]

I'm sorry I'm lost what patch do you want me to apply or what is wrong with the current state of the PR.

[lpinca]

This #57022 (comment), move server.close() under conn.resume(). After the change from conn.destroy() to conn.end() an ECONNRESET error can be emitted on the client if the same issue we are discussing occurs.

[mcollina]

Done.

[lpinca]

I'm still puzzled as to how this problem could occur, but now the test is better regardless.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.10%. Comparing base (2bd5694) to head (2f9f49c).
Report is 190 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57022      +/-   ##
==========================================
- Coverage   89.16%   89.10%   -0.07%     
==========================================
  Files         665      665              
  Lines      192602   193203     +601     
  Branches    37050    37221     +171     
==========================================
+ Hits       171732   172147     +415     
- Misses      13678    13769      +91     
- Partials     7192     7287      +95     

see 93 files with indirect coverage changes

[lpinca]

Changes are ok, but the more I think about it, the more it does not make sense. The client was writing at least 200 MB of data and was emitting 200 'drain' events with data not flowing on the other end (socket.resume() never called). This seems more a bug than a race condition in the test ¯_(ツ)_/¯.

[mcollina]

Seems to be blocked by nodejs/build#4022.

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 9840ca3