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.
There was a problem hiding this comment.
Any further thoughts on this one? I'm mainly wondering if my analysis of the custom couroutine scope in the comment above is correct. That's the part I'm least sure about.
RE IO-bound tasks vs other kinds of tasks: Would you use a different Dispatcher for the two cases? AFAICT, the current code always uses Dispatchers.IO, but if you want more flexibility we can build it in, but it's hard to propose something without more concrete details.
|
|
||
| ```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.
There was a problem hiding this comment.
Yes, after reading more closely, bridging back to Swift Concurrency feels like it'll be delicate if we use GCD here. 🤔
Can we instead use Swift Concurrency here do you think @Cramsden ? Like you said, for that we need to know whether this work is always guaranteed to complete first. At the SwiftRockies Concurrency workshop we discussed this a bit with Matt and he suggested a custom actor with a custom executor, which uses a custom thread. He also mentioned not to worry about starving the thread pool unless we have work that is not finishing or evidence of a problem. 🤔
So, I have two questions for the Rust side (cc @bendk):
- Can we guarantee all calls will complete?
- Must all work be serialized on a single thread?
There was a problem hiding this comment.
Can we guarantee all calls will complete?
Yes, with the caveat that buggy code might not complete.
Must all work be serialized on a single thread?
No, in fact you might want a few threads for this work. The only thing that's important is that it's a background thread, not the main thread and not one of the non-blocking Swift Concurrency threads.
There was a problem hiding this comment.
| func run(task: () -> ()) { | ||
| queue.async { | ||
| task() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Yes, after reading more closely, bridging back to Swift Concurrency feels like it'll be delicate if we use GCD here. 🤔
Can we instead use Swift Concurrency here do you think @Cramsden ? Like you said, for that we need to know whether this work is always guaranteed to complete first. At the SwiftRockies Concurrency workshop we discussed this a bit with Matt and he suggested a custom actor with a custom executor, which uses a custom thread. He also mentioned not to worry about starving the thread pool unless we have work that is not finishing or evidence of a problem. 🤔
So, I have two questions for the Rust side (cc @bendk):
- Can we guarantee all calls will complete?
- Must all work be serialized on a single thread?
| getStorage().add(entry.toLoginEntry()).toLogin() | ||
| ``` | ||
|
|
||
| * Swift would have similar changes. |
There was a problem hiding this comment.
It would be nice to see an example for the Swift side too maybe! I'm not familiar enough with the Android side/Kotlin to grok this immediately.
Here's the current Swift code for adding a LoginEntry:
open func add(login: LoginEntry) throws -> Login {
return try queue.sync {
try self.store.add(login: login)
}
}
There was a problem hiding this comment.
Great question and I'm not sure the answer, maybe we can work it out. From what I can tell, there's a few layers of complexity around that add() function that I think we can remove, but I'm not sure what the final result should be.
Here's how I think it works right now:
- LoginsStorage.add() gets wrapped by RustLogins.addLogin(). That runs the function inside a
queue.asyncclosure, and inputs acompletionHandlerto return the result - PasswordManagerViewModule.save() calls RustLogins.addLogin, sending a closure that handles the result. It also has its own
completionHandler - Finally, top-level functions like PasswordManagerListViewController call
PasswordManagerViewModule.save()
If the Rust add function was async, then I think we could refactor that code to something like this (apologies for my bad Swift):
class RustLogins {
public async func addLogin(login: LoginEntry) -> Login throws {
// getStoredKey refactored to not need a completion handler.
// Does it also need to be async?
// BTW, do we need to call this at all? It doesn't seem like the key is ever used
let key = try self.getStoredKey()
// I think this can replace the old `guard self.isOpen` code
if let storage = self.storage {
// One benefit is the return value never null
return try await storage.add(login: login)
} else {
throw LoginsStoreError.UnexpectedLoginsApiError(reason: "Database is closed")
}
}
}
class PasswordManagerViewModule {
// Save now either returns the login ID or throws. Before it called the completion handler
// with either the login ID or an error message, which doesn't seem useful to me
public async func save(loginRecord: LoginEntry) -> String throws {
let encrypted = try loginProvider.addLogin(login: loginRecord)
self.sendLoginsSavedTelemetry()
return encrypted.id
}
}
class PasswordManagerListViewController {
@objc
func presentAddCredential() {
// Assuming, we don't refactor `pressedAddPassword`, we still need a completion
// handler for that result
let completion: (LoginEntry) -> Void = { [weak self] record in
// Since the completion handler is sync, we need a bridge to call async code
// I'm not sure what the best way to do this is, maybe we just construct a Task?
Task {
do {
self?.viewModel.save(loginRecord: record)
// Since we're in an async task, we can't probably shouldn't call
// our sync methods.
// This could be refactored somehow, but for now let's just use
// `DispatchQueue.main.async` like before
DispatchQueue.main.async {
self?.loadLogins()
self?.tableView.reloadData()
}
} catch {
// You could put error handling here or just ignore the error.
// The previous code reloaded the data which doesn't seem right to me
}
}
}
sendLoginsManagementAddTappedTelemetry()
coordinator?.pressedAddPassword(completion: completion)
}
}Each new version seems significantly simpler to me than the old one. I don't think you need to do all that refactoring at once though. Wherever you decide to stop refactoring, you can keep the function sync and use Task to call the async function.
- Does that seem right to you?
- Is
Taskthe right way to call async code from a sync function?
There was a problem hiding this comment.
Thanks for the detailed walkthrough @bendk ! Sorry, I didn't expect it to be quite so much work for you!
Wherever you decide to stop refactoring, you can keep the function sync and use Task to call the async function. [...] Is Task the right way to call async code from a sync function?
The short answer is "yes", but the longer answer is that we should probably take a slightly different approach to avoid the aforementioned refactoring as much as possible. 🤔
A big portion of the Firefox for iOS Client codebase uses GCD (i.e. queues, callbacks) and not modern Swift Concurrency (i.e. async/await). PasswordManagerListViewController is one of them. So, in the case of your example, we would be best not making the RustLogins addLogin method async (which removes the problem of needing the Task later in the presentAddCredential() method).
Given your proposed SwiftWorkQueue implementation (which uses an underlying GCD queue), I suspect our interface between the Swift MozillaRustComponents <> Generated uniFFI calls should exclusively be in GCD and callbacks. 🤔 Does that sound right to you @bendk? (and CC @Cramsden if you want to double check me here too to make sure I'm not way off base)
If that's the case, making a method async or not later is a trivial Client-side detail. We would be able to add a simple wrapping continuation to any calls we want to convert from GCD callbacks to Concurrency async-await.
I think what I'm most curious to understand now is what the API looks like between the Firefox iOS Client and the package we import from AS. The original snippet I posted was from the LoginsStorage.swift file, which is what the RustLogins.swift's addLogin method you dissected calls into.
I just want to check my understanding on that LoginsStorage code:
open func add(login: LoginEntry) throws -> Login {
return try queue.sync { // --> [A]
try self.store.add(login: login). // --> [B]
}
}
Am I correct in understanding that:
- [A] This
queue.syncis how Firefox for iOS Client currently manages the threading of the rust calls. Is this the async bridge we're proposing to move in this ADR? - [B] This is the call into the uniFFI code (e.g.
return try FfiConverterTypeLogin_lift()). With this proposal, would we instead call a method wrapped by the new SwiftWorkQueue, which in turn wraps this uniFFI call?
I might be completely off base here so I just wanted to make sure I'm on the same page. 😅
There was a problem hiding this comment.
[A] is really mysterious to me. AFAICT, it doesn't have any benefit -- it still needs to be wrapped in queue.async call the next level up. It also spins up an extra thread. My best guess is this is leftover code from a refactor or code that was copy-and-pasted without thinking it all though.
I may be misunderstanding you on [B], but the way I see it everything would be in the UniFFI call: the top-level add method wraps an internal Rust method with SwiftWorkQueue and the whole thing is exposed by UniFFI. If we kept the current proposal and you wanted to keep GCD callbacks and make the changes as small as possible then I think it would look something like this:
class LoginsStorage {
open func add(
login: LoginEntry,
completionHandler: @escaping @Sendable (Result<Login?, Error>) -> Void)
{
Task {
do {
let login = try await self.store.add(login: login)
completionHandler(.success(login))
} catch {
completionHandler(.failure(error))
}
}
}
}
Alternatively, maybe we don't need LoginsStorage at all and the change could be made at the level of RustLogins.addLogin. All LoginsStorage seems to be doing is the wrapping things in queue.sync and I don't see what that's accomplishing.
For the ADR, I'm thinking I can suggest something like the snippet above and mention that we don't know exactly how the Swift code should be refactored.
Stepping back a bit. If you really want to keep with the GCD + completionHandler approach, then there's an argument not to use async Rust at all. Maybe UniFFI could wrap things such that the Rust code stayed sync, but it was scheduled in a queue and called the completionHandler at the end. The advantage is you don't need the Task to wrap the async Rust function, but there's a lot of disadvantages: a) if you wanted to switch add() to be async in the future, there's no path to do that b) I'm not sure how to implement the system and c) I think Android does want suspend functions and it seems hard to integrate both of those things. The only downside of constructing the Task that I see is it might be slightly less efficient, but that seems okay to me.
There was a problem hiding this comment.
Hi again @bendk, I had a chance to chat with @Cramsden today. I have a better understanding of where we're at now (I was missing some context earlier).
Regarding [A], I also find this code pretty mysterious. I think you're likely right that it could be cleaned up.
Regarding [B], Carson and I are going to come to the AS Sync tomorrow in hopes of talking through this a bit more in person. We have a few questions from our discussion today. It sounds like there's no reason not to make all the synchronous blocking rust calls surface as async on the Swift side, so you can ignore my earlier ramblings about that (I misunderstood the level we were solving for; sounds like we'll still be queuing work on the Client side one way or another).
As a preview, some of the questions we'll bring to the meeting:
- Do we use
SwiftWorkQueueonly for blocking synchronous rust calls so we can control the threading from the Swift Client? - How do we define where work is happening from the Client side? (e.g. how do we choose the thread, and can we choose a thread per module, like logins vs. places)
- Will simple (non blocking) synchronous work remain unchanged (i.e. no
async)?
|
Hi all, thanks for all the feedback on this one. The official close date was today, but there's still a couple open questions. From what I can tell, none of the open questions point to us not making this decision though. I'm thinking we keep this open another week and see if there aren't any new concerns raised or someone points out what I'm missing in the current threads. If not, then we approve/merge -- maybe with additional notes based on the discussions. Is that okay with you all? |
Thanks @bendk, the extra week would be great RE: our discussion thread. I'm going to chat with @Cramsden to confirm my understanding on something and get back to you soon. 🙏 |
Rendered version
Pull Request checklist
[ci full]to the PR title.