worker: fix crash when a worker joins after exit

[Qard]

If a worker has not already joined before running to completion it will join in a SetImmediateThreadsafe which could occur after the worker has already ended by other means. Mutating a JS object at that point would fail because the isolate is already disposed.

Fixes #56020 (Note that this issue is not reproducible on 22+, but is likely still technically present on later versions as it appears to be a race condition which just happened to be reproducible in the specific conditions described in the issue. The originating code has not been changed in a long time, so it's likely just a very rare condition.)

cc @addaleax I would greatly appreciate a review from you as you're the last person to touch this code...even if it's been many years now. 😅

[nodejs-github-bot]

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.49%. Comparing base (56c8360) to head (6d4a4d5).
Report is 40 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56191      +/-   ##
==========================================
+ Coverage   87.99%   88.49%   +0.50%     
==========================================
  Files         656      656              
  Lines      189103   189262     +159     
  Branches    35997    36349     +352     
==========================================
+ Hits       166402   167494    +1092     
+ Misses      15859    14976     -883     
+ Partials     6842     6792      -50     

Files with missing lines	Coverage Δ
src/node_worker.cc	84.26% <100.00%> (+0.34%)	⬆️

... and 94 files with indirect coverage changes

[mcollina]

cc @jasnell @joyeecheung

[addaleax]

Yeah, I don't think this is the right fix here ... according to the stack trace in #56020, this crash occurs while the parent thread is still running its event loop, so I'm not sure why the Isolate is already disposed? This is happening in the parent thread, not the child whose thread exited

[mcollina]

lgtm

[Qard]

@addaleax It's happening while the parent thread is alive, but it's the child thread that it's happening to because it's scheduling the SetImmediateThreadSafe from the StartThread of the worker after the Run completes. The worker completes its Run before main exits, schedules an immediate, and then seemingly is having its exit trigger before the immediate which results in the worker already having disposed its isolate by the time it reaches that immediate which attempts to join the thread.

[addaleax]

@Qard Yeah, but the thing here is that the scheduled immediate runs on the parent thread, and it's accessing the parent Isolate, not the worker Isolate, so that should be fine?

Anyway, the current fix here does look good to me

(Oh, and: It looks like the Isolate here is terminating, i.e. TerminateExecution() has been called, and not actually been disposed of?)

[Qard]

I don't know. Just going by the logs that said "isolate disposed" in it. 🤷

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 586814b