fix: terminate the process correctly

[tsctx]

Fixes #2413

[tsctx]

Sorry, but I could not create a unit test.

[codecov-commenter]

Codecov Report

Attention: 159 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (b126e91) 84.31%.
Report is 151 commits behind head on main.

Files	Patch %	Lines
lib/fetch/index.js	66.01%	52 Missing ⚠️
lib/fetch/util.js	31.42%	48 Missing ⚠️
lib/handler/RetryHandler.js	74.35%	30 Missing ⚠️
lib/fetch/headers.js	90.00%	6 Missing ⚠️
lib/api/readable.js	88.88%	5 Missing ⚠️
lib/compat/dispatcher-weakref.js	28.57%	5 Missing ⚠️
lib/client.js	93.75%	3 Missing ⚠️
lib/core/tree.js	94.23%	3 Missing ⚠️
lib/core/util.js	95.55%	2 Missing ⚠️
lib/fetch/request.js	90.47%	2 Missing ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2499      +/-   ##
==========================================
- Coverage   85.54%   84.31%   -1.24%     
==========================================
  Files          76       79       +3     
  Lines        6858     7211     +353     
==========================================
+ Hits         5867     6080     +213     
- Misses        991     1131     +140     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KhafraDev]

A test that just makes a fetch call in a worker sounds useful. FWIW I couldn't repro it locally either, but maybe it is in the CIs.

Is there a reason you're using the first tee'd stream instead of the second? IIRC I had issues using the first one.

[tsctx]

A test that just makes a fetch call in a worker sounds useful. FWIW I couldn't repro it locally either, but maybe it is in the CIs.

Is there a reason you're using the first tee'd stream instead of the second? IIRC I had issues using the first one.

No particular reason. What problems did you have?

[KhafraDev]

Don't recall exactly, probably related to the comment

[tsctx]

Is this? #1700

[KhafraDev]

that's why we needed the tee, not why I used the second stream. Just wondering, how do you know that this fixes the issue?

[tsctx]

I found that I could run a reproducible script of 2 issues and fix it.

[KhafraDev]

why can't that be made into a test?

[tsctx]

why can't that be made into a test?

This is because it requires access to an external server.
That test did not work when I set up a local server.

If you want the test, you need a few days. Still, you may not be able to create the test.

[KhafraDev]

as I mentioned before, it might not work locally but it might run on the CI, which is better than nothing (even if it doesn't work on the CI either)

[tsctx]

as I mentioned before, it might not work locally but it might run on the CI, which is better than nothing (even if it doesn't work on the CI either)

I can't do that right now. I will do some research

[mcollina]

lgtm

[mcollina]

that'd be a good test

[AriPerkkio]

@tsctx does this fix reproduction case from #2026 for you locally? I can still reproduce it after applying the changes from this PR.

Running stress command in second terminal makes reproducing the issue a lot easier.

[tsctx]

Hi @AriPerkkio,
Changes are added. Is this fixed?

[ronag]

@KhafraDev I'd like you approval on this before merging.

[AriPerkkio]

Hi @AriPerkkio,
Changes are added. Is this fixed?

No, #2026 still gets stuck with these changes applied locally:

ari ~/Git/undici (main) $ node repro.mjs 
Unable to terminate
...
There are 1 stuck workers
Forcing process.exit on main thread

[tsctx]

Hi @AriPerkkio,

Changes are added. Is this fixed?

No, #2026 still gets stuck with these changes applied locally:

ari ~/Git/undici (main) $ node repro.mjs 

Unable to terminate

...

There are 1 stuck workers

Forcing process.exit on main thread

Thanks for the report! I thought I had fixed it.

[KhafraDev]

@tsctx it's most likely a bug in node, not really sure how much of it can be fixed here. If you find a fix for nodejs/node#44985 it would probably help.