-
Notifications
You must be signed in to change notification settings - Fork 30k
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: make HTTP/1.0 connection test more robust #55959
Conversation
test/parallel/test-http-remove-connection-header-persists-connection.js
Outdated
Show resolved
Hide resolved
socket.write('GET / HTTP/1.0\r\n' + | ||
'Host: localhost:' + server.address().port + '\r\n' + | ||
'\r\n'); | ||
socket.resume(); // Ignore the response itself | ||
socket.end(); // we are done sending data |
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.
socket.end(); // we are done sending data |
This invalidate the test. The socket should be closed by the server.
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 only closes one end of the socket, as I understand it. But I will remove it.
test/parallel/test-http-remove-connection-header-persists-connection.js
Outdated
Show resolved
Hide resolved
test/parallel/test-http-remove-connection-header-persists-connection.js
Outdated
Show resolved
Hide resolved
b6671e5
to
33caf96
Compare
test/parallel/test-http-remove-connection-header-persists-connection.js
Outdated
Show resolved
Hide resolved
3fddc59
to
4c41b60
Compare
4c41b60
to
075cf49
Compare
Failed to start CI⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/11986219813 |
(lint failed due to unused |
Use |
075cf49
to
e078758
Compare
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55959 +/- ##
========================================
Coverage 87.99% 87.99%
========================================
Files 653 653
Lines 187852 188091 +239
Branches 35888 35945 +57
========================================
+ Hits 165295 165514 +219
- Misses 15729 15742 +13
- Partials 6828 6835 +7 |
@FliegendeWurst to actually verify that the socket is closed immediately after sending the response, I would refactor the test as follows diff --git a/test/parallel/test-http-remove-connection-header-persists-connection.js b/test/parallel/test-http-remove-connection-header-persists-connection.js
index df7e39ae94..4afb863c44 100644
--- a/test/parallel/test-http-remove-connection-header-persists-connection.js
+++ b/test/parallel/test-http-remove-connection-header-persists-connection.js
@@ -10,6 +10,13 @@ const server = http.createServer(function(request, response) {
// For HTTP/1.0, the connection should be closed after the response automatically.
response.removeHeader('connection');
+ if (request.httpVersion === '1.0') {
+ const socket = request.socket;
+ response.on('finish', common.mustCall(function() {
+ assert.ok(socket.writableEnded);
+ }));
+ }
+
response.end('beep boop\n');
});
@@ -49,10 +56,7 @@ function makeHttp10Request(cb) {
'Host: localhost:' + server.address().port + '\r\n' +
'\r\n');
socket.resume(); // Ignore the response itself
-
- setTimeout(function() {
- cb(socket);
- }, common.platformTimeout(50));
+ socket.on('close', cb);
});
}
@@ -62,9 +66,7 @@ server.listen(0, function() {
// Both HTTP/1.1 requests should have used the same socket:
assert.strictEqual(firstSocket, secondSocket);
- makeHttp10Request(function(socket) {
- // The server should have immediately closed the HTTP/1.0 socket:
- assert.strictEqual(socket.closed, true);
+ makeHttp10Request(function() {
server.close();
});
});
|
Fixes: nodejs#47200 Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
e078758
to
401f5f2
Compare
Done as suggested. I also checked the http server: the socket should be closed before our 'finish' handler is called. |
Landed in b6fe731 |
Fixes: nodejs#47200 Co-authored-by: Luigi Pinca <luigipinca@gmail.com> PR-URL: nodejs#55959 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Jason Zhang <xzha4350@gmail.com>
Fixes: #47200
This is a more robust alternative to #47316