baphomet_the_traveller•15mo 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•15mo 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_travellerOP•15mo ago

It's not the most specific message i have seen The warning is gone but the crash is not gone

mmastrac•15mo 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_travellerOP•15mo 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•15mo ago

Yeah, you need to recreate it 🙂

baphomet_the_travellerOP•15mo ago

and the structs they come from don't have a trait

mmastrac•15mo ago

https://github.com/denoland/deno_core/issues/52

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•15mo ago

The code in Deno to juggle snapshots + extensions + extending snapshots for workers is extremely complex

baphomet_the_travellerOP•15mo 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•15mo 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_travellerOP•15mo ago

in the meantime I guess the user will need to provide the extensions twice, in new() and in reset()

mmastrac•15mo ago

Yes

baphomet_the_travellerOP•15mo ago

ouch

mmastrac•15mo ago

Yeah, I've struggled with it. If I had more bandwidth I'd fix it today

baphomet_the_travellerOP•15mo ago

oddly the deno_console one still works after a reload with no extensions

mmastrac•15mo ago

No ops == no crash

baphomet_the_travellerOP•15mo ago

No ops == no fun

mmastrac•15mo 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_travellerOP•15mo ago

I've already started writing it haha

mmastrac•15mo 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_travellerOP•15mo 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•15mo ago

Unfortunately because we have the different extension modes this doesn't necessarily work

baphomet_the_travellerOP•15mo 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•15mo 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_travellerOP•15mo 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•15mo ago

Yup

baphomet_the_travellerOP•15mo 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•15mo ago

Most of the time you create the snapshot and regular runtime in different processes, however

baphomet_the_travellerOP•15mo 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•15mo 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_travellerOP•15mo ago

....... so I can't use a closure to provide the extensions either then

mmastrac•15mo ago

Maybe a macro? That duplicates the same calls for init_ops and init_ops_and_esm

baphomet_the_travellerOP•15mo 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•15mo 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_travellerOP•15mo 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

Chat about Deno, a modern runtime for JavaScript and TypeScript.

20KMembers

View on Discord