Repository navigation
Conversation
| // Rust code here | ||
| }.await | ||
| } | ||
| ``` |
There was a problem hiding this comment.
I don't think this needs to block the ADR, but I'm happy to bikeshed on this. I agree with Mark that we should be keeping the Rust learning curve as gradual as possible.
dea3bb6 to
8dd3182
Compare
jonalmeida
left a comment
jonalmeida
left a comment
There was a problem hiding this comment.
This largely looks fine to me with a small hold out on the hard-coding of Dispatchers.IO in the KotlinWorkQueue. I originally understood it as a placeholder in the pseudo-code, so I want to make sure that it isn't a specific Dispatcher that the embedder cannot specify.
| ```kotlin | ||
| class KotlinWorkQueue : WorkQueue { | ||
| fun run(task: () -> Unit) { | ||
| withContext(Dispatchers.IO) { |
There was a problem hiding this comment.
The layer above or even this one can have a default of Dispatchers.IO as long as we allow for it to be overridden by the embedder.
The current design would not let me supply my own CoroutineContext where needed (example).
There was a problem hiding this comment.
+1 to that too.
I think we might have some tasks that are long running but not IO bound?
There was a problem hiding this comment.
Thanks for the discussion, I'd love to figure this out. I think there are 2 things here:
You might want to wrap the entire operation in a custom coroutine scope, like in the autopush example where you want to add the exception handler and the supervisor job. I don't the WorkQueue is the right place to do this, since only the blocking parts of the Rust function are going to be scheduled with the work queue. This will often be the entire operation, but not always -- see the potential future work section. Instead, I think you'd need to continue to wrap the Rust call in the custom scope which should be okay, you'd just be wrapping an async function instead of a sync function.
You also might want to schedule the blocking work with a different dispatcher like in the IO bound example. I think there's a couple ways we could handle that:
- Each component inputs its own work queue
- Tasks get a type tag that specifies what kind of work they're doing. IO-bound work gets scheduled with Dispatchers.IO, CPU-bound work gets scheduled with a separate dispatcher.
|
|
||
| ```kotlin | ||
| class KotlinWorkQueue : WorkQueue { | ||
| fun run(task: () -> Unit) { |
There was a problem hiding this comment.
For this future ADR when we think about cancellation, a task could be a Continuation when we can describe what that means at the rust layer so that we can inform the dispatcher that this task is not cancellable, like the sqlite tasks.
I mention this here because it might be worth thinking about the API design for the task if it helps you in the future.
| ```kotlin | ||
| class KotlinWorkQueue : WorkQueue { | ||
| fun run(task: () -> Unit) { | ||
| withContext(Dispatchers.IO) { |
| The Kotlin and Swift implementations are straightforward: | ||
|
|
||
| ```kotlin | ||
| class KotlinWorkQueue : WorkQueue { |
There was a problem hiding this comment.
The name WorkQueue implies to me, that it is a queue of tasks, but I didn't see that in the implementation.
It seems to me more like a "runner" / or "executor" than a "work queue"
There was a problem hiding this comment.
Good point. I like TaskRunner. TaskExecutor could also work, but "executor" sometimes means "async runtime" which could be slightly confusing.
| pub fn add(&self, entry: LoginEntry) -> ApiResult<Login> { | ||
| spawn_blocking(move || { | ||
| // code to do the work | ||
| }) | ||
| } |
There was a problem hiding this comment.
Question: Should add() become async add() now?
One of the goals of this document is to alleviate the sync/async ambiguities to Kotlin/Swift/JS developers.
As a Kotlin developer using these APIs, one of the things that make it unclear to me is that the APIs do not clearly look async, so I have to look at the API whether it's async or not.
With this work, it seems to me that we can now employ async constructs in the function definitions.
This add() function is not a suspend function in Kotlin, and so to me, it could be potentially blocking, and I wouldn't know for sure until I check the implementation.
If add became a suspend function, then I would know for sure that it is suspending, and I would only be able to call it in scenarios where I am prepared for handling asynchronous operations.
There was a problem hiding this comment.
Oops, I just forgot to add the async (and the .await at the end). I agree that seeing the async / suspend keyword is useful as a developers since you have a better idea what's happening inside the function.
8dd3182 to
8b01486
Compare
titooan
left a comment
titooan
left a comment
There was a problem hiding this comment.
Sorry, I didn't intend to "Request changes". Trying to dismiss this request for changes here.
| func run(task: () -> ()) { | ||
| queue.async { | ||
| task() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems fine to me. I guess we might want to leverage a continuation here to make this interface use swift concurrency for callers. I take that back we would want to be extremely thoughtful about where and how we bridge this back to the Swift concurrency world since these calls are blocking. It should still be possible as long as they are guaranteed to complete. @ih-codes what do you think?
There was a problem hiding this comment.
Apologies for the delay, I ran out of time this week but I will read through the ADR on Monday and get back to you with my thoughts.
This branch has not been deployed
7 participants
Rendered version
Pull Request checklist
[ci full]to the PR title.