feat: add PlatformImpl trait for custom platform callbacks

[fraidev]

Add a trait-based API (following the V8InspectorClientImpl pattern) that lets embedders hook into platform virtual methods. A CustomPlatform C++ class subclasses DefaultPlatform and delegates on_foreground_task_posted and on_isolate_shutdown to a Rust PlatformImpl trait object.

This allows embedders like Deno to receive notifications when V8 background threads post foreground tasks, enabling event-driven wakeups instead of polling.

[CLAassistant]

All committers have signed the CLA.

[kajukitli]

few things:

1. Thread leak in NotifyAfterDelay - spawning and detaching a thread for every delayed task is bad. if you post 1000 delayed tasks you're spawning 1000 threads. this will OOM or hit thread limits on busy apps. consider using a single dedicated timer thread with a priority queue, or just call the callback immediately and let the embedder handle timing.

2. Race in NotifyingTaskRunner - the callback is invoked after posting the task to the wrapped runner. if another thread pumps the message loop immediately, it could execute the task before your callback fires. depending on what the callback does (wake event loop?), this might be fine, but worth documenting the ordering.

3. Missing UnregisterIsolate equivalent - NotifyIsolateShutdown cleans up runners, but DefaultPlatform also has UnregisterIsolate(). should you override that too? or is shutdown sufficient?

4. int in rust ffi - the extern decl uses int but rust ffi uses c_int. should be std::os::raw::c_int or i32 explicitly.

5. gen file committed - is gen/src_binding_release_aarch64-apple-darwin.rs supposed to be committed? 946 lines of generated bindgen output seems like it should be in gitignore or generated at build time.

the overall design makes sense for waking event loops when background work completes. just needs the thread spawning fixed before this is viable for production.

[devsnek]

by trait i meant being able to build your own platform by providing a trait that represents the various virtual c++ methods. so it wouldn't be specifically a "notifying platform". again you can refer to the inspector code to see what this sort of pattern looks like.

[fraidev]

makes sense! I created a PlatformImpl, wdyt?

[devsnek]

the trait methods should just be the the virtual c++ methods that are being overridden. there should not be any onForegroundTaskPosted or anything extra like that.

[bartlomieju]

Code Review

Overview

Adds a trait-based PlatformImpl API that wraps V8's DefaultPlatform, intercepting foreground task runner methods and NotifyIsolateShutdown to notify Rust embedders. Follows the existing V8InspectorClientImpl pattern with double-boxed trait objects passed as void* context through C++ FFI.

---

Issues & Suggestions

1. NotifyIsolateShutdown is not override (bug)

In CustomPlatform::NotifyIsolateShutdown, the method is missing the override keyword. This means if the base class signature changes or if V8 calls it through the base pointer, the custom implementation won't be dispatched — it just shadows the base method.

// Current:
void NotifyIsolateShutdown(v8::Isolate* isolate) {
// Should be:
void NotifyIsolateShutdown(v8::Isolate* isolate) override {

This is a correctness bug — without override, V8 will call DefaultPlatform::NotifyIsolateShutdown directly and the Rust callback will never fire.

2. Panic safety in FFI callbacks

All the extern "C" Rust callbacks (e.g. v8__Platform__CustomPlatform__BASE__PostTask) call trait methods that could panic. Unwinding across an FFI boundary is undefined behavior. These should be wrapped in std::panic::catch_unwind or at minimum documented that implementations must not panic. The inspector client callbacks have the same pattern, but this is worth noting since post_task can be called from any V8 background thread.

3. Raw pointer API for isolate_ptr

The trait methods expose *mut std::ffi::c_void for the isolate pointer. This could be *mut v8::Isolate (or a wrapper type) for better type safety on the Rust side, since the caller always passes a v8::Isolate*. As-is, the user must cast it themselves.

4. No tests

There are no tests for the new API. At minimum, a test that:

• Creates a custom platform with a PlatformImpl
• Creates an isolate
• Posts a task and verifies the callback fires
• Shuts down the isolate and verifies notify_isolate_shutdown fires

This is important for validating the FFI plumbing actually works end-to-end.

5. Thread safety of Rust callback dispatch

The callbacks dereference context as &*(context as *const Box<dyn PlatformImpl>) without synchronization. This is fine because they only take a shared &self reference and PlatformImpl: Send + Sync is required. However, the DROP callback uses Box::from_raw which takes ownership — if DROP races with any other callback, that's UB. The C++ side should ensure no callbacks fire after or during DROP (the destructor). This seems to be the case since ~CustomPlatform runs DROP and the platform should be single-owner, but it's worth a comment.

6. GetThreadIsolatedAllocator override returns nullptr

v8::ThreadIsolatedAllocator* GetThreadIsolatedAllocator() override {
    return nullptr;
}

This matches UnprotectedDefaultPlatform but the PR description doesn't mention this. Worth a comment explaining this is intentional (disabling thread-isolated allocations for the same reasons as new_unprotected_default_platform).

7. Runner cache uses weak_ptr but creates new wrappers on expiry

The runners_ map stores weak_ptr<CustomTaskRunner>, and the shared_ptr is returned to the caller. If no one holds the returned shared_ptr, the weak_ptr expires and a new CustomTaskRunner is created on the next call. This means V8 could get different CustomTaskRunner instances for the same isolate+priority across calls, which is fine functionally but creates unnecessary allocations. Consider whether strong refs would be more appropriate.

8. Minor: thread_pool_size clamping is duplicated

The clamping logic std::max(std::min(thread_pool_size, 16), 1) is done in C++ and .min(16) is done in Rust. Pick one side.

---

Summary

The most important issue is #1 (missing override) — this is a correctness bug that would make the shutdown callback silently fail. The lack of tests (#4) is also significant for a PR adding FFI plumbing. The rest are improvements that would strengthen the API but aren't blockers.

[fraidev]

Code Review

Overview

Adds a trait-based PlatformImpl API that wraps V8's DefaultPlatform, intercepting foreground task runner methods and NotifyIsolateShutdown to notify Rust embedders. Follows the existing V8InspectorClientImpl pattern with double-boxed trait objects passed as void* context through C++ FFI.

Issues & Suggestions

1. NotifyIsolateShutdown is not override (bug)

In CustomPlatform::NotifyIsolateShutdown, the method is missing the override keyword. This means if the base class signature changes or if V8 calls it through the base pointer, the custom implementation won't be dispatched — it just shadows the base method.

// Current:
void NotifyIsolateShutdown(v8::Isolate* isolate) {
// Should be:
void NotifyIsolateShutdown(v8::Isolate* isolate) override {

This is a correctness bug — without override, V8 will call DefaultPlatform::NotifyIsolateShutdown directly and the Rust callback will never fire.

2. Panic safety in FFI callbacks

All the extern "C" Rust callbacks (e.g. v8__Platform__CustomPlatform__BASE__PostTask) call trait methods that could panic. Unwinding across an FFI boundary is undefined behavior. These should be wrapped in std::panic::catch_unwind or at minimum documented that implementations must not panic. The inspector client callbacks have the same pattern, but this is worth noting since post_task can be called from any V8 background thread.

3. Raw pointer API for isolate_ptr

The trait methods expose *mut std::ffi::c_void for the isolate pointer. This could be *mut v8::Isolate (or a wrapper type) for better type safety on the Rust side, since the caller always passes a v8::Isolate*. As-is, the user must cast it themselves.

4. No tests

There are no tests for the new API. At minimum, a test that:

• Creates a custom platform with a PlatformImpl
• Creates an isolate
• Posts a task and verifies the callback fires
• Shuts down the isolate and verifies notify_isolate_shutdown fires

This is important for validating the FFI plumbing actually works end-to-end.

5. Thread safety of Rust callback dispatch

The callbacks dereference context as &*(context as *const Box<dyn PlatformImpl>) without synchronization. This is fine because they only take a shared &self reference and PlatformImpl: Send + Sync is required. However, the DROP callback uses Box::from_raw which takes ownership — if DROP races with any other callback, that's UB. The C++ side should ensure no callbacks fire after or during DROP (the destructor). This seems to be the case since ~CustomPlatform runs DROP and the platform should be single-owner, but it's worth a comment.

6. GetThreadIsolatedAllocator override returns nullptr

v8::ThreadIsolatedAllocator* GetThreadIsolatedAllocator() override {
    return nullptr;
}

This matches UnprotectedDefaultPlatform but the PR description doesn't mention this. Worth a comment explaining this is intentional (disabling thread-isolated allocations for the same reasons as new_unprotected_default_platform).

7. Runner cache uses weak_ptr but creates new wrappers on expiry

The runners_ map stores weak_ptr<CustomTaskRunner>, and the shared_ptr is returned to the caller. If no one holds the returned shared_ptr, the weak_ptr expires and a new CustomTaskRunner is created on the next call. This means V8 could get different CustomTaskRunner instances for the same isolate+priority across calls, which is fine functionally but creates unnecessary allocations. Consider whether strong refs would be more appropriate.

8. Minor: thread_pool_size clamping is duplicated

The clamping logic std::max(std::min(thread_pool_size, 16), 1) is done in C++ and .min(16) is done in Rust. Pick one side.

Summary

The most important issue is #1 (missing override) — this is a correctness bug that would make the shutdown callback silently fail. The lack of tests (#4) is also significant for a PR adding FFI plumbing. The rest are improvements that would strengthen the API but aren't blockers.

Thanks for the detailed review!

#1 (override): NotifyIsolateShutdown is actually not virtual on DefaultPlatform, so override doesn't compile. Our method hides the base method, which works because callers always go through the CustomPlatform* type (the platform is created as CustomPlatform and stored in a shared_ptr). Added a comment explaining this.

#2 (panic safety): Agreed this is a concern in general, but the existing inspector client callbacks (V8InspectorClientImpl) follow the same pattern without catch_unwind. I'd prefer to keep consistency and address panic safety across all FFI callbacks in a separate PR if needed.

#3 (raw pointer type): The C++ side passes void* through the FFI boundary, and v8::Isolate in rusty_v8 is an opaque type whose inner pointer isn't directly constructible from FFI context. Using *mut c_void matches what the C++ side actually provides. The embedder (deno_core) uses the pointer as an opaque key for waker lookup, so the void pointer is appropriate here.

#4 (tests): Good call — I'll add a test in a follow-up.

#5 (DROP safety): Added a comment. The platform is single-owner via unique_ptr, and the destructor runs only after all isolates are disposed, so no callbacks can race with DROP.

#6 (GetThreadIsolatedAllocator): Added a comment explaining this intentionally disables thread-isolated allocations (same as UnprotectedDefaultPlatform), needed when isolates are created on worker threads.

#7 (weak_ptr): This is intentional — weak_ptr lets runners be cleaned up when V8 drops its reference (e.g. on isolate shutdown). Strong refs would keep wrappers alive unnecessarily. Added a comment.

#8 (clamping duplication): Fixed — removed the .min(16) from the Rust side, C++ handles all clamping.

[bartlomieju]

Re #1: Thanks for the clarification that NotifyIsolateShutdown is not virtual on DefaultPlatform. That makes sense that override won't compile.

However, this raises a subtler concern: since it's not virtual, the custom implementation only works when called through a CustomPlatform* pointer. If any V8 internal code path (or future refactor) holds a Platform* or DefaultPlatform* and calls NotifyIsolateShutdown, it will call the base implementation directly, silently bypassing the Rust callback. This is inherently fragile.

Worth verifying the actual call sites in V8 to confirm that the platform pointer is never downcast or stored as a base type when NotifyIsolateShutdown is invoked. If V8 calls it through a base pointer anywhere, the callback will never fire.

[fraidev]

Re #1: Thanks for the clarification that NotifyIsolateShutdown is not virtual on DefaultPlatform. That makes sense that override won't compile.

Oh, actually this was dead code, I've removed it