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

WIP V3: Refactor RPC #9802

Closed
wants to merge 2 commits into from
Closed

WIP V3: Refactor RPC #9802

wants to merge 2 commits into from

Conversation

alshdavid
Copy link
Contributor

@alshdavid alshdavid commented Jun 17, 2024

This more of an RFC - I'm not sure I'm convinced by the RPC implementation (even though I wrote it initially lol)

In this PR I refactor the RPC implementation to reduce the boilerplate and make it more versatile.

I removed the RpcMessage enum and instead have the RpcHost expose a public send method that uses serde to send/receive types.

On the Rust side:

let return_value: usize = rpc_host.send("event-name", "data-can-be-anything").unwrap();
println!("{}", return_value);  // 42

On the JavaScript side

type MyMessage = HandlerFunc<'message-name', string, number>

rpc.on<MyMessage>('event-name', (data) => {
  console.log(data) // "data-can-be-anything"
  return 42
})

This should make it easier to create external users of the Rpc implementation (like FS and Cache)

Changelog

  • Renamed RpcConnection to RpcWorker
  • Removed the channels from RpcWorker, calling the ThreadSafeFunction directly
  • Sharing the callback init code between RpcHost and RpcWorker
  • Exposing a RpcHost.send() and RpcWorker.send() method that uses serde to convert types in/out of JavaScript
    • Utility function send_serde will automatically typecast
    • Utility function send_with will let you manually map the parameters and return value
  • Removed the RpcHostMessage & RpcConnectionMessage enums
  • Added types to RPC events in the Flow code

Copy link
Contributor Author

@alshdavid alshdavid Jun 18, 2024

Choose a reason for hiding this comment

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

Event names can be shared with JavaScript using:

#[napi]
pub enum FileSystemEvent {
  ReadToString = 1,
  IsFile,
  IsDir,
}

impl FileSystem for RpcFileSystemNodejs {
  fn read_to_string(&self, path: &Path) -> io::Result<String> {
    self.rpc_host.send::<PathBuf, String>(FileSystemEvent.ReadToString, path.to_path_buf())
  }
}
import { FileSystemEvent } from '@parcel/rust'

console.log(FileSystemEvent.IsDir) // 3

Though it might get a bit difficult with clashing events (given this assigns them as numbers). Otherwise we can use strings and add a prefix

type IsFileHandler = HandlerFunc<'fs/is_file', string, boolean>;
type IsDirHandler = HandlerFunc<'fs/is_dir', string, boolean>;

export class FileSystemRpc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel like a function would be a cleaner choice here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so too, though I am wondering if we need to have methods on it - perhaps a .destroy() to detach listeners

@@ -23,7 +22,7 @@ pub struct ParcelNapiBuildResult {}
pub struct ParcelNapiOptions {
pub threads: Option<u32>,
pub node_workers: Option<u32>,
pub fs: Option<JsObject>,
pub use_file_system: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

use_rpc_file_system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, the FileSystem object isn't being passed into Rust from JavaScript. The ParcelV3 public API accepts a FileSystem object which gets mapped in JS to events from the RPC system.

If an FS is supplied to the public options, then we know we are using RPC to delegate calls to the FileSystem so we need to tell the rust-side ParcelNapi to send FS events to JS via RPC.

this.#routes.set(event, callback);
}

callback: any = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

is the any here needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flow wanted this to property to be typed and for some reason it couldn't infer the type from the assignment 🤷

err: any,
id: string,
data: any,
done: any,
Copy link
Contributor

Choose a reason for hiding this comment

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

Done can be typed a little bit more

@@ -17,11 +17,11 @@ use super::RpcWorkerNodejs;
// RpcHostNodejs has a connection to the main Nodejs thread
pub struct RpcHostNodejs {
Copy link
Contributor

Choose a reason for hiding this comment

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

As inverted this ;we'll always suffix trait implementations with the trait name.

This should be NodejsRpcHost

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted

Copy link
Contributor

@yamadapc yamadapc left a comment

Choose a reason for hiding this comment

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

Personally, I still think it's better to use delegation rather than events like this. Current multi-threaded function FS implementation has no boilerplate and should be equivalent for our use.

Ultimately I don't think it'll be too different functionality wise, but it'll be easier to write and has a few benefits as there is no boilerplate.

Some potential use-cases like WASM may not immediately work, but I'd not optimise for those & they can be updated later on.

}
};

#on_event(id: string, data: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can type everything on the file

also could we use camel case for all JS symbols?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I didn't even realize! Haha going between Rust and JavaScript 😅 Maybe we should have a lint rule for JavaScript variable declarations

@yamadapc
Copy link
Contributor

I don't understand why channels would be required on WASM because WASM can't be multi-threaded anyhow.

@devongovett
Copy link
Member

Can we just call different functions directly from rust instead of emitting events? I liked your create_js_thread_safe_method helper.

@alshdavid
Copy link
Contributor Author

Can we just call different functions directly from rust instead of emitting events? I liked your create_js_thread_safe_method helper.

This PR is a bit of an RFC to help visualize & discuss what an events pattern would look like - and I am starting to feel like the more direct approach will be more ergonomic.

The original intention was to have an abstraction for bindings (be it napi, wasm or a whatever) that makes it easy to support external execution environments without needing anything specific within core to change - e.g. an RpcResolver which could be running in napi, wasm, or whatever.

But after applying it, it doesn't seem like there is much merit to this. At least for things like FileSystem. Bindings probably simply need to be specific to the context they are running in. I'm going to spend a bit more time thinking about this problem

@alshdavid alshdavid changed the title V3: Refactor RPC WIP V3: Refactor RPC Jun 18, 2024
@alshdavid
Copy link
Contributor Author

alshdavid commented Jun 18, 2024

WASM can't be multi-threaded anyhow.

You can use a single threaded async runtime on wasm so you can use async channels without blocking the main thread.

There are also approaches to use a web Worker farm with Rayon or Tokio to get true multi-threading working - though the host needs to have some restrictive security headers in order to use threads.

I assume our usage of wasm won't be for serious applications, only for the playground, so it doesn't really matter that much.

@alshdavid
Copy link
Contributor Author

Closing this in favour of calling JS a little more directly from napi

@alshdavid alshdavid closed this Jun 21, 2024
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.

None yet

5 participants