net: defer self.destroy calls to nextTick by anilhelvaci · Pull Request #54857 · nodejs/node

anilhelvaci · 2024-09-09T06:50:00Z

What is the problem being solved?

#48771 Reported request object returned from http.request method cannot catch error events triggered when there’s an immediate failure trying to connect to an address returned from dns lookup.

Solution

#51038 implemented changes suggested in #48771 (comment). However the #51038 couldn’t be merged due to lack of tests(#51038 (review)). In this PR, I apply the same fix but with some tests.

Testing Considerations

All process.nextTick(() => self.destroy()) are hit except one. Below is the self.destroy() call that is not hit in these tests provided:

node/lib/net.js

Line 1113 in 9404d3a

self.destroy(new ERR_SOCKET_CONNECTION_TIMEOUT());

I am not tagging this PR as "DRAFT" since the piece of code that isn't tested is for a connection timeout case.

@mcollina Please let me know if these tests are sufficient or not.

nodejs-github-bot · 2024-09-09T06:50:05Z

Review requested:

@nodejs/net

mcollina

lgtm

codecov · 2024-09-09T08:33:36Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.48%. Comparing base (4396cf2) to head (793c722).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54857      +/-   ##
==========================================
+ Coverage   88.46%   88.48%   +0.02%     
==========================================
  Files         703      703              
  Lines      207368   207540     +172     
  Branches    39983    40020      +37     
==========================================
+ Hits       183438   183639     +201     
+ Misses      15933    15871      -62     
- Partials     7997     8030      +33

Files with missing lines	Coverage Δ
lib/_http_client.js	`97.36% <100.00%> (+0.02%)`	⬆️

... and 44 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ShogunPanda

LGTM!

anonrig

nice. thank you and congrats on your first contribution!

anilhelvaci · 2024-09-10T12:07:50Z

nice. thank you and congrats on your first contribution!

Thank you, so excited to become a part of the family! @anonrig

What are the next steps then? Do I have to do anything to initiate the merge? I see that my branch is behind a few commits, should I rebase and push again?

mcollina · 2024-09-11T10:09:44Z

What are the next steps then?

Running the CI and getting it to pass. We'll take of running it, in case there are related failures it would up to you to fix them.

Do I have to do anything to initiate the merge? I see that my branch is behind a few commits, should I rebase and push again?

no need unless there are conflicts.

nodejs-github-bot · 2024-09-11T10:20:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/62310/

anilhelvaci · 2024-09-11T15:19:35Z

Hey @mcollina , thanks for your reply!

So the CI is currently failing. When I look up the details, I see two kinds of failures;

parallel.test-http-client-immidiate-error.js - The file I work on

Stack trace shows that my tests timeout for some reason.

---
duration_ms: 120011.463
exitcode: -15
severity: fail
stack: |-
  timeout
  (node:376527) internal/test/binding: These APIs are for internal testing only. Do not use them.
  (Use `node --trace-warnings ...` to show where the warning was created)
...

parallel.test-runner-watch-mode-complex

For this file, I have no idea why this is failing. Here's the stack trace;

duration_ms: 6031.101
exitcode: 1
severity: fail
stack: "\u25B6 test runner watch mode with more complex setup\n  \u2716 should run\
  \ tests when a dependency changed after a watched test file being deleted (4747.503807ms)\n\
  \    AssertionError [ERR_ASSERTION]: The input did not match the regular expression\
  \ /tests 2/. Input:\n\n    '\u2714 second test has ran (3.238638ms)\\n' +\n    \
  \  '\u2714 first test has ran (4.048804ms)\\n' +\n      '\u2714 first test has ran\
  \ (12.434145ms)\\n' +\n      '\u2139 tests 3\\n' +\n      '\u2139 suites 0\\n' +\n\
  \      '\u2139 pass 3\\n' +\n      '\u2139 fail 0\\n' +\n      '\u2139 cancelled\
  \ 0\\n' +\n      '\u2139 skipped 0\\n' +\n      '\u2139 todo 0\\n' +\n      '\u2139\
  \ duration_ms 1312.515689\\n'\n\n        at TestContext.<anonymous> (file:///home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-runner-watch-mode-complex.mjs:99:12)\n\
  \        at process.processTicksAndRejections (node:internal/process/task_queues:105:5)\n\
  \        at async Test.run (node:internal/test_runner/test:888:9)\n        at async\
  \ Promise.all (index 0)\n        at async Suite.run (node:internal/test_runner/test:1268:7)\n\
  \        at async startSubtestAfterBootstrap (node:internal/test_runner/harness:283:3)\
  \ {\n      generatedMessage: true,\n      code: 'ERR_ASSERTION',\n      actual:\
  \ '\u2714 second test has ran (3.238638ms)\\n\u2714 first test has ran (4.048804ms)\\\
  n\u2714 first test has ran (12.434145ms)\\n\u2139 tests 3\\n\u2139 suites 0\\n\u2139\
  \ pass 3\\n\u2139 fail 0\\n\u2139 cancelled 0\\n\u2139 skipped 0\\n\u2139 todo 0\\\
  n...',\n      expected: /tests 2/,\n      operator: 'match'\n    }\n\n\u25B6 test\
  \ runner watch mode with more complex setup (4776.910571ms)\n\u2139 tests 1\n\u2139\
  \ suites 1\n\u2139 pass 0\n\u2139 fail 1\n\u2139 cancelled 0\n\u2139 skipped 0\n\
  \u2139 todo 0\n\u2139 duration_ms 4833.662639\n\n\u2716 failing tests:\n\ntest at\
  \ test/parallel/test-runner-watch-mode-complex.mjs:53:3\n\u2716 should run tests\
  \ when a dependency changed after a watched test file being deleted (4747.503807ms)\n\
  \  AssertionError [ERR_ASSERTION]: The input did not match the regular expression\
  \ /tests 2/. Input:\n\n  '\u2714 second test has ran (3.238638ms)\\n' +\n    '\u2714\
  \ first test has ran (4.048804ms)\\n' +\n    '\u2714 first test has ran (12.434145ms)\\\
  n' +\n    '\u2139 tests 3\\n' +\n    '\u2139 suites 0\\n' +\n    '\u2139 pass 3\\\
  n' +\n    '\u2139 fail 0\\n' +\n    '\u2139 cancelled 0\\n' +\n    '\u2139 skipped\
  \ 0\\n' +\n    '\u2139 todo 0\\n' +\n    '\u2139 duration_ms 1312.515689\\n'\n\n\
  \      at TestContext.<anonymous> (file:///home/iojs/build/workspace/node-test-commit-linux-containered/test/parallel/test-runner-watch-mode-complex.mjs:99:12)\n\
  \      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)\n\
  \      at async Test.run (node:internal/test_runner/test:888:9)\n      at async\
  \ Promise.all (index 0)\n      at async Suite.run (node:internal/test_runner/test:1268:7)\n\
  \      at async startSubtestAfterBootstrap (node:internal/test_runner/harness:283:3)\
  \ {\n    generatedMessage: true,\n    code: 'ERR_ASSERTION',\n    actual: '\u2714\
  \ second test has ran (3.238638ms)\\n\u2714 first test has ran (4.048804ms)\\n\u2714\
  \ first test has ran (12.434145ms)\\n\u2139 tests 3\\n\u2139 suites 0\\n\u2139 pass\
  \ 3\\n\u2139 fail 0\\n\u2139 cancelled 0\\n\u2139 skipped 0\\n\u2139 todo 0\\n...',\n\
  \    expected: /tests 2/,\n    operator: 'match'\n  }"

There are other failing tests but their severity is flaky so I assume they are no problem for me.

I rebased my branch on top of the current main and make -j4 test passed. Could use any suggestions on how to debug/reproduce these failures happening in CI 🤔

aduh95 · 2024-09-13T08:38:05Z

parallel.test-http-client-immidiate-error.js - The file I work on

Stack trace shows that my tests timeout for some reason.

That's a red flag, we don't want to merge flaky tests in our codebase. A timeout probably means there's a race condition somewhere that you forgot to take into account and result in the test never exiting. To try to reproduce the flakiness locally, you can try running:

tools/test.py --repeat 9999 -t 2 test/parallel/test-http-client-immediate-error.js

Once you have a repro, you can attempt getting a fix ready.

anilhelvaci · 2024-09-13T09:09:34Z

Hey @aduh95 , thanks for the clarifying 🙏

Unfortunately the piece of code you suggested did not reproduce the problem for me. I'm on OSX, can you think of anything else that I can try?

ronag · 2024-09-18T11:54:56Z

@mcollina FYI, this adds overhead to net to fix a bug in the "legacy" HTTP client. I don't mind per se, but it's not optimal since the bug is not actually in net.

mcollina · 2024-10-15T16:24:10Z

CI is failing

anilhelvaci · 2024-10-15T19:45:44Z

Yep, it is @mcollina. Any suggestions on how to reproduce it? Any chance I can shell into the machine(or container) facing problems? Or, are there any snapshot/images I can pull into my machine and try to spin it up?

In other words, what are the usual steps that you guys take when you face a problem in CI?

mcollina · 2024-10-15T20:57:10Z

In this specific case, it seems that any Linux box on top of any virtualization software would do.

For more exotic systems @nodejs/build can provide access.

anilhelvaci · 2025-06-17T09:51:13Z

Could anyone approve the CI workflow please? I've rebased my work on top of latest main.

cc @ShogunPanda @anonrig

ShogunPanda · 2025-06-17T10:30:06Z

@anilhelvaci Done :)

anilhelvaci · 2025-09-14T16:01:08Z

Hey @mcollina @ronag @ShogunPanda ,

I am very confused because of the ping pong about where the problem lies between net and http. I will try to gather the arguments for each of them but I need some guidance which path is the best.

Argument for `http`

According @mcollina 's this comment the socket.destroy() already adds a microtick and therefore http should be the party who successfully catches the fired error event. @ronag also supports this by stating changes to net module might introduce overhead to net module. Also, since http is already a legacy module, adding this overhead just for http might not be wise. See comment

What confuses me about this?

node/lib/_http_client.js

Lines 939 to 943 in ff11b59

    
           ClientRequest.prototype.onSocket = function onSocket(socket, err) { 
        
             // TODO(ronag): Between here and onSocketNT the socket 
        
             // has no 'error' handler. 
        
             process.nextTick(onSocketNT, this, socket, err); 
        
           };

Why do we defer onSocketNT to the next tick? The TODO is pretty clear that @ronag is aware of we don't have any error listeners if there's an immediate failure. Since internalConnect is immediately invoked in lookupAndConnect (also true for internalMultipleConnect), socket.destroy()'s micro tick is still before any event listener is registered.

node/lib/net.js

Lines 1356 to 1360 in c8c6bfa

    
           defaultTriggerAsyncIdScope( 
        
             self[async_id_symbol], 
        
             internalConnect, 
        
             self, host, port, addressType, localAddress, localPort, 
        
           );

Why not just register the listeners immediately in req.onSocket and then apply the logic in onSocketNT in the next tick?

Argument for `net`

In comment, @ShogunPanda says if we fix this in net other libraries don't have to register an error handler on the socket immediately. The initial fix agreed between @mcollina and @ShogunPanda is deferring all destroy calls one more tick. See comment.

Regardless of http, not deferring calls to next tick in lookupAndConnect introduces an inconsistency. Because currently some calls are already deferred to the next tick and some are not. For example, failures in the dns lookup are deferred to the next tick exactly because of this:

node/lib/net.js

Lines 1417 to 1430 in ff11b59

    
           if (err) { 
        
             // net.createConnection() creates a net.Socket object and immediately 
        
             // calls net.Socket.connect() on it (that's us). There are no event 
        
             // listeners registered yet so defer the error event to the next tick. 
        
             process.nextTick(connectErrorNT, self, err); 
        
           } else if ((typeof ip !== 'string') || !isIP(ip)) { 
        
             err = new ERR_INVALID_IP_ADDRESS(ip); 
        
             process.nextTick(connectErrorNT, self, err); 
        
           } else if (addressType !== 4 && addressType !== 6) { 
        
             err = new ERR_INVALID_ADDRESS_FAMILY(addressType, 
        
                                                  options.host, 
        
                                                  options.port); 
        
             process.nextTick(connectErrorNT, self, err); 
        
           } else {

So why not defer internalConnect and internalConnectMultiple to the next tick too?

// lookupAndConnect
else {
        self._unrefTimer();
        defaultTriggerAsyncIdScope(
          self[async_id_symbol],
         process.nextTick,
          internalConnect, self, ip, port, addressType, localAddress, localPort,
        );
      }

// lookupAndConnectMultiple
        defaultTriggerAsyncIdScope(
          self[async_id_symbol],
          process.nextTick
          internalConnect,
          self,
          ip,
          port,
          addressType,
          localAddress,
          localPort,
        );

Conclusion

To be honest I've no strong opinion between any of the above. It's for sure I lack information about API requirements and the rationale behind most of the design decisions. In order for me to land this PR I need help on two things;

Guidance on which path the solution should take
Being able to run CI on command in case there's a problem in the CI I cannot reproduce in my local machine (See comment

Any chance I can get the help I need, please?

cc @anonrig @UlisesGascon

ronag · 2025-09-14T17:53:14Z

@anilhelvaci I'm sorry we are not responsive. Your work and engagement is appreciated and imho quite impressive. However, this is not a priority for me and also quite a complicated problem. I will try to make time for this and give some reasonable guidance. However, right now I just don't have time.

anilhelvaci · 2025-09-15T18:26:58Z

@anilhelvaci I'm sorry we are not responsive. Your work and engagement is appreciated and imho quite impressive. However, this is not a priority for me and also quite a complicated problem. I will try to make time for this and give some reasonable guidance. However, right now I just don't have time.

Thanks a lot @ronag !

Wrote tests for the suggested fix in nodejs#48771 (comment) Fixes: nodejs#48771

…ll be squashed later.

Fixes: 48771 fix: revert net.js fix: rollback net changes

anilhelvaci · 2025-09-24T05:26:09Z

I tried something in ClientRequest.prototype.onSocket at a69a7e1. I am aware that the code has room for a little more sophistication. I'm just interested in learning if the solution is directionally correct. I've verified make test passes. But CI is another story of course.

cc @mcollina @ronag

ronag · 2025-09-24T05:36:09Z

IMHO whatever solution we come up with should be changes to the http client, and net should be left untouched, unless it can be shown it's a bug in net that affects other things as well.

anilhelvaci · 2025-09-24T05:40:05Z

IMHO whatever solution we come up with should be changes to the http client, and net should be left untouched, unless it can be shown it's a bug in net that affects other things as well.

Yep @ronag , the current version leaves net untouched, solution is implemented in _http_client.js.

If we agree on the solution, I'll rephrase the PR title and commit messages. I think I'm not allowed to update the labels so I'll ask one of the reviewers to change it to http from net.

ronag · 2025-09-24T05:45:37Z

Where can I see that version?

anilhelvaci · 2025-09-24T05:49:39Z

Where can I see that version?

You can take a look at the Files Changed tab of this PR. @ronag

ronag · 2025-09-24T05:50:51Z

Now it shows. Certainly in the right direction!

ronag · 2025-09-24T05:51:22Z

lib/_http_client.js

+
+  if (!err && socket) {
+    socket.onImmediateError = function onImmediateError(err) {
+      req.immediateErr = err;


Use a private symbol

I believe this should do it 793c722. Haven't been able to find a guide on when to use a k prefix for the symbols but in order keep things consistent still used it.

Fixes: nodejs#48771

lib/_http_client.js

…rrors

github-actions · 2025-10-01T08:31:53Z

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - net: defer self.destroy calls to nextTick
   ⚠  - debug: insert console.logs for detecting where the test times out. Wi…
   ⚠  - chore: try handling the error event onSocket
   ⚠  - http: use private symbol for immediateError property
   ⚠  - chore: apply private symbol to onImmediateError and address linting e…
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/18156288164

anilhelvaci · 2025-10-07T11:13:53Z

Failed to start CI

I believe the Jenkins wasn’t kicked off because there’s a standing change request by @mcollina. I remember @joyeecheung started a job manually once. Don’t know what’s the best thing to do here. Presumably having the change request resolved is the best but there’s some debugging logs left from the previous troubleshooting efforts. I don’t want to remove them yet simply because I’m not sure what to expect from the CI run and asking people to manually trigger Jenkins jobs on behalf of me is not convenient for anybody.

cc @ronag

mcollina · 2025-10-30T12:50:42Z

@anilhelvaci you need somebody else to approve the PR to start the job.

nodejs-github-bot · 2025-10-30T13:09:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69957/

nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Sep 9, 2024

mcollina approved these changes Sep 9, 2024

View reviewed changes

mcollina requested a review from ShogunPanda September 9, 2024 07:23

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2024

jasnell approved these changes Sep 9, 2024

View reviewed changes

ShogunPanda approved these changes Sep 9, 2024

View reviewed changes

anonrig approved these changes Sep 9, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 9, 2024

This comment was marked as outdated.

Sign in to view

JungMinu approved these changes Sep 11, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 11, 2024

mertcanaltin approved these changes Oct 15, 2024

View reviewed changes

anilhelvaci force-pushed the fix-48771-net-exception-with-tests branch from 4725298 to fcc49db Compare June 17, 2025 09:47

anonrig approved these changes Jun 17, 2025

View reviewed changes

anilhelvaci added 2 commits September 23, 2025 11:59

net: defer self.destroy calls to nextTick

07034b4

Wrote tests for the suggested fix in nodejs#48771 (comment) Fixes: nodejs#48771

debug: insert console.logs for detecting where the test times out. Wi…

24ec50c

…ll be squashed later.

anilhelvaci force-pushed the fix-48771-net-exception-with-tests branch from e9ce892 to 4ab3376 Compare September 24, 2025 05:07

chore: try handling the error event onSocket

a69a7e1

Fixes: 48771 fix: revert net.js fix: rollback net changes

anilhelvaci force-pushed the fix-48771-net-exception-with-tests branch from 4ab3376 to a69a7e1 Compare September 24, 2025 05:20

anilhelvaci requested a review from mcollina September 24, 2025 05:21

ronag reviewed Sep 24, 2025

View reviewed changes

http: use private symbol for immediateError property

793c722

Fixes: nodejs#48771

ronag reviewed Sep 25, 2025

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

chore: apply private symbol to onImmediateError and address linting e…

0b8febf

…rrors

ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 1, 2025

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 Oct 1, 2025

ronag approved these changes Oct 30, 2025

View reviewed changes

ronag 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 Oct 30, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2025

Uh oh!

Conversation

What is the problem being solved?

Solution

Testing Considerations

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Argument for http

What confuses me about this?

Argument for net

Conclusion

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Argument for `http`

Argument for `net`