-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
test: fix test-http2-socket-close.js #54900
test: fix test-http2-socket-close.js #54900
Conversation
@@ -62,6 +62,6 @@ netServer.listen(0, common.mustCall(() => { | |||
setTimeout(() => { | |||
serverH2Session.close(); | |||
}, 10); | |||
}, 10); | |||
}, 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does still flaky means that ECONNRESET
is still unhanded? If so I wonder where that error is emitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it should not crash the process if there is a listener for the 'error'
event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this seems to have fixed the issue (it didn't fail in 10k runs). I'll update the PR accordingly. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huseyinacacak-janea thanks for your work on helping to make the test more reliable.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 |
d69e971
to
3331a88
Compare
This comment was marked as outdated.
This comment was marked as outdated.
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/.ncuhttps://github.com/nodejs/node/actions/runs/10862411224 |
Landed in c6f514c |
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 ofdestroy()
, but I didn't want to change the test logic.Fixes: #54819