Skip to content
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

Windows: Client Wrapper: Expose TaskRunner #31134

Closed
wants to merge 1 commit into from

Conversation

jnschulze
Copy link
Member

@jnschulze jnschulze commented Jan 28, 2022

This PR adds an interface for posting tasks back to the main thread on Windows.

Example:

// View is flutter::FlutterView*

view->GetPlatformTaskRunner()->PostTask([]() {
  // This runs on the main thread.
});

See flutter/flutter#79213

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@jnschulze
Copy link
Member Author

cc @stuartmorgan

@stuartmorgan
Copy link
Contributor

stuartmorgan commented Jan 29, 2022

Assuming we actually want to expose this functionally (which hasn't actually been decided): why is this so complex? If this isn't going to be a fully-fledged runner with things like delayed execution, why not just expose a single function to run a callback on the main thread? I don't see why the step of checking the current thread needs to be externalized, for instance.

@jnschulze
Copy link
Member Author

Thanks for your feedback.
Sure, this is slightly more complex than having a single method in FlutterView, for example, but I want to keep it extendable.

When is it going to be decided if this should be exposed?

My primary use case is having the possibility of using EventChannels from background threads.

@stuartmorgan
Copy link
Contributor

When is it going to be decided if this should be exposed?

@cbracken should review and decide.

My primary use case is having the possibility of using EventChannels from background threads.

You already do, via the hidden window mechanism mentioned in the issue; this would just be a convenience.

But the other way to make that use case easier would be to make EventChannel thread-safe, which would be in keeping with the recent work on platform channel responses. /cc @gaaclarke

@gaaclarke
Copy link
Member

My primary use case is having the possibility of using EventChannels from background threads.

Yea, we've enabled that for iOS and Android, see the TaskQueue API. I'm in the process of cleaning up the documentation but the information is still good here: https://docs.flutter.dev/development/platform-integration/platform-channels?tab=type-mappings-kotlin-tab#channels-and-platform-threading (the cleanup is just mentioning that iOS is not yet available on stable and formatting).

@cbracken cbracken self-requested a review March 14, 2022 19:18
@jnschulze jnschulze requested a review from moko256 May 26, 2022 17:40
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

We discussed this in the chat on Discord over the last week; I've given it a quick look and my current thinking on this is similar to Stuart's.

Looking at Windows APIs in general, it seems that:

  1. There isn't the same idea of a 'main' UI thread you're expected to work from like on many other platforms. e.g. Related StackOverflow question.
  2. That posting messages cross-thread seems to be commonly supported. e.g. PostMessage.

Since the goal is to enable EventChannel access from other threads, we should probably bite the bullet and make our APIs safe for calling from any thread on Windows and enable that internally via TaskRunner rather than exposing it directly.

@cbracken
Copy link
Member

On further discussion, I do think this is something we want to support for desktop for consistency with the mobile embedders, but needs a pass from @gaaclarke who is the subject-matter expert on this API.

/cc @gaaclarke for a pass at this. Tracking issue is flutter/flutter#93945. Aaron is also working on related work on the Dart side, so we may want to make sure that we're consistent with whatever refactoring you have in the works there.

@gaaclarke
Copy link
Member

It sounds like the impetus for this change is wanting to respond to platform channel messages on background threads since @jnschulze said "My primary use case is having the possibility of using EventChannels from background threads".

If that is the case I think it's better that we implement this the same way we did for mobile and make the response API thread-safe so it can be invoked from any thread (flutter/flutter#93945). I'm a bit concerned about introducing an API that is meant to be used by plugins that will be hard to rip out in the future.

I'm sorry that that feature hasn't come as fast as I would have liked but in the meantime you can use the logic in this PR individually, it just won't be part of the Flutter API. We might get the feature as a result of implementing Isolate Platform Channels (flutter/flutter#13937) too which is going to happen sooner for windows. That will require making the message dispatcher to be thread-safe too.

@stuartmorgan
Copy link
Contributor

for consistency with the mobile embedders

How so? We don't have an API for this on mobile (since the OS provides one).

@cbracken
Copy link
Member

cbracken commented Aug 24, 2022

for consistency with the mobile embedders

How so? We don't have an API for this on mobile (since the OS provides one).

Perhaps I misunderstood @gaaclarke when we spoke about this yesterday. I was specifically thinking about the makeBackgroundTaskQueue and friends that we implemented for iOS/Android. e.g. this patch. #29147 (docs).

@gaaclarke
Copy link
Member

After looking at this PR more closely, the task queue work is less applicable to what this PR is solving despite the similarities at first blush. There are 2 things that got implemented as part of background platform channels: 1) Allowing you to select which thread a Flutter->Host message will be handled on (previously only the platform thread) 2) Allowing Flutter->Host message responses to be called on any thread (previously had to be called on the platform thread).

The second thing is what is applicable here that solves the same problem without introducing any new API.

@stuartmorgan
Copy link
Contributor

Right, the reason I think we shouldn't do what this PR does, and instead make calling back safe from other threads, is that doing that is consistent with mobile. Prior discussion about that is what was referenced in this part of your comment above:

Since the goal is to enable EventChannel access from other threads, we should probably bite the bullet and make our APIs safe for calling from any thread on Windows and enable that internally via TaskRunner rather than exposing it directly.

@gaaclarke
Copy link
Member

We might get the feature as a result of implementing Isolate Platform Channels (flutter/flutter#13937) too which is going to happen sooner for windows.

I just implemented Isolate Platform Channels for Windows. It turns out that I didn't need to implement background platform channels to implement the feature (f957658). I kept the thread hop to the platform thread early enough that it wasn't needed.

@cbracken
Copy link
Member

I'm going to close this for now. Since @gaaclarke has the most recent context on exposing the ability to handle tasks on background threads, I'd suggest you do some back and forth over Discord to see if his current/upcoming work will resolve this usecase.

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.

4 participants