test: make HTTP/1.0 connection test more robust#55959
Conversation
| '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.
| 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.
This only closes one end of the socket, as I understand it. But I will remove it.
b6671e5 to
33caf96
Compare
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. |
Fixes: #47200
This is a more robust alternative to #47316