Skip to content

chore: Refactor Bridge #1638

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

mjameswh
Copy link
Contributor

@mjameswh mjameswh commented Mar 5, 2025

What changed

Major refactor of Core bridge.

  • Removed reactor event loop on Runtime and Worker; relying on Tokio's event loop. This greatly simplify the bridge code.

  • Use Neon's Deferred/Promise support rather than callback functions + promisify on the TS side.

  • Split code to files.

  • Overall, align TS' bridge with how we do things in other Core-based SDKs. That should make it easier to port features across Core-based SDKs in the future.

  • Fix some runtime-initialization hack related to metrics setup, which was in the way of implementing custom metrics support.

Things to know

  • I will continue this refactoring effort, but will do so in distinct PRs.

@mjameswh mjameswh changed the title [WIP] Refactor Bridge chore: Refactor Bridge Mar 11, 2025
@mjameswh mjameswh marked this pull request as ready for review March 14, 2025 16:51
@mjameswh mjameswh requested a review from a team as a code owner March 14, 2025 16:51
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Overall the new structure makes good sense to me. A lot of comments around various cleanup though

Copy link
Contributor

@yuandrew yuandrew left a comment

Choose a reason for hiding this comment

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

Cool to learn more about the core bridge! Left a few minor comments/questions, deferring to Spencer for approval

@mjameswh mjameswh force-pushed the bridge-refactor branch 2 times, most recently from 25c359f to 0f1440c Compare April 3, 2025 20:11
// VALIDATE: Do we need a memory barrier here to ensure that js_counterpart and aborted are coherent?
if let Some(js_counterpart) = self.js_counterpart.get() {
// Fire and forget
let _ = js_counterpart.abort.call_on_js_thread((reason.clone(),));
Copy link
Member

Choose a reason for hiding this comment

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

Clone isn't necessary (clippy should whine about this, if it isn't we've probably got it set up wrong).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, Clippy wasn't complaining. Something was apparently misconfigured, I can't figure what.

I got it working by adding clippy directives at the top of lib.rs, but I see no other of our project does that. Suggestions?

Comment on lines +126 to +127
// If the Rust controller has already been aborted, call the JS abort callback now
// VALIDATE: Do we need a memory barrier here to ensure that js_counterpart and aborted are coherent?
// I assume that the get() call ensures visibility of the js_counterpart
Copy link
Member

Choose a reason for hiding this comment

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

I believe this works, but it's really hard to be sure.

Another option, that I thiiiink works, is to use a mutex that protects both things. The only situation you need to deal with is having the JS call not happen under the lock - but that's easy, because it's already in an Arc. So, just clone it under the lock and then call it outside of it. We already accept that double-calls are fine.

That might make it a lot easier to reason about.

@@ -0,0 +1,418 @@
# Bridge Layer Coding Norms, Best Practices and Pertinent Knowledge
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WIP - Just ignore for now

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.

3 participants