Skip to content

Conversation

@FliegendeWurst
Copy link
Contributor

Fixes: #47200

This is a more robust alternative to #47316

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 22, 2024
'Host: localhost:' + server.address().port + '\r\n' +
'\r\n');
socket.resume(); // Ignore the response itself
socket.end(); // we are done sending data
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
socket.end(); // we are done sending data

This invalidate the test. The socket should be closed by the server.

Copy link
Contributor Author

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.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 23, 2024
@github-actions
Copy link
Contributor

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/11986219813

@FliegendeWurst
Copy link
Contributor Author

(lint failed due to unused common)

@lpinca
Copy link
Member

lpinca commented Nov 23, 2024

Use require('../common');. Do not remove it completely, it is mandatory.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 23, 2024
@nodejs-github-bot
Copy link
Collaborator

@FliegendeWurst
Copy link
Contributor Author

parallel.test-http-server-headers-timeout-keepalive failed. I don't see how it relates to this change.

@codecov
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.99%. Comparing base (3178a76) to head (401f5f2).
Report is 560 commits behind head on main.

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     

see 29 files with indirect coverage changes

@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Nov 23, 2024

@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();
       });
     });

@FliegendeWurst
Copy link
Contributor Author

Done as suggested. I also checked the http server: the socket should be closed before our 'finish' handler is called.

@lpinca lpinca added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Nov 23, 2024
@aduh95 aduh95 added backported-to-v20.x PRs backported to the v20.x-staging branch. and removed lts-watch-v20.x PRs that may need to be released in v20.x labels Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported-to-v20.x PRs backported to the v20.x-staging branch. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Flaky Test parallel/test-http-remove-connection-header-persists-connection

6 participants