Skip to content

[pigeon] add modern concurrency support - #12382

Open
tarrinneal wants to merge 5 commits into
flutter:mainfrom
tarrinneal:swift-async-kotlin-suspend
Open

[pigeon] add modern concurrency support#12382
tarrinneal wants to merge 5 commits into
flutter:mainfrom
tarrinneal:swift-async-kotlin-suspend

Conversation

@tarrinneal

Copy link
Copy Markdown
Contributor

Adds support for modern async method styles in swift and kotlin. This is now the default. Also adds a new annotation AsyncCallback that reverts to the old style.

precursor to #11352

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the Pigeon generator to produce native asynchronous signatures (suspend in Kotlin and async throws in Swift) by default for @FlutterApi and @HostApi methods, while introducing a new @asyncCallback annotation to retain callback-style signatures. Feedback on the changes identifies a thread-safety issue in the Swift generator where spawning a Task without isolation can cause the reply callback to execute on a background thread, and suggests isolating the Task to @MainActor when no background queue is specified.

Comment thread packages/pigeon/lib/src/swift/swift_generator.dart Outdated

@stuartmorgan-g stuartmorgan-g left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good at a high level, with the caveat that I have very little experience with the modern concurrency syntax (although certainly the ergonomics in the test plugin look way better). It might be worth making a draft PR that adopts it in one of our plugins and getting a review from Android and iOS folks so they can flag anything that looks unexpected in real use, especially since we are making this the default. (To avoid pulling the entire diff into that PR, you can use a git dependency that points to this branch).

This will also need README updates, both explaining the two modes, and also explaining that only some generators distinguish, and explaining what happens when it's not supported (which IIUC is that the two annotations do the same thing).

Comment thread packages/pigeon/pigeons/core_tests.dart

@stuartmorgan-g stuartmorgan-g left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

I'll leave it up to you if you want to get a draft adoption PR reviewed by platform team folks before landing, or just want to iterate in future PRs if any issues come up that the tests missed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants