Skip to content

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

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.

Sign up for GitHub

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

Merged
nodejs-github-bot merged 5 commits into from
Feb 25, 2025
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b41341
test: resolve race condition in test-net-write-fully-async-*
mcollina Feb 13, 2025
a3c5d6d
fixup
mcollina Feb 13, 2025
15c1690
fixup: switch to .end()
mcollina Feb 15, 2025
94596f7
fixup
mcollina Feb 15, 2025
2f9f49c
Update test-net-write-fully-async-buffer.js
mcollina Feb 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions test/parallel/test-net-write-fully-async-buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const net = require('net');
const data = Buffer.alloc(1000000);

const server = net.createServer(common.mustCall(function(conn) {
// Communicate to the client that the server is ready to receive data
conn.write('start');
conn.resume();
})).listen(0, common.mustCall(function() {
const conn = net.createConnection(this.address().port, common.mustCall(() => {
Expand All @@ -27,8 +29,13 @@ const server = net.createServer(common.mustCall(function(conn) {
// The buffer allocated above should still be alive.
}

conn.on('drain', writeLoop);

writeLoop();
// Wait for the server to be ready to receive data
// otherwise the test might be flaky as the mustCall above
// might not be invoked.
conn.once('data', common.mustCall(() => {
conn.on('drain', writeLoop);
conn.resume();
writeLoop();
}));
}));
}));
11 changes: 10 additions & 1 deletion test/parallel/test-net-write-fully-async-hex-string.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ const net = require('net');
const data = Buffer.alloc(1000000).toString('hex');

const server = net.createServer(common.mustCall(function(conn) {
// Communicate to the client that the server is ready to receive data
conn.write('start');
conn.resume();
})).listen(0, common.mustCall(function() {
const conn = net.createConnection(this.address().port, common.mustCall(() => {
Expand All @@ -27,6 +29,13 @@ const server = net.createServer(common.mustCall(function(conn) {

conn.on('drain', writeLoop);

writeLoop();
// Wait for the server to be ready to receive data
// otherwise the test might be flaky as the mustCall above
// might not be invoked.
conn.once('data', common.mustCall(() => {

@lpinca lpinca Feb 13, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mcollina mcollina Feb 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lpinca Feb 13, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mcollina mcollina Feb 13, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lpinca Feb 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lpinca lpinca Feb 15, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mcollina Feb 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lpinca Feb 15, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mcollina Feb 15, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@lpinca lpinca Feb 15, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

conn.on('drain', writeLoop);
conn.resume();
writeLoop();
}));
}));
}));