fix: wake event loop when V8 posts foreground tasks from background threads#32450
fix: wake event loop when V8 posts foreground tasks from background threads#32450fraidev merged 4 commits intodenoland:mainfrom
Conversation
4e0dc76 to
bd9b6b1
Compare
bd9b6b1 to
c9647a2
Compare
ec2a726 to
3f99ebb
Compare
There was a problem hiding this comment.
This code seem unrelated to the task at hand?
There was a problem hiding this comment.
yeah totally unrelated, this file and the test was from other PR, I removed them.
libs/core/runtime/jsruntime.rs
Outdated
| std::thread::spawn(move || { | ||
| std::thread::sleep(std::time::Duration::from_millis(100)); | ||
| flag.store(false, std::sync::atomic::Ordering::Relaxed); | ||
| waker.wake_by_ref(); | ||
| }); |
There was a problem hiding this comment.
What was the reason to spawn a separate thread and sleep for 100ms? Wouldn't just calling waker.wake() here be sufficient?
There was a problem hiding this comment.
you're right, like that worked very well too. fixed!
11145fd to
271c7ae
Compare
kajukitli
left a comment
There was a problem hiding this comment.
overall direction looks right, but i think there
kajukitli
left a comment
There was a problem hiding this comment.
overall direction looks right, but i think there
kajukitli
left a comment
There was a problem hiding this comment.
real bug in the delayed-task path
DenoPlatformImpl::wake_delayed() spawns a brand new OS thread for every delayed foreground task via std::thread::spawn + sleep.
that means Atomics.waitAsync timeouts can turn into unbounded sleeping threads, because those timeouts are user-controlled. a program can create thousands of delayed tasks and now you've got thousands of sleeping OS threads. that's basically a DoS footgun in core runtime code.
this needs a shared timer queue / scheduler, not one thread per delayed task.
kajukitli
left a comment
There was a problem hiding this comment.
real bug in the delayed-task path
DenoPlatformImpl::wake_delayed() spawns a brand new OS thread for every delayed foreground task via std::thread::spawn + sleep.
that means Atomics.waitAsync timeouts can turn into unbounded sleeping threads, because those timeouts are user-controlled. a program can create thousands of delayed tasks and now you've got thousands of sleeping OS threads. that's basically a DoS footgun in core runtime code.
this needs a shared timer queue / scheduler, not one thread per delayed task.
271c7ae to
253bbf5
Compare
|
The |
Fixed! |
kajukitli
left a comment
There was a problem hiding this comment.
rechecked after the last commits — the shared timer queue fixes the big problem i called out earlier. much better than spawning one OS thread per delayed task.
lgtm now
minor note: TimerEntry ordering only compares deadline, so equal-deadline entries are considered equal in ordering even if they target different isolates. that's fine for BinaryHeap correctness here, but if you ever need stable ordering / dedupe semantics, include isolate_key too.
|
Sadly #32450 isn't fixed by this. Log shows worker |
Closes #14786
Closes #32034
Needs: denoland/rusty_v8#1924
When V8 background threads post foreground tasks (module compilation callbacks, Atomics.waitAsync timeouts, worker lifecycle events), the event loop had no way to know it should wake up and pump those tasks. This caused stalls where programs would hang until an unrelated timer or I/O event happened to fire.
The fix uses rusty_v8's new PlatformImpl trait to create a custom V8 platform (DenoPlatformImpl) that intercepts foreground task posts. When a V8 background thread posts a task for an isolate, the callback looks up that isolate's AtomicWaker in a global registry and wakes the corresponding Tokio task.