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

Async blocking task queue #1837

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

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Nov 9, 2023

Added the BlockingTaskQueue type BlockingTaskQueue allows a Rust closure to be scheduled on a foreign thread where blocking operations are okay. The closure runs inside the parent future, which is nice because it allows the closure to reference its outside scope.

On the foreign side, a BlockingTaskQueue is a native type that runs a task in some sort of thread queue (DispatchQueue, CoroutineContext, futures.Executor, etc.).

Added new tests for this in the futures fixtures. Updated the tests to check that handles are being released properly.

@bendk bendk requested a review from a team as a code owner November 9, 2023 15:50
@bendk bendk requested review from tarikeshaq and removed request for a team November 9, 2023 15:50
/// We start with an assumption: if we notify the waker then this future will be polled when the
/// top-level task is polled next. If a future does not honor this then we consider it a broken
/// future. This seems fair, since that future would almost certainly break a lot of other future
/// code.
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'm pretty sure this is a good assumption, but I'd love a double check on this one.

When I mention "other future code", I was thinking of something like a lock. If your async lock type locks a mutex and notifies the waker, then any intermediate futures must poll your type or else you're in a bad state. Unless I'm missing something, async lock authors just have assume they will get polled and we can too.

On Rust, `BlockingTaskQueue` is a UniFFI type that can safely run blocking code.
It's `execute` method works like tokio's [block_in_place](https://docs.rs/tokio/latest/tokio/task/fn.block_in_place.html) function.
It inputs a closure and runs it in the `BlockingTaskQueue`.
This closure can reference the outside scope (i.e. it does not need to be `'static`).
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'm not sure what this method should be named. I chose execute simply because I hadn't seen it used in other places so it didn't have the baggage of other names. I almost want to name in block_in_place since it matches the semantics of the tokio method. But the "in_place" part feels weird since we don't have a spawn_blocking method to contrast with.

///
/// WARNING: the call to [rust_future_poll] must be scheduled to happen soon after the callback is
/// called, but not inside the callback itself. If [rust_future_poll] is called inside the
/// callback, some futures will deadlock and our scheduler code might as well.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't really related to the PR, but it's something I noticed when trying to refactor some things.

@bendk
Copy link
Contributor Author

bendk commented Dec 13, 2023

Updated this code to not depend on the handles work, because I didn't want that to block this. This means the foreign handle-map code is a bit of a mess, but #1823 should clean that up.

At the same time, I did make it depend on the ffi-vtables branch because that code was extremely useful for implementing Drop for BlockingTaskQueue (I also used it to implement Clone). It would be possible to land this without those changes, but I think it would be much more complicated.

@bendk bendk force-pushed the blocking-task-queue branch 3 times, most recently from 28464e6 to ff1f241 Compare December 13, 2023 22:52
@tarikeshaq
Copy link
Contributor

Removing myself from the review, will also remove myself from auto-assign - if I take more UniFFI work in the future I'll add myself back

@tarikeshaq tarikeshaq requested review from mhammond and removed request for tarikeshaq December 13, 2023 22:56
@bendk bendk force-pushed the blocking-task-queue branch 2 times, most recently from 5432e27 to 0f39a50 Compare December 15, 2023 22:10
@bendk bendk mentioned this pull request Dec 21, 2023
2 tasks
@bendk bendk force-pushed the blocking-task-queue branch 2 times, most recently from 26a9649 to 5a7ee85 Compare March 12, 2024 16:02
@bendk
Copy link
Contributor Author

bendk commented Mar 12, 2024

Rebased this one now that the vtable code has been merged. It's looking pretty good now so I removed the do-not-merge tag.

@bendk bendk changed the title Blocking task queue Async blocking task queue Mar 14, 2024
@bendk bendk added the v0.27 Issues/PRs for the 0.27 release label Mar 14, 2024
@bendk bendk removed the v0.27 Issues/PRs for the 0.27 release label Mar 23, 2024
@bendk
Copy link
Contributor Author

bendk commented Mar 23, 2024

Removed this one from the list of PRs for 0.27 since it seems too early to tell how we should handle it. See #2040 for a discussion.

Added the `BlockingTaskQueue` type BlockingTaskQueue allows a Rust
closure to be scheduled on a foreign thread where blocking operations
are okay. The closure runs inside the parent future, which is nice
because it allows the closure to reference its outside scope.

On the foreign side, a `BlockingTaskQueue` is a native type that runs a
task in some sort of thread queue (`DispatchQueue`, `CoroutineContext`,
`futures.Executor`, etc.).

Updated handlemaps to always start at 1 rather than 0, which is reserved
for a NULL handle.

Added new tests for this in the futures fixtures.  Updated the tests to
check that handles are being released properly.
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.

2 participants