Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deno_runtime extension_with_ops example fails to find registered op #22600

Closed
jonmmease opened this issue Feb 27, 2024 · 13 comments · Fixed by #22906
Closed

deno_runtime extension_with_ops example fails to find registered op #22600

jonmmease opened this issue Feb 27, 2024 · 13 comments · Fixed by #22906

Comments

@jonmmease
Copy link

Context

Hi, I'm the maintainer of vl-convert, which is a crate that embeds deno_runtime and deno_core. When updating from the crate versions associated with Deno 1.38.4 to those associated with Deno 1.41.0, I found that the custom Rust ops that we register are no longer found from within the JavaScript runtime. After investigating a bit more, I'm seeing the same behavior in the extension_with_ops example as well.

I don't know if this is a bug, or if there has been a change in how ops should be registered which isn't reflected in the example, or something else.

Thanks for taking the time to take a look at this!

Setup

Extract the extension_with_ops example into a standalone cargo project.

Cargo.toml

[package]
name = "try_extension_with_ops"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
deno_runtime = "0.147.0"
deno_core = "0.264.0"
tokio = "1.36.0"

src/main.rs (Note modification of main.js path compared to example).

use std::path::Path;
use std::rc::Rc;

use deno_core::error::AnyError;
use deno_core::op2;
use deno_core::FsModuleLoader;
use deno_core::ModuleSpecifier;
use deno_runtime::permissions::PermissionsContainer;
use deno_runtime::worker::MainWorker;
use deno_runtime::worker::WorkerOptions;

deno_core::extension!(hello_runtime, ops = [op_hello]);

#[op2(fast)]
fn op_hello(#[string] text: &str) {
    println!("Hello {}!", text);
}

#[tokio::main]
async fn main() -> Result<(), AnyError> {
    let js_path = Path::new(env!("CARGO_MANIFEST_DIR"))
        .join("src/main.js");
    let main_module = ModuleSpecifier::from_file_path(js_path).unwrap();
    let mut worker = MainWorker::bootstrap_from_options(
        main_module.clone(),
        PermissionsContainer::allow_all(),
        WorkerOptions {
            module_loader: Rc::new(FsModuleLoader),
            extensions: vec![hello_runtime::init_ops()],
            ..Default::default()
        },
    );
    worker.execute_main_module(&main_module).await?;
    worker.run_event_loop(false).await?;
    Ok(())
}

src/main.js

Deno[Deno.internal].core.ops.op_hello("World");

Issue

When running this example with cargo run, the following panic is displayed.

Error: TypeError: Deno[Deno.internal].core.ops.op_hello is not a function

This is the same error I'm seeing when updating vl-convert

@mmastrac
Copy link
Contributor

We've started moving away from Deno[Deno.internal].core.ops to the ops module:

import {
  op_cache_delete,
  op_cache_match,
  op_cache_put,
  op_cache_storage_delete,
  op_cache_storage_has,
  op_cache_storage_open,
} from "ext:core/ops";

@jonmmease
Copy link
Author

Thanks @mmastrac, I tried updating the main.js file above to

import {
    op_hello,
} from "ext:core/ops";

op_hello("World");

And get this error:

Error: Importing ext: modules is only allowed from ext: and node: modules. 

Is this what you meant?

@jonmmease
Copy link
Author

I found a hack/workaround. Things work if I move the import code from main.js into strings in main.rs and execute them with worker.execute_script, as long as the script_name starts with ext:.

src/main.rs

use std::path::Path;
use std::rc::Rc;

use deno_core::error::AnyError;
use deno_core::op2;
use deno_core::FsModuleLoader;
use deno_core::ModuleSpecifier;
use deno_runtime::BootstrapOptions;
use deno_runtime::permissions::PermissionsContainer;
use deno_runtime::worker::MainWorker;
use deno_runtime::worker::WorkerOptions;

deno_core::extension!(hello_runtime, ops = [op_hello]);

#[op2(fast)]
fn op_hello(#[string] text: &str) {
    println!("Hello {}!", text);
}

#[tokio::main]
async fn main() -> Result<(), AnyError> {
    let js_path = Path::new(env!("CARGO_MANIFEST_DIR"))
        .join("src/main.js");
    let main_module = ModuleSpecifier::from_file_path(js_path).unwrap();
    // main_module.as_str()
    let mut worker = MainWorker::bootstrap_from_options(
        main_module.clone(),
        PermissionsContainer::allow_all(),
        WorkerOptions {
            module_loader: Rc::new(FsModuleLoader),
            extensions: vec![hello_runtime::init_ops()],
            bootstrap: BootstrapOptions {
                enable_testing_features: true,
                ..Default::default()
            },
            ..Default::default()
        },
    );
    worker.execute_main_module(&main_module).await?;
    worker.run_event_loop(false).await?;

    let script_code = r#"
    var op_hello;
    import("ext:core/ops").then((imported) => {{
        op_hello = imported.op_hello;
    }})
    "#.to_string();
    worker.execute_script("ext:<anon>", script_code.into())?;
    worker.run_event_loop(false).await?;

    let script_code = r#"
    op_hello("World");
    "#.to_string();
    worker.execute_script("ext:<anon>", script_code.into())?;
    worker.run_event_loop(false).await?;

    Ok(())
}

src/main.js (now empty)

I think this is enough for our current usecase, but I'd be interested to know if there's a better way to do this. Thanks!

@mmastrac
Copy link
Contributor

Ideally you should add some ES modules to your extension, and from those modules re-export the op functions you need.

There's an example here -- we need to update this example to the modern style when we have some time (or if you'd like to PR something for it, happy to review it!)

https://github.com/denoland/deno_core/blob/main/testing/checkin/runtime/async.ts

@jonmmease
Copy link
Author

Thanks @mmastrac, I'd be happy to contribute a PR if I figure out how to make this work.

When I follow the async.ts approach above, and update main.js with:

const { op_hello } = Deno.core.ensureFastOps();
op_hello("Hello")

I get an error that TypeError: Cannot read properties of undefined (reading 'ensureFastOps'). What determines whether Deno.core is available?

@AuTsing
Copy link

AuTsing commented Mar 13, 2024

@mmastrac
I have the same problem, I want to know where is the op_hello function, I had try to find it in

deno v1.39.4
import { core } from 'ext:core/mod.js';
const { op_hello } = core.ensureFastOps();
// op_hello is undefined
deno 1.40.5
import { op_hello } from 'ext:core/ops';
// op_hello is undefined

but I can't find it anywhere.

@AuTsing
Copy link

AuTsing commented Mar 13, 2024

@jonmmease
Have you solve this problem?

@jonmmease
Copy link
Author

Hi @AuTsing, I only worked around it as I described in #22600 (comment), where everything is evaluated from Rust using worker.execute_script. I didn't figure out how to fix the original example where the import of op_hello is performed in the main module.

@AuTsing
Copy link

AuTsing commented Mar 13, 2024

@jonmmease
I think the key to figure out what the problem is to how deno load the ops.

Its a challenge.

@AuTsing
Copy link

AuTsing commented Mar 13, 2024

@jonmmease
I think I know what your problem is.
Since deno v1.40.3, extensions had been migrated to virtual ops module. #22135
Therefore, you should use your op in a different way, just like this(#22600 (comment)).

In your case, just make the following modifications.

  • Add a js file to import your op and export it in some way.
// maybe like this
// mod.js
import { op_hello } from "ext:core/ops";
globalThis.myops.hello = op_hello;
  • Add this file to your extension definition.
extension!(hello_runtime,
  ops = [op_hello],
  esm_entry_point = "ext:hello_runtime/mod.js",
  esm = ["mod.js"],
)

I think this would be work.

@mmastrac
Copy link
Contributor

I'm fixing up the extension example in #22906 -- please let me know if that helps with understanding the op registration.

mmastrac added a commit that referenced this issue Mar 14, 2024
)

Better example to close #22600

---------

Signed-off-by: Matt Mastracci <matthew@mastracci.com>
@AuTsing
Copy link

AuTsing commented Mar 14, 2024

Thanks @mmastrac, that's a clear example.
But I think my issue is not this.
I found my problem by reading the source code.
The reason I couldn't find my custom op anywhere is that I added extension after the snapshot.
And the WorkerOptions skip_op_registration disable the op registration when sub command is run.
Anyway, thank you for your guidance.

nathanwhit pushed a commit that referenced this issue Mar 14, 2024
)

Better example to close #22600

---------

Signed-off-by: Matt Mastracci <matthew@mastracci.com>
dsherret pushed a commit to dsherret/deno that referenced this issue Mar 15, 2024
…oland#22906)

Better example to close denoland#22600

---------

Signed-off-by: Matt Mastracci <matthew@mastracci.com>
@jonmmease
Copy link
Author

Thanks @mmastrac, that's very helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants