baphomet_the_traveller
baphomet_the_traveller•13mo ago

`WARNING: v8::OwnedIsolate for snapshot was leaked` and/or crash after using snapshot

On startup, I create a runtime:
let js_runtime = JsRuntimeForSnapshot::new(deno_core::RuntimeOptions {
module_loader: Some(Rc::new(FsModuleLoader)),
extensions: options.extensions,
..Default::default()
});
let snapshot = js_runtime.snapshot().to_vec();
let js_runtime = JsRuntimeForSnapshot::new(deno_core::RuntimeOptions {
module_loader: Some(Rc::new(FsModuleLoader)),
extensions: options.extensions,
..Default::default()
});
let snapshot = js_runtime.snapshot().to_vec();
Then I will recreate the runtime, since snapshot consumes it:
let _snapshot = Snapshot::Boxed(snapshot.clone().into_boxed_slice());
JsRuntimeForSnapshot::new(deno_core::RuntimeOptions {
startup_snapshot: Some(_snapshot),
module_loader: Some(Rc::new(FsModuleLoader)),
..Default::default()
})
let _snapshot = Snapshot::Boxed(snapshot.clone().into_boxed_slice());
JsRuntimeForSnapshot::new(deno_core::RuntimeOptions {
startup_snapshot: Some(_snapshot),
module_loader: Some(Rc::new(FsModuleLoader)),
..Default::default()
})
But the next time I reload from the snapshot in this way I get WARNING: v8::OwnedIsolate for snapshot was leaked, and some of the extensions result in a crash:
(exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)
(exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)
My extremely roundabout method here seems off to me So how do I actually use snapshot correctly to avoid this
35 Replies
mmastrac
mmastrac•13mo ago
Ah yeah. In the second one, JsRuntimeForSnapshot should just be JsRuntime We've played around with how we can make this API less terrible but it might be useful just to improve that warning message instead
baphomet_the_traveller
baphomet_the_traveller•13mo ago
It's not the most specific message i have seen The warning is gone but the crash is not gone
mmastrac
mmastrac•13mo ago
If you're using snapshotting, there's some very specific requirements around external references Make sure you pass the extensions list to the second runtime
baphomet_the_traveller
baphomet_the_traveller•13mo ago
it's when I call this op:
#[op2]
/// Registers a JS function with the runtime as being the entrypoint for the script
///
/// # Arguments
/// * `state` - The runtime's state, into which the function will be put
/// * `callback` - The function to register
fn op_register_entrypoint(
state: &mut OpState,
#[global] callback: v8::Global<v8::Function>,
) -> Result<(), Error> {
state.put(callback);
Ok(())
}
#[op2]
/// Registers a JS function with the runtime as being the entrypoint for the script
///
/// # Arguments
/// * `state` - The runtime's state, into which the function will be put
/// * `callback` - The function to register
fn op_register_entrypoint(
state: &mut OpState,
#[global] callback: v8::Global<v8::Function>,
) -> Result<(), Error> {
state.put(callback);
Ok(())
}
Ah see there's the rub Extension does not provide clone or copy
mmastrac
mmastrac•13mo ago
Yeah, you need to recreate it 🙂
baphomet_the_traveller
baphomet_the_traveller•13mo ago
and the structs they come from don't have a trait
mmastrac
mmastrac•13mo ago
GitHub
Create an ExtensionSet to handle logic around multiple extensions...
There is a fair amount of logic in JsRuntime that deals with collating the data from multiple Extensions. We should move all of this logic to an ExtensionSet that can be initialized from a slice of...
mmastrac
mmastrac•13mo ago
The code in Deno to juggle snapshots + extensions + extending snapshots for workers is extremely complex
baphomet_the_traveller
baphomet_the_traveller•13mo ago
Maybe we could move the init_esm/init_ops stuff to a trait? That would fix things for a lot of cases allow copy on the structs, then call init from the trait boom no clone needed
mmastrac
mmastrac•13mo ago
The long-term plan is that extensions would become const structs, but we're only about halfway through that work Then extensions would be static consts and extension init becomes nearly free
baphomet_the_traveller
baphomet_the_traveller•13mo ago
in the meantime I guess the user will need to provide the extensions twice, in new() and in reset()
mmastrac
mmastrac•13mo ago
Yes
baphomet_the_traveller
baphomet_the_traveller•13mo ago
ouch
mmastrac
mmastrac•13mo ago
Yeah, I've struggled with it. If I had more bandwidth I'd fix it today
baphomet_the_traveller
baphomet_the_traveller•13mo ago
oddly the deno_console one still works after a reload with no extensions
mmastrac
mmastrac•13mo ago
No ops == no crash
baphomet_the_traveller
baphomet_the_traveller•13mo ago
No ops == no fun
mmastrac
mmastrac•13mo ago
The problem is in the bindings.rs stuff If you want to PR a basic, #[doc(hidden)] extension trait I can definitely merge that
baphomet_the_traveller
baphomet_the_traveller•13mo ago
I've already started writing it haha
mmastrac
mmastrac•13mo ago
Sweet! If you can make a very simple one with just the init_ functions for now, it'll make it easier to merge. It's probably better if we do this one small step at a time
baphomet_the_traveller
baphomet_the_traveller•13mo ago
hm right, the parameters in the macro It would not be compatible with that option What about something simple like adding to JsRuntime:
/// Consume the extensions in this snapshot, removing them from the runtime
pub fn consume_extensions(&mut self) -> Vec<Extension> {
std::mem::take(&mut self.extensions)
}
/// Consume the extensions in this snapshot, removing them from the runtime
pub fn consume_extensions(&mut self) -> Vec<Extension> {
std::mem::take(&mut self.extensions)
}
That way one could use that to extract loaded extensions just prior to snapshot, Then provide them to the new Runtime during reload? Because I don't see snapshot() reading from extensions
mmastrac
mmastrac•13mo ago
Unfortunately because we have the different extension modes this doesn't necessarily work
baphomet_the_traveller
baphomet_the_traveller•13mo ago
Then for the moment I think Instead of having the user provide a vec of extensions, I will take in a closure that returns the vec - so I can just call it twice! That way no changes needed to deno_core
mmastrac
mmastrac•13mo ago
That would work 🙂 I fully agree that this interface is not great It's just that solving it is not easy (for example -- it is silly that you need to provide initialization data for snapshots)
baphomet_the_traveller
baphomet_the_traveller•13mo ago
Clone could work for Extensions if it weren;t for that meddling middleware Yeah and snapshot consumes the runtime, so I need to do it twice
mmastrac
mmastrac•13mo ago
Yup
baphomet_the_traveller
baphomet_the_traveller•13mo ago
There should be a way to load a snapshot into a running instance without needing to create a new one Acually - that might be a far easier change
mmastrac
mmastrac•13mo ago
Most of the time you create the snapshot and regular runtime in different processes, however
baphomet_the_traveller
baphomet_the_traveller•13mo ago
A different usecase than mine then for sure Ok so... when I do provide the same set of Extensions during snapshot reload I get a different crash, about the extensions being already loaded Perhaps you've re-provided a module or extension that was already included in the snapshot The one it is complaining about is the same one that crashes if I do not provide it
mmastrac
mmastrac•13mo ago
I believe you need to use init_ops the second time and init_ops_and_esm for the snapshot Or maybe the other way around hmm
baphomet_the_traveller
baphomet_the_traveller•13mo ago
....... so I can't use a closure to provide the extensions either then
mmastrac
mmastrac•13mo ago
Maybe a macro? That duplicates the same calls for init_ops and init_ops_and_esm
baphomet_the_traveller
baphomet_the_traveller•13mo ago
New train of thought... can I unload a module? Make sure other modules can't import it, invalidate the id etc? Because if so I'll just forget snapshots for now They don't seem like a good fit for my use-case here I'll just unload all the modules and reset globalThis If not I smell a PR coming Super easy: In realms:
/// Clears all loaded modules
pub fn clear_modules(&mut self) {
self.0.module_map().borrow_mut().clear_module_map(&[])
}
/// Clears all loaded modules
pub fn clear_modules(&mut self) {
self.0.module_map().borrow_mut().clear_module_map(&[])
}
And in runtime:
/// Clears all loaded modules
pub fn clear_modules(&mut self) {
self.main_realm().clear_modules()
}
/// Clears all loaded modules
pub fn clear_modules(&mut self) {
self.main_realm().clear_modules()
}
I'll take a backup copy of the global object on startup, Then I just have to revert that and call clear_modules() on my runtime instance And boom, no snapshots Yup, seems to let me reset, still use ops, and reload modules again! If it sounds kosher to you I'll set up the PR
mmastrac
mmastrac•13mo ago
I think we will just need to make sure the v8 resources around the modules have been released. Are you able to see if memory is reclaimed by loading/unloading 1000 modules repeatedly?
baphomet_the_traveller
baphomet_the_traveller•13mo ago
Running the test now Loading and unloading 1000 modules 1000 times It's still going but its been over a minute and it keeps bouncing between 36 and 40k usage for the process 🙂 No steady climb Test passed I want to try again with a larger module, but seems stable so far If that passes too I'll open a pr later tonight PR is up