-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[pigeon]: Support of modern asynchronous api for Swift and Kotlin #8341
base: main
Are you sure you want to change the base?
Conversation
@@ -25,6 +25,16 @@ private class PigeonApiImplementation: ExampleHostApi { | |||
} | |||
completion(.success(true)) | |||
} | |||
|
|||
func sendMessageModernAsync(message: MessageData) async throws -> Bool { |
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.
you can just overload the function name func sendMessage(...)
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 don't think that I understood.
It's generated Host API code. We could not overload functions in Dart even if we do in Swift
@@ -25,6 +25,16 @@ private class PigeonApiImplementation: ExampleHostApi { | |||
} | |||
completion(.success(true)) | |||
} | |||
|
|||
func sendMessageModernAsync(message: MessageData) async throws -> Bool { | |||
try? await Task.sleep(nanoseconds: 2_000_000_000) |
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.
is this used for unit test? we shouldn't wait for so long as it slows down the tests
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 is used in example app for demonstration purposes
I am not sure if it is used in any tests. I could remove this code if it is necessary
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.
Removed Task.sleep
Task { | ||
do { | ||
let result = try await api.sendMessageModernAsync(message: messageArg) | ||
DispatchQueue.main.async { |
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.
you can do Task { @MainActor }
here.
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.
you can do
Task { @MainActor }
here.
Do you mean:
sendMessageModernAsyncChannel.setMessageHandler { message, reply in
let args = message as! [Any?]
let messageArg = args[0] as! MessageData
Task {
do { @MainActor
let result = try await api.sendMessageModernAsync(message: messageArg)
reply(wrapResult(result))
} catch {
reply(wrapError(error))
}
}
}
Will it lead to executing api.sendMessageModernAsync
on main thread that could block it?
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.
No, i meant you can replace DispatchQueue.main.async {}
with Task { @MainActor}
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.
No, i meant you can replace
DispatchQueue.main.async {}
withTask { @MainActor}
What's the point/profit?
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.
The purpose of swift concurrency is to replace GCD. So since you are using swift concurrency here, there's no reason to use GCD at all.
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.
GCD
Okay I get it.
I am not very familiar with swift concurrency actually. Is that what you meant?
sendMessageModernAsyncChannel.setMessageHandler { message, reply in
let args = message as! [Any?]
let messageArg = args[0] as! MessageData
Task {
do {
let result = try await api.sendMessageModernAsync(message: messageArg)
await Task { @MainActor in
reply(wrapResult(result))
}
} catch {
await Task { @MainActor in
reply(wrapError(error))
}
}
}
}
Maybe we could use MainActor.run
instead? I think it is more readable.
sendMessageModernAsyncChannel.setMessageHandler { message, reply in
let args = message as! [Any?]
let messageArg = args[0] as! MessageData
Task {
do {
let result = try await api.sendMessageModernAsync(message: messageArg)
await MainActor.run(){
reply(wrapResult(result))
}
} catch {
await MainActor.run(){
reply(wrapError(error))
}
}
}
}
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.
In your first chunk of code, don't put await
in front of Task
.
For this particular case, you can use MainActor.run { ... }
, since reply
doesn't require an async context.
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.
@hellohuanlin done
@@ -222,6 +223,30 @@ class ExampleHostApiSetup { | |||
} else { | |||
sendMessageChannel.setMessageHandler(nil) | |||
} | |||
let sendMessageModernAsyncChannel = FlutterBasicMessageChannel( |
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 we just reuse the channel?
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.
How?
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 point me to where this is used? im wondering if you can just use the original channel
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.
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.
Maybe i misunderstood how pigeon is used. Just a strawman - can we share the same channel for both old and new ways of doing things?
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.
The way we use channels will not differ at all either "new way" or "old". We just can not use two handlers for one channel at the same time.
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 we have old api to be the wrapper of the new api?
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.
Could you provide code sample of what you mean?
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.
nvm, i misunderstood what this code is doing.
fdb7254
to
a8df692
Compare
a8df692
to
583d6d7
Compare
@hellohuanlin @LouiseHsu @tarrinneal CI check fails:
Do I need to bump version in |
let messageArg = args[0] as! MessageData | ||
Task { | ||
let result = await api.sendMessageModernAsync(message: messageArg) | ||
await MainActor.run { |
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.
Why doesn't the old version switch thread? Is setMessageHandler
's callback guaranteed to be called on main thread?
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.
The old version uses the callback approach that doesn't introduce asynchronous things like task so there was no need to switch to main to reply via channel
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.
Is
setMessageHandler
's callback guaranteed to be called on main thread?
I guess so as it is platform channel way of communication
Have found implementation where I see dispatch_async(dispatch_get_main_queue()
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.
After marking the func definition as @MainActor
discussed here, here we can just do:
Task { @MainActor in
let result = await api.send...
reply(wrapResult(result))
}
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.
After marking the func definition as
@MainActor
discussed here, here we can just do:Task { @MainActor in let result = await api.send... reply(wrapResult(result)) }
No problem it could be easily done.
But could you explain to me how it would work?
If we mark the whole function with @mainactor doesn't that mean that some expensive async/await function call could potentially be called at main?
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.
If we mark the whole function with @mainactor doesn't that mean that some expensive async/await function call could potentially be called at main?
The API contract is that this function will be called on main. The old API is also called on main and this does not change that. Marking it as @MainActor
will allow compiler to enforce the implementor of this API to only use API that's safe to be used on main thread. That's one of the benefit of swift concurrency.
An expensive aync/await call inside this function will be called on the thread specified by that call. For example,
@MainActor
func sendMessageModernAsync() {
let foo = await myActor.getExpensiveFoo()
}
Here getExpensiveFoo
will be called under the async context of myActor
, rather than that of the main actor.
@@ -25,6 +25,10 @@ private class PigeonApiImplementation: ExampleHostApi { | |||
} | |||
completion(.success(true)) | |||
} | |||
|
|||
func sendMessageModernAsync(message: MessageData) async -> Bool { |
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 just looked at the code again - it looks like the contract of pigeon is that the API (e.g. sendMessage
) should be called on main thread. If that's the contract we want to preserve, we should mark the async API as main actor.
@MainActor
func sendMessageModernAsync(message: MessageData) async -> Bool {}
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.
The only thing that should be called on the main thread is dispatching messages via channel
https://docs.flutter.dev/platform-integration/platform-channels#jumping-to-the-main-thread-in-ios
If we mark the whole function with @MainActor
doesn't that mean that some expensive async/await function call could potentially be called at main?
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.
iiuc both way of communication needs to be on main thread.
The link you add here is calling from platform to dart side. This is the opposite side communication, which needs to be on main as well.
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.
Actually it is the same from the point of dispatching reply via channel, be it call from platform to dart or replying to call from dart
@modernAsync
ModernAsync
annotation withSwiftModernAsynchronousOptions
to specify if method throwsResolves #123867, resolves #147283
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.