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

Update deno crates to latest version #105

Merged
merged 6 commits into from
Aug 7, 2024

Conversation

jaudiger
Copy link
Contributor

Resolve #81

This PR update deno_coreand serde_v8 to their latest version. I also made the switch to the usage of Rust 1.79, since deno_core requires it to make use of rust-lang/rust#104087.

@jaudiger jaudiger marked this pull request as draft July 19, 2024 09:00
@jaudiger
Copy link
Contributor Author

Not yet ready, since I'm still getting the following error when trying to execute this brioche command:

bash-5.2$ RUST_BACKTRACE=full cargo run -- run -p /tmp/brioche-packages/examples/nodejs_frontend 
    Finished `dev` profile [optimized + debuginfo] target(s) in 1.80s
     Running `target/debug/brioche run -p /tmp/brioche-packages/examples/nodejs_frontend`
[1.27s] 0 / 0+ jobs complete
thread 'main' panicked at /home/container/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_unsync-0.3.10/src/task.rs:56:3:
assertion failed: Handle::current().runtime_flavor() == RuntimeFlavor::CurrentThread
[3.35s] 0 / 0+ jobs complete
   0:     0x555558a2b745 - std::backtrace_rs::backtrace::libunwind::trace::h1a07e5dba0da0cd2
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/../../backtrace/src/backtrace/libunwind.rs:105:5
   1:     0x555558a2b745 - std::backtrace_rs::backtrace::trace_unsynchronized::h61b9b8394328c0bc
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x555558a2b745 - std::sys_common::backtrace::_print_fmt::h1c5e18b460934cff
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/sys_common/backtrace.rs:68:5
   3:     0x555558a2b745 - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h1e1a1972118942ad
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/sys_common/backtrace.rs:44:22
   4:     0x555558a5b20b - core::fmt::rt::Argument::fmt::h07af2b4071d536cd
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/fmt/rt.rs:165:63
   5:     0x555558a5b20b - core::fmt::write::hc090a2ffd6b28c4a
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/fmt/mod.rs:1157:21
   6:     0x555558a2723f - std::io::Write::write_fmt::h8898bac6ff039a23
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/io/mod.rs:1832:15
   7:     0x555558a2b51e - std::sys_common::backtrace::_print::h4e80c5803d4ee35b
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/sys_common/backtrace.rs:47:5
   8:     0x555558a2b51e - std::sys_common::backtrace::print::ha96650907276675e
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/sys_common/backtrace.rs:34:9
   9:     0x555558a2cb29 - std::panicking::default_hook::{{closure}}::h215c2a0a8346e0e0
  10:     0x555558a2c86d - std::panicking::default_hook::h207342be97478370
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:298:9
  11:     0x555558a2cfc3 - std::panicking::rust_panic_with_hook::hac8bdceee1e4fe2c
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:795:13
  12:     0x555558a2ce6b - std::panicking::begin_panic_handler::{{closure}}::h00d785e82757ce3c
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:656:13
  13:     0x555558a2bc09 - std::sys_common::backtrace::__rust_end_short_backtrace::h1628d957bcd06996
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/sys_common/backtrace.rs:171:18
  14:     0x555558a2cbd7 - rust_begin_unwind
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:652:5
  15:     0x5555558e1243 - core::panicking::panic_fmt::hdc63834ffaaefae5
                               at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:72:14
  16:     0x5555558e12ec - core::panicking::panic::h75b3c9209f97d725
[3.45s] 0 / 0+ jobs complete
  17:     0x555555883abb - deno_unsync::task::spawn::h3d4b45d6e4bac497
                               at /home/container/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_unsync-0.3.10/src/task.rs:56:3
  18:     0x555555883abb - deno_core::runtime::op_driver::futures_unordered_driver::FuturesUnorderedDriver<C>::spawn_task::h86233caecc35174d
                               at /home/container/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.294.0/runtime/op_driver/futures_unordered_driver.rs:111:37
  19:     0x55555605ef6d - deno_core::runtime::op_driver::futures_unordered_driver::FuturesUnorderedDriver<C>::ensure_task::hb57d870c90b5c30e
                               at /home/container/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.294.0/runtime/op_driver/futures_unordered_driver.rs:101:7
  20:     0x55555605ef6d - deno_core::runtime::op_driver::futures_unordered_driver::FuturesUnorderedDriver<C>::spawn::hb867395fbc18560f
                               at /home/container/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.294.0/runtime/op_driver/futures_unordered_driver.rs:118:10
  21:     0x55555605ef6d - <deno_core::runtime::op_driver::futures_unordered_driver::FuturesUnorderedDriver<C> as deno_core::runtime::op_driver::OpDriver<C>>::submit_op_fallible::h5e38d39f60f07cbc
                               at /home/container/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.294.0/runtime/op_driver/futures_unordered_driver.rs:152:26
  22:     0x555555d5a6c4 - deno_core::runtime::op_driver::OpDriver::submit_op_fallible_scheduling::h3b66a7572f8985f8
                               at /home/container/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.294.0/runtime/op_driver/mod.rs:110:30
  23:     0x555555d5a6c4 - deno_core::runtime::ops::map_async_op_fallible::h8e82c55b669cf249
                               at /home/container/.cargo/registry/src/index.crates.io-6f17d22bba15001f/deno_core-0.294.0/runtime/ops.rs:60:19
  24:     0x555555d5a6c4 - brioche_core::script::op_brioche_bake_all::op_brioche_bake_all::slow_function_impl::h866d23a13198d95b
                               at /workspace/crates/brioche-core/src/script.rs:187:1
  25:     0x555555d5a6c4 - brioche_core::script::op_brioche_bake_all::op_brioche_bake_all::v8_fn_ptr::hfc35036d7274a845
                               at /workspace/crates/brioche-core/src/script.rs:187:1
  26:     0x5555577db6df - Builtins_CallApiCallbackGeneric
fatal runtime error: failed to initiate panic, error 5
Aborted

Box::pin(future)
_requested_module_type: deno_core::RequestedModuleType,
) -> deno_core::ModuleLoadResponse {
deno_core::ModuleLoadResponse::Sync(self.load_module_source(module_specifier))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before that, we needed to pin a future here, since the function load() was only accepting an async result. We can now use a synchronous result. It let us remove this unnecessary async stuff.

@jaudiger
Copy link
Contributor Author

I'm suspecting this issue: denoland/deno_core#812 on deno_core side. Since Tokio is configured to execute a multithreaded context, and core maintainers of Deno as confirmed, the library doesn't like to be called in multiple threads.

@jaudiger
Copy link
Contributor Author

I did the upgrade this morning to deno_core 0.296.0 which deprecates the usage of trait SourceMapGetter in favor of ModuleLoader: denoland/deno_core#823

It saves us a clone of the BriocheModuleLoader during initializing the JsRuntime.

@kylewlacy
Copy link
Member

Hmm, based on the exception and looking at the deno_unsync crate here: https://github.com/denoland/deno_unsync/blob/d61882969f8b69e1ff32678abb2fb187bd1a3afb/src/task.rs#L56-L58

...it looks like it truly depends on a single-threaded runtime as a safety requirement 🙁

I think the fix will be to use a single-threaded Tokio runtime just to interact with deno_core, then to use channels to communicate with the other runtime. The Tokio docs make it clear that at least tokio::sync::mpsc channels can be used between different runtimes: https://docs.rs/tokio/1.38.1/tokio/sync/mpsc/index.html#multiple-runtimes

Later today, I can take a look at trying to get something working as a proof-of-concept at least

@bartlomieju
Copy link

Hmm, based on the exception and looking at the deno_unsync crate here: https://github.com/denoland/deno_unsync/blob/d61882969f8b69e1ff32678abb2fb187bd1a3afb/src/task.rs#L56-L58

...it looks like it truly depends on a single-threaded runtime as a safety requirement 🙁

I think the fix will be to use a single-threaded Tokio runtime just to interact with deno_core, then to use channels to communicate with the other runtime. The Tokio docs make it clear that at least tokio::sync::mpsc channels can be used between different runtimes: https://docs.rs/tokio/1.38.1/tokio/sync/mpsc/index.html#multiple-runtimes

Hey, Bartek from Deno here. Yes, deno_core can only be used from a single-threaded runtime and it stems from the fact that the JsRuntime needs to be pinned to a specific thread (in turn because of how types in rusty_v8 dictate that). Setting up a pair of channels is a good solution here - it's essentially how Deno implements Web Worker API

Squashed commit of the following:

commit 960fc46
Author: Kyle Lacy <[email protected]>
Date:   Tue Aug 6 01:03:14 2024 -0700

    Add some comments

commit ee030ea
Author: Kyle Lacy <[email protected]>
Date:   Tue Aug 6 00:46:34 2024 -0700

    Combine the different Deno task types into new `RuntimeBridge` type

commit d1a73b7
Author: Kyle Lacy <[email protected]>
Date:   Tue Aug 6 00:02:47 2024 -0700

    Update task runners to spawn new Tokio tasks to handle messages

commit cefb0ac
Author: Kyle Lacy <[email protected]>
Date:   Mon Aug 5 23:43:38 2024 -0700

    Fix warnings

commit 55d7b86
Author: Kyle Lacy <[email protected]>
Date:   Mon Aug 5 23:07:03 2024 -0700

    Update `lsp` module to use separate Tokio runtime for Deno

commit f31e5c6
Author: Kyle Lacy <[email protected]>
Date:   Mon Aug 5 22:12:58 2024 -0700

    Update `check` module to se separate Tokio runtime for Deno

commit fae9f7a
Author: Kyle Lacy <[email protected]>
Date:   Sun Aug 4 21:59:30 2024 -0700

    Fix deadlock when resolving promises

commit eda87b0
Author: Kyle Lacy <[email protected]>
Date:   Thu Aug 1 02:40:15 2024 -0700

    [WIP] Use second runtime with channels for Deno
@kylewlacy kylewlacy marked this pull request as ready for review August 7, 2024 06:57
Copy link
Member

@kylewlacy kylewlacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super happy to finally see this cross the finish line! 🎉

@kylewlacy kylewlacy merged commit a06c8f4 into brioche-dev:main Aug 7, 2024
5 checks passed
@jaudiger jaudiger deleted the deno-update-crates branch August 19, 2024 04:57
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 this pull request may close these issues.

Upgrade Deno Core and Rust toolchain
3 participants