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

Blocking task queue without specialized code #2040

Merged

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Mar 19, 2024

When discussing #1837, @mhammond asked me a great question: should blocking task queues be UniFFI's responsibility? This is my attempt to see what would happen if we decided it wasn't.

The code here implements more-or-less the same feature as #1837, but without changing any UniFFI code. Instead it leverages async callback methods to implement it.

Benefit of this approach:

  • No new code needed
  • No new features to commit to supporting
  • Somewhat more flexible. With Async blocking task queue #1837, Python users need to use futures.executor.ThreadPoolExecutor. With this approach, they can use whatever technology they want.

Downsides of this approach:

  • Tasks need to be 'static and cannot use references from their local scope. This makes the code somewhat awkward. Here's similar code in the other PR, note that it could stay basically the same even it used &self or any other reference.
  • It requires users to write boilerplate code. Maybe something like this could be rolled into uniffi or a support library.
  • It requires locking a Mutex and allocating an Arc.

So, should we try to implement #1837 or not? My feeling is that it's not clear, so for 0.27 maybe we should just merge this one and let that one hang out for a while.

@bendk bendk added the v0.27 Issues/PRs for the 0.27 release label Mar 19, 2024
@bendk bendk requested a review from a team as a code owner March 19, 2024 18:27
@bendk bendk requested review from jeddai and removed request for a team March 19, 2024 18:27
@bendk bendk force-pushed the blocking-task-queue-without-specialized-code branch from e6c7523 to f779250 Compare March 19, 2024 18:47
@Sajjon
Copy link
Contributor

Sajjon commented Mar 20, 2024

The link to the "boilerplate code" seems broken, the commit does not exist?

@bendk
Copy link
Contributor Author

bendk commented Mar 20, 2024

The link to the "boilerplate code" seems broken, the commit does not exist?

I think it should work now.

@bendk bendk changed the title Updating the async API client example Blocking task queue without specialized code Mar 20, 2024
Copy link
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Benefit of this approach:
No new code needed
No new features to commit to supporting
Somewhat more flexible.

IMO, that makes this a bit of a no-brainer.

With
#1837, Python users need to use futures.executor.ThreadPoolExecutor. With this approach, they can use whatever technology they want.

TBH, this is my main concern about #1837 - I'm really not sure we are making correct assumptions there, which is best highlighted by this statement and example.

We don't really have a great actual use-case for #1837 yet, and I think it's fine that the first app with a real use-case needs to do some hacky things like that example - it means we can then learn from actual experience and that can help inform whether the approach we are taking there makes sense. Further, there also seems a slight risk that the correct approach might not be a one-size-fits-all - different bindings might have different requirements - ie, the approach in that PR might be perfect for (say) Kotlin, but not at all correct for (say) golang, which might mean consumers end up needing to avoid that mechanism anyway.

So I'm in favor of landing this and keeping #1837 on the back-burner until we get more signals indicating it's a good approach.

Maybe add a new README to the example explaining how it's an experimental, aspirational approach to blocking task queues to help inform how uniffi might better support it natively, with a link to that PR?

@bendk bendk force-pushed the blocking-task-queue-without-specialized-code branch from f779250 to 46976f9 Compare March 23, 2024 15:33
Added some code to show how you can run blocking Rust code inside a
foreign task queue.
@bendk bendk force-pushed the blocking-task-queue-without-specialized-code branch from 46976f9 to 1fbfde3 Compare March 23, 2024 15:34
@bendk
Copy link
Contributor Author

bendk commented Mar 23, 2024

Yeah, that seems right to me. I just added a README. I'll merge this one and remove the 0.27 label from #1837.

@bendk bendk merged commit 17482fa into mozilla:main Mar 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v0.27 Issues/PRs for the 0.27 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants