Weak handles and finalizers #895
Conversation
|
|
andreubotella
force-pushed
the
weak
branch
2 times, most recently
from
7d6c565 to
1b98d81
Compare
|
I know this is WIP, but it would be nice to get some sort of test so we can see that it works (and that lets us poke at some edge cases). |
src/handle.rs
Outdated
| } | ||
|
|
||
| extern "C" fn second_pass_callback(wci: *const WeakCallbackInfo) { | ||
| // FIXME!!!!: Do we know for a fact that the parameter hasn't been dropped?? |
There was a problem hiding this comment.
I think it is safe? WeakData<T> is not dropped until Weak<T> itself drops.
There was a problem hiding this comment.
The first pass callback must be called the moment the reference is GC'd, since it's that callback's job to reset the weak handle, but the second pass callback has no such guarantees as far as I'm aware. Looking in the V8 code it seems like sometimes second passes run during GC, but other times they're scheduled on the platform task runner (which I believe would only get called when pumping the V8 message loop at the beginning of the next run in Deno's event loop).
There was a problem hiding this comment.
What mechanism exists that could cause the WeakData to be dropped earlier than expected?
There was a problem hiding this comment.
My concern is a scenario where the value is GC'd while Weak is still alive, but the second pass callback is scheduled for later, and Weak drops before that second pass callback runs. We could have flags in WeakData to check whether the second callback has run, and whether the corresponding Weak has dropped, and then leak the Box<WeakData> in the drop impl and deallocate it in the second pass callback if this is the case.
It seems like the various ways to trigger GC in tests (gc(), sending a low-memory notification, even running the GC in predictable mode) will cause second passes to run synchronously, so I'm not sure that this is testable.
|
I was getting weird results when creating weak handles from globals, and apparently dropping globals will not cause the first callback to be called. #[test]
fn weak_from_global() {
let _setup_guard = setup();
let mut isolate = v8::Isolate::new(Default::default());
let mut scope = v8::HandleScope::new(&mut isolate);
let context = v8::Context::new(&mut scope);
let scope = &mut v8::ContextScope::new(&mut scope, context);
let global = {
let object = v8::Object::new(scope);
v8::Global::new(scope, object)
};
let weak = v8::Weak::new(scope, &global);
assert!(!weak.is_empty());
assert_eq!(weak.open(scope).unwrap(), global.open(scope));
drop(global);
eval(scope, "gc()").unwrap();
assert!(weak.is_empty()); // This assertion fails.
} |
|
@andreubotella Try a |
I think the problem is that the HandleScope is still keeping the |
|
@andreubotella FYI I've fixed and added the |
|
Added some comments but this looks pretty good to me overall. |
I was trying to figure out how to test that there's no UB if the second pass callback is called asynchronously, after |
I think the biggest concern is now what happens when the isolate is disposed between the first and second stage. Is that correct? |
That's right. But I don't think there's a way to prevent leaking the |
| } | ||
| } | ||
|
|
||
| fn get_pointer(&self) -> Option<NonNull<T>> { |
There was a problem hiding this comment.
In NAPI, we implement reference counting to these Weak instances, if the ref count is greater than 0 we need to "clear the weak" and prevent finalization (sorta like what's happening in drop() rn?) What would be the right API for this?
There was a problem hiding this comment.
Per conversation on Discord, keeping track of the reference count and clearing the weak handle can be done in Deno, but we would need a way to prevent finalization of a Weak, possibly even after the first pass callback has run. Since the second pass callback will only run the finalizer if it's Some, preventing finalization would just involve setting it to None.
andreubotella
changed the title
src/handle.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn unset_finalizer(&mut self) { |
There was a problem hiding this comment.
This method doesn't really have to take &mut self, since the finalizer is behind a Cell, but semantically it does mutate the handle. WDYT?
There was a problem hiding this comment.
I think this is better that using &self 👍 let's keep it
|
Rebasing this to main seems to introduce a V8 crash in Edit: Nevermind, this was me forgetting that I had to update the usage of |
|
I'm not 100% sure that the semantics of when finalizers are run make sense. If a Also, while I was thinking of this I noticed that the |
I don't think that makes sense - we should prevent the finalizer from running after the Weak has been dropped.
I'm also not super happy with the potential for memory leaks when the Isolate is dropped between first and second stage callbacks. Maybe what we should do is the following:
|
Done. Rather than storing the |
|
@littledivy In the end we decided to remove the @piscisaureus This should be now ready. |
yup |
There was a problem hiding this comment.
Using this branch crashes the Deno repl. (Something going on with typescript compiler ops).
#
# Fatal error in ../../../../rusty_v8/v8/src/objects/objects-inl.h, line 455
# Debug check failed: IsNumber().
#
#
#
#FailureMessage Object: 0x1701736c8
==== C stack trace ===============================
0 deno 0x000000010470d18c v8::base::debug::StackTrace::StackTrace() + 24
1 deno 0x0000000104714d5c v8::platform::(anonymous namespace)::PrintStackTrace() + 116
2 deno 0x0000000104704ce4 V8_Fatal(char const*, int, char const*, ...) + 268
3 deno 0x00000001047047ac std::__1::enable_if<((!(std::is_function<std::__1::remove_pointer<char>::type>::value)) && (!(std::is_enum<char>::value))) && (has_output_operator<char, v8::base::CheckMessageStream>::value), std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >::type v8::base::PrintCheckOperand<char>(char) + 0
4 deno 0x00000001047e1a18 v8::Integer::Value() const + 148
5 deno 0x00000001046e9778 v8__Integer__Value + 24
6 deno 0x00000001046b31dc _ZN2v86number35_$LT$impl$u20$v8..data..Integer$GT$5value17h664286859e3a18ffE + 24
7 deno 0x0000000100df1d6c deno::lsp::tsc::op_respond::v8_func::h35ecb08cd383903e + 120
8 deno 0x0000000100e1f938 core::ops::function::Fn::call::h519fb15c654e8819 + 52
9 deno 0x000000010136ec84 _ZN2v88function147_$LT$impl$u20$v8..support..MapFnFrom$LT$F$GT$$u20$for$u20$extern$u20$$u22$C$u22$$u20$fn$LP$$BP$const$u20$v8..function..FunctionCallbackInfo$RP$$GT$7mapping28_$u7b$$u7b$closure$u7d$$u7d$17h75289b9897a40af5E + 332
10 deno 0x00000001013ea9f4 _ZN105_$LT$extern$u20$$u22$C$u22$$u20$fn$LP$A0$RP$$u20$.$GT$$u20$R$u20$as$u20$v8..support..CFnFrom$LT$F$GT$$GT$7mapping4c_fn17h0f7a4e174e2d7ed8E + 188
11 deno 0x00000001048444c0 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) + 800
12 deno 0x0000000104842a8c v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) + 816
13 deno 0x00000001048413e8 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) + 524
14 deno 0x0000000105e2b6c8 Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit + 136
15 deno 0x0000000105bb4314 Builtins_InterpreterEntryTrampoline + 308
16 deno 0x0000000105bb4314 Builtins_InterpreterEntryTrampoline + 308
17 deno 0x0000000105bb4314 Builtins_InterpreterEntryTrampoline + 308
18 deno 0x0000000105bb4314 Builtins_InterpreterEntryTrampoline + 308
19 deno 0x0000000105bad7d8 Builtins_JSEntryTrampoline + 184
20 deno 0x0000000105bad468 Builtins_JSEntry + 168
21 deno 0x0000000104a47c9c v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) + 4376
22 deno 0x0000000104a48640 v8::internal::Execution::CallScript(v8::internal::Isolate*, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>) + 472
23 deno 0x00000001047c32e4 v8::Script::Run(v8::Local<v8::Context>, v8::Local<v8::Data>) + 1216
24 deno 0x00000001046eb3f4 v8__Script__Run + 92
25 deno 0x00000001046ca600 _ZN2v86script34_$LT$impl$u20$v8..data..Script$GT$3run28_$u7b$$u7b$closure$u7d$$u7d$17hedcb8a831580682dE + 56
26 deno 0x00000001046d88f0 _ZN2v85scope27HandleScope$LT$$LP$$RP$$GT$10cast_local17h64b34c9057b07c6dE + 72
27 deno 0x00000001046b4434 _ZN2v86script34_$LT$impl$u20$v8..data..Script$GT$3run17h89116a7209c48deaE + 64
28 deno 0x00000001045bfc0c deno_core::runtime::JsRuntime::execute_script::h157dd1708db9f406 + 512
29 deno 0x0000000100d52b80 deno::lsp::tsc::request::h29a86fe263d8ab14 + 1340
30 deno 0x0000000100d29630 _ZN4deno3lsp3tsc8TsServer3new28_$u7b$$u7b$closure$u7d$$u7d$28_$u7b$$u7b$closure$u7d$$u7d$17h4c11f6a6e80c014fE + 900
31 deno 0x0000000100f2448c _ZN97_$LT$core..future..from_generator..GenFuture$LT$T$GT$$u20$as$u20$core..future..future..Future$GT$4poll17h707b9a8b1146c4e8E + 84
32 deno 0x0000000100a3a350 _ZN72_$LT$core..pin..Pin$LT$P$GT$$u20$as$u20$core..future..future..Future$GT$4poll17h1e2f367c7e90200aE + 88
33 deno 0x0000000100bfb574 _ZN5tokio7runtime15basic_scheduler9CoreGuard8block_on28_$u7b$$u7b$closure$u7d$$u7d$28_$u7b$$u7b$closure$u7d$$u7d$28_$u7b$$u7b$closure$u7d$$u7d$17h20239f57c9824759E + 72
34 deno 0x000000010118486c _ZN5tokio4coop11with_budget28_$u7b$$u7b$closure$u7d$$u7d$17h5b94841875dd443bE + 196
35 deno 0x0000000100a55708 _ZN3std6thread5local17LocalKey$LT$T$GT$8try_with17heb4e8f9baffa8d6bE + 188
36 deno 0x0000000100a4f9f0 _ZN3std6thread5local17LocalKey$LT$T$GT$4with17h7ee4b6176ab330f5E + 48
37 deno 0x0000000100bfb428 _ZN5tokio7runtime15basic_scheduler9CoreGuard8block_on28_$u7b$$u7b$closure$u7d$$u7d$28_$u7b$$u7b$closure$u7d$$u7d$17hf11bb8487b25d55dE + 172
38 deno 0x0000000100bf354c tokio::runtime::basic_scheduler::Context::enter::h7faa10e32a8bb836 + 308
39 deno 0x0000000100bf9034 _ZN5tokio7runtime15basic_scheduler9CoreGuard8block_on28_$u7b$$u7b$closure$u7d$$u7d$17h3fb3f58991aba375E + 380
40 deno 0x0000000100bfd6c0 _ZN5tokio7runtime15basic_scheduler9CoreGuard5enter28_$u7b$$u7b$closure$u7d$$u7d$17ha03d9cf4d5dfbc31E + 48
41 deno 0x00000001009bb458 _ZN5tokio6macros10scoped_tls18ScopedKey$LT$T$GT$3set17hedbcace6b9a4df13E + 128
42 deno 0x0000000100bfd368 tokio::runtime::basic_scheduler::CoreGuard::enter::hfbe77a5b314a5684 + 284
43 deno 0x0000000100bf8038 tokio::runtime::basic_scheduler::CoreGuard::block_on::hf7a1f36eeb94d7fa + 56
44 deno 0x0000000100bf0c70 tokio::runtime::basic_scheduler::BasicScheduler::block_on::hf19a33b65c602371 + 204
45 deno 0x000000010104fd40 tokio::runtime::Runtime::block_on::h40ace54bea16a12c + 152
46 deno 0x0000000100d29254 _ZN4deno3lsp3tsc8TsServer3new28_$u7b$$u7b$closure$u7d$$u7d$17h54152d3e4debc869E + 168
47 deno 0x0000000100ae5820 std::sys_common::backtrace::__rust_begin_short_backtrace::h47533819778b5f96 + 32
48 deno 0x0000000101167a8c _ZN3std6thread7Builder15spawn_unchecked28_$u7b$$u7b$closure$u7d$$u7d$28_$u7b$$u7b$closure$u7d$$u7d$17hda4235906f1b64d0E + 32
49 deno 0x00000001008b0484 _ZN115_$LT$core..panic..unwind_safe..AssertUnwindSafe$LT$F$GT$$u20$as$u20$core..ops..function..FnOnce$LT$$LP$$RP$$GT$$GT$9call_once17h99da90c9c54a2aa7E + 32
50 deno 0x00000001008d55cc std::panicking::try::do_call::h732e87fb06b765f5 + 68
51 deno 0x00000001008fb9e4 __rust_try + 32
52 deno 0x00000001008cd518 std::panicking::try::hb4cfc88c580ffc43 + 116
53 deno 0x00000001013d02bc std::panic::catch_unwind::hf674a37210def406 + 32
54 deno 0x0000000101166e7c _ZN3std6thread7Builder15spawn_unchecked28_$u7b$$u7b$closure$u7d$$u7d$17h456ccffcf1429df8E + 280
55 deno 0x0000000100e21ed4 _ZN4core3ops8function6FnOnce40call_once$u7b$$u7b$vtable.shim$u7d$$u7d$17h4e4a95ed1146f2b7E + 24
56 deno 0x00000001062c7298 std::sys::unix::thread::Thread::new::thread_start::ha92b558f2b29b4cc + 48
57 libsystem_pthread.dylib 0x00000001bad41240 _pthread_start + 148
58 libsystem_pthread.dylib 0x00000001bad3c024 thread_start + 8
fish: Job 1, 'target/debug/deno' terminated by signal SIGTRAP (Trace or breakpoint trap)
Not caused by this PR. It seems to be triggered by updating the V8 submodule to |
This change adds support for weak handles that don't prevent GC of the referenced objects, through the `v8::Weak<T>` API. A weak handle can be empty (if it was created empty or its object was GC'd) or non-empty, and if non-empty it allows getting its object as a global or local. When creating a `v8::Weak` you can also set a finalizer that will be called at some point after the object is GC'd, as long as the weak handle is still alive at that point. This finalization corresponds to the second-pass callback in `kParameter` mode in the C++ API, so it will only be called after the object is GC'd. The finalizer function is a `FnOnce` that may close over data, and which takes a `&mut Isolate` as an argument. The C++ finalization API doesn't guarantee _when_ or even _if_ the finalizer will ever be called, but in order to prevent memory leaks, the rusty_v8 wrapper ensures that it will be called at some point, even if it's just before the isolate gets dropped. `v8::Weak<T>` implements `Clone`, but a finalizer is tied to a single weak handle, so its clones won't be able to keep the finalizer alive. And in fact, cloning will create a new weak handle that isn't tied to a finalizer at all. `v8::Weak::clone_with_finalizer` can be used to make a clone of a weak handle which has a finalizer tied to it. Note that `v8::Weak<T>` doesn't implement `Hash`, because the hash would have to change once the handle's object is GC'd, which is a big gotcha and would break some of the algorithms that rely on hashes, such as the Rust std's `HashMap`. Closes denoland#861.
|
Since there's been a lot of back and forth on some topics, the resulting behavior might not be clear, so I've squashed all the commits and written a single commit message that hopefully informs the reviews. |
|
Currently, this PR runs any remaining finalization callbacks as the isolate is being disposed, in particular after dropping the isolate's annex, which as @littledivy found, triggers a crash when you run
The V8 C++ API guarantees nothing about whether a finalizer will run. This PR currently guarantees that the finalizer will always run, but that causes the above mess. I think we should instead run the finalizers as long as there isn't a The tricky part is how to run the GC after all scopes and contexts have been dropped, so you could have finalizers on the global object. Edit: I think this is now ready. I checked every weak or finalizer-related test, as well as @littledivy's test case, with valgrind, and there seem to be no memory bugs anymore. I did notice a use-after-free of |
This change adds support for weak handles that don't prevent GC of the referenced objects, through the `v8::Weak<T>` API. A weak handle can be empty (if it was created empty or its object was GC'd) or non-empty, and if non-empty it allows getting its object as a global or local. When creating a `v8::Weak` you can also set a finalizer that will be called at some point after the object is GC'd, as long as the weak handle is still alive at that point. This finalization corresponds to the second-pass callback in `kParameter` mode in the C++ API, so it will only be called after the object is GC'd. The finalizer function is a `FnOnce` that may close over data, and which takes a `&mut Isolate` as an argument. The C++ finalization API doesn't guarantee _when_ or even _if_ the finalizer will ever be called, but in order to prevent memory leaks, the rusty_v8 wrapper ensures that it will be called at some point, even if it's just before the isolate gets dropped. `v8::Weak<T>` implements `Clone`, but a finalizer is tied to a single weak handle, so its clones won't be able to keep the finalizer alive. And in fact, cloning will create a new weak handle that isn't tied to a finalizer at all. `v8::Weak::clone_with_finalizer` can be used to make a clone of a weak handle which has a finalizer tied to it. Note that `v8::Weak<T>` doesn't implement `Hash`, because the hash would have to change once the handle's object is GC'd, which is a big gotcha and would break some of the algorithms that rely on hashes, such as the Rust std's `HashMap`.
Just a strawperson proposal to test out the waters. The Discord conversation mentioned storing the callbacks in the isolate annex (or something), but doing it with pinned boxes seems easier to implement. We can still change it to use the annex, of course.
Closes #861.