Conversation
Sushisource
left a comment
Sushisource
left a comment
There was a problem hiding this comment.
Overall the new structure makes good sense to me. A lot of comments around various cleanup though
yuandrew
left a comment
yuandrew
left a comment
There was a problem hiding this comment.
Cool to learn more about the core bridge! Left a few minor comments/questions, deferring to Spencer for approval
mjameswh
force-pushed
the
bridge-refactor
branch
2 times, most recently
from
25c359f to
0f1440c
Compare
| // 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(),)); |
There was a problem hiding this comment.
Clone isn't necessary (clippy should whine about this, if it isn't we've probably got it set up wrong).
There was a problem hiding this comment.
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?
| // 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 |
There was a problem hiding this comment.
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.
mjameswh
force-pushed
the
bridge-refactor
branch
2 times, most recently
from
6948ee6 to
7e8300f
Compare
| @@ -0,0 +1,418 @@ | |||
| # Bridge Layer Coding Norms, Best Practices and Pertinent Knowledge | |||
There was a problem hiding this comment.
WIP - Just ignore for now
mjameswh
force-pushed
the
bridge-refactor
branch
from
633def6 to
5ef703b
Compare
|
Opening a new PR. The PR has grown too much since prior reviews. |
3 participants
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 +
promisifyon 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