denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
94.96k stars 5.27k forks source link

Don't spawn a thread when terminating a worker #14003

Open andreubotella opened 2 years ago

andreubotella commented 2 years ago

12658, which is a still unfixed V8 bug, was worked around by first signaling to the worker that its event loop should stop whenever possible, and then starting a new thread that would wait for two seconds and then kill the worker's isolate, in case the worker was stuck in a JS endless loop. I felt it necessary to perform this waiting in a new thread rather than in a task on the current thread's event loop because the current event loop might finish well before the process finished, and it was not acceptable to leave the worker unkilled in that case.

I've since realized that, since child workers don't survive termination of their parent, it's fine to kill any not-yet-killed workers when their parent's event loop is finishing. In order to do this, there should be a set in the current OpState to store the isolate handles (and has_terminated flags?) for scheduled-but-not-yet-killed workers, with a Drop impl that kills all workers still in the set. Then, WebWorkerHandle::terminate should:

13941 is a precondition for this.

bartlomieju commented 2 years ago

@andreubotella it seems we're unblocked on fixing this one now?

andreubotella commented 2 years ago

I forgot to mention it here, but we are not actually unblocked, because doing what I proposed in the OP before https://crbug.com/v8/12379 is fixed would actually bring back the crash in #12658. I didn't actually realize this when proposing this fix, but it was something that became clear as I started implementing it.