-
Notifications
You must be signed in to change notification settings - Fork 235
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
Async internal docs #2353
Async internal docs #2353
Conversation
c8b7d79
to
5c1ef38
Compare
Thanks for the quick review, I just pushed one more section that describes an async call at a high-level. @gruberb Does that section makes sense? I was trying to describe this in our meeting 2 weeks ago, but I think the diagram does a better job. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a great read, thank you! What I am a bit struggling with are the terms. But it's already highly valuable to have this @bendk
|
||
### Why not have `rust_future_poll` return a boolean? | ||
|
||
As described above, `rust_future_poll` inputs a callback and an opaque pointer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain (or link) to the definition of an "opaque pointer"?
docs/manual/src/internals/async.md
Outdated
|
||
The issue is that it doesn't work well with the way foreign code's lifetime management. | ||
The opaque pointer is usually either something like an `Arc` or `Box` that holds data about the async call. | ||
When the foreign code passes the pointer to `rust_future_poll`, it temporarily leaks a reference (either an Arc refcount or the Box itself). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean to "leak a reference"?
docs/manual/src/internals/async.md
Outdated
The issue is that it doesn't work well with the way foreign code's lifetime management. | ||
The opaque pointer is usually either something like an `Arc` or `Box` that holds data about the async call. | ||
When the foreign code passes the pointer to `rust_future_poll`, it temporarily leaks a reference (either an Arc refcount or the Box itself). | ||
Then when callback is called, it reconstructs the leaked reference using the pointer, undoing the leak and restoring balance to the universe. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Undoing the leak"?
|
||
If `rust_future_poll` returned `true` to indicate that the future is ready, then the foreign code is in an awkward situation. | ||
It wants to use the async call data that the pointer was referring to, but it just sent a leaked pointer to Rust. | ||
It would have to hold on to a copy of the raw pointer, use that to restore the reference, and assume the callback is never going to be called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raw pointer, opaque pointer, restore the reference.... There are a lot of terms. Could we have an index of terms and their definitions somewhere? The definition doesn't have to come from us!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this idea, but I feel like that's out-of-scope for the async docs. The best we currently have is this doc: https://mozilla.github.io/uniffi-rs/latest/internals/object_references.html. Maybe we could update that soon to also talk about foreign objects, future handles, etc.
docs/manual/src/internals/async.md
Outdated
|
||
## Foreign async callback methods | ||
|
||
The twin to this is async callback/trait interface methods, which allows Rust to make async calls across the FFI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand what "twin" means here, and I like the lighter language, but I am afraid now I have to hold this reference in my head, and not sure if I am right, so I can't focus on what is actually written.
|
||
* The async Rust code runs in a [Future::poll](https://doc.rust-lang.org/std/future/trait.Future.html#tymethod.poll) method, inside the foreign event loop. This code never blocks. | ||
* The sync Rust code runs inside a worker queue where it's okay to block. | ||
* This all combines together to create an async Rust call that's driven by the foreign event loop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know where to put it: But I would love to also talk here about "data". Where is it? Showing the functions and who is calling what is excellent. And I would love to see some form of diagram which explains where does the data live and when does it getting transferred through the FFI etc. If that makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the diagram to do a bit more of this. Initially, I had a very detailed diagram that showed all the FFI calls, but it got extremely busy so I decided to show more of a zoomed out view.
After that, it returns a high-level API response struct to the application. | ||
Furthermore, let's stipulate that the deserialization/validation is slow enough to be considered blocking. | ||
To manage this, the Rust library also depends on a (synchronous) callback interface that runs a task in a worker queue where blocking is allowed. | ||
Here's how the async API call could be handled: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph packs in a bunch. It's a huge ask, so not sure if we should cover this here. But it assumes a lot of the reader, and at the same time, assuming they don't know how this would work. So if someone understands each term used in this paragraph, they don't need to read it. However, the ones you come here to understand it more, are lost I believe.
Can we reference or give an overview of worker queue, where the challenges are inside just one ecosystem, and how this is getting transalted or communicated to the foregin side?
5c1ef38
to
89a2ef4
Compare
89a2ef4
to
ce21ef8
Compare
That was great feedback, thanks! I updated the doc in a few places to address the concerns. I think there's still more work to be done, but I'm not sure how to do it at this point. I'm definitely open to suggestions. WDYT? |
I think it's in a really great state! The diagrams and explanations helped a lot!! We can always iterate! |
I've been meaning to document the async internals for a while now, here's my first draft.