[pigeon] Adds support of TaskQueue for Swift #8501
Conversation
feduke-nukem
requested review from
hellohuanlin,
LouiseHsu and
tarrinneal
as code owners
| @@ -851,6 +854,7 @@ if (wrapped == nil) { | |||
| }) { | |||
| return 'try instanceManager.removeAllObjects()'; | |||
| }, | |||
| taskQueueType: TaskQueueType.serial, | |||
There was a problem hiding this comment.
is serial task queue the only supported type? do we support concurrent queue?
There was a problem hiding this comment.
do we support concurrent queue?
We have never had a use case in our own plugins for concurrent queues, AFAIK nobody else has ever requested it, and I would not expect concurrent task queues to be a common use case in platform channels, so currently we have had no reason to implement it.
| indent.writeln( | ||
| 'let $varChannelName = FlutterBasicMessageChannel(name: "$channelName", binaryMessenger: binaryMessenger, codec: codec)'); | ||
| String? taskQueue; | ||
| if (taskQueueType != TaskQueueType.serial) { |
There was a problem hiding this comment.
im confused here - does TaskQueueType.serial mean it's not background queue? If it means it's a main queue, maybe we should rename to serialMainQueue instead.
There was a problem hiding this comment.
Doc states:
/// Handlers are invoked serially on the default thread. This is the value if
/// unspecified.There was a problem hiding this comment.
im confused here - does
TaskQueueType.serialmean it's not background queue?
No, it means that all calls on that API object using the serial task queue will be executed in order, each one completing before the next starts.
As compared with, e.g., a default NSOperationQueue, which will dispatch jobs into a thread pool, and might run everything in parallel on N different threads.
There was a problem hiding this comment.
Handlers are invoked serially on the default thread.
Is the "default thread" referring to main thread?
There was a problem hiding this comment.
No, it's an arbitrary background thread. That's why the API called by the generated code is makeBackgroundTaskQueue.
Calling things serially on the main thread is what platform channels do by default, and what they always did before the creation of the task queue API. The whole point of adding task queues to the channel APIs in the engine was to move them off of the main thread.
There was a problem hiding this comment.
Please forgive my numerous questions, I think I may be missing something.
What you just described sounds very like the .serialBackgroundThread case, rather than the .serial case. Also, the code here is:
if NOT serial { // the only other case is serialBackgroundThread
use makeBackgroundTaskQueue API
}
There was a problem hiding this comment.
What you just described sounds very like the
.serialBackgroundThreadcase, rather than the.serialcase.
Sorry, I forgot that Pigeon for some reason has a task queue enum value that actually means "don't use task queues". I have no idea why it was written that way; it's incredibly confusing.
So yes, it's the main thread and everything I said was wrong.
maybe we should rename to
serialMainQueueinstead
We should probably just remove it, or if we keep it call it the even simpler none, but either way that's out of scope here.
| let result = try api.echoAllTypesTaskQueueBackground(everythingArg) | ||
| reply(wrapResult(result)) | ||
| } catch { | ||
| reply(wrapError(error)) |
There was a problem hiding this comment.
IIUC reply should still be called on main thread.
There was a problem hiding this comment.
Would it be sufficient to add?
DispatchQueue.main.async {
reply(wrapResult(result))
}like it is mentioned in docs
There was a problem hiding this comment.
However, I wonder why it was not done for others generators in the past.
Outputs for methods marked with TaskQueue:
Objective-c:
Java:
Kotlin:
There was a problem hiding this comment.
IIUC
replyshould still be called on main thread.
Please see the second bullet point here.
There was a problem hiding this comment.
IIUC
replyshould still be called on main thread.Please see the second bullet point here.
Sorry i misread your message.
There was a problem hiding this comment.
looking at it again, I must have misunderstood the 3rd bullet point as the reply call. Sorry for the confusion.
There was a problem hiding this comment.
I must have misunderstood the 3rd bullet point as the
replycall
Does it have any implications for the discussion in this PR?
Apologies if I’m missing something.
corrects linux methods' names, windows response, swift redeclaration taskQueue variable
TaskQueuefor Swiftcloses 111512
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or this PR is [exempt from CHANGELOG changes].///).