Reapply: Android background platform channels

[gaaclarke]

Reverts #29344

That PR was reverted because the breaking change was too much. We've made it not a breaking change by overloading setMessageHandler and providing a default implementation for makeBackgroundTaskQueue.

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[gaaclarke]

@dnfield I made the changes we discussed. In order to make it easier to review the change I've kept the revert of the revert as a separate commit. That represents the work that is already reviewed.

[dnfield]

We should check if there's already a task queue assigned for this channel and throw if there is one. I'm also trying to figure out if we need a mutex around this somehow.

[dnfield]

I guess it's decorated as @UiThread ...

I'm also wondering if we want to assert that this is called before setting the message handler, so that people won't get first called on the platform thread and then later called on a background thread.

[gaaclarke]

We shouldn't be throwing if it is already set. This follows the same semantics for setMessageHandler. There doesn't need to be a mutex. The only datastructure accessed from 2 threads is messageHandlers and it is a thread-safe datastructure.

[gaaclarke]

I added a test that asserts that you can call setMessageHandler first or setTaskQueue first, both work.

[gaaclarke]

I'm also wondering if we want to assert that this is called before setting the message handler, so that people won't get first called on the platform thread and then later called on a background thread.

Most people are going to be using the channels, not the BinaryMessenger so it's unlikely to be a problem. I thought about this too but the semantics of forcing someone to call setTaskQueue first is ugly. I prefer to enforce something statically or just make it work and not rely on convention like that.

[dnfield]

What happens if you call setMessageHandler first, get an onMethodCall callback on the platform thread, then call setTaskQueue, and get a new onMethodCall from some other thread, and your code in onMethodCall is not thread safe?

Even if 99% of customers will use the MethodChannel interface, we know for sure we have some using this interface directly. We should try to make it difficult/impossible to use incorrectly.

[gaaclarke]

What happens if you call setMessageHandler first, get an onMethodCall callback on the platform thread, then call setTaskQueue, and get a new onMethodCall from some other thread, and your code in onMethodCall is not thread safe?

If you call setMessageHandler, then a couple events later call setTaskQueue you will be changing the thread the handler executes on, and the user explicitly chose to do that, so it's their responsibility to make sure their handler is thread-safe. This would be consistent behavior with how the user is using the interface and is avoidable by calling setMesageHandler and setTaskQueue in the same event.

[gaaclarke]

There is a draft PR that will open the plugins tree after this change lands and is rolled into master: flutter/plugins#4453

[jason-simmons]

Would it work to have a method that sets both the BinaryMessageHandler and the TaskQueue but has a different name from setMessageHandler?

That would preserve backward compatibility with existing uses of setMessageHandler but would avoid the risk of the message handler and the task queue getting into an inconsistent state.

[dnfield]

Would it work to have a method that sets both the BinaryMessageHandler and the TaskQueue but has a different name from setMessageHandler?

That would preserve backward compatibility with existing uses of setMessageHandler but would avoid the risk of the message handler and the task queue getting into an inconsistent state.

I think the option there would be to just add a new overloaded member called setMessageHandler with TaskQueue as a third parameter, and then make the existing one call that with null.

[gaaclarke]

I did have an overloaded version like this:

default void setMessageHandler(channel, handler, taskQueue) {
  setMessageHandler(channel, handler);
  setTaskQueue(channel, handler == null ? null : taskQueue);
}

I removed it because people can override it which makes things messy since they have to call the 2 parameter variant or things break. We also don't want people to start doing verify calls onto it. It is easier just to do the minimal thing to the interface to get what we want.

[gaaclarke]

Would it work to have a method that sets both the BinaryMessageHandler and the TaskQueue but has a different name from setMessageHandler?

If we had another method that had overlapping semantics with setMessageHandler it confuses tests. Do we verify setMessageHandler(channel, handler) or setMessageHandlerAndTaskQueue(channel, handler, taskQueue)? If they do the former and setMessageHandlerAndTaskQueue calls setMessageHandler, that's fine but then changing the implementation could be a breaking change.

That would preserve backward compatibility with existing uses of setMessageHandler but would avoid the risk of the message handler and the task queue getting into an inconsistent state.

We can also add unit tests to reduce the risk of them getting out of sync. I've added those, let me know if you think there is another case we should add.

[jason-simmons]

The issue is whether it should be possible to have a point in time where the message handler is registered but it isn't yet associated with the right task queue.

I'd prefer that the handler and the task queue be set in one API call. Either an overload or a method with a new name would be ok.

Tests that are tightly coupled to the DartMessenger implementation are going to risk breakages whenever DartMessenger changes. If this is a common scenario, then we may need to provide APIs to check whatever these tests are trying to verify (e.g. whether a plugin registered a handler for the expected channel)

[gaaclarke]

@jason-simmons @dnfield I implemented what we discussed. This is no longer a breaking change for anyone because we've provided default implementations for the new methods and maintained the signature for setMessageHandler. PTAL.

[dnfield]

nit: add a comment here (perhaps a TODO) explaining why we're doing this and that it can be simplified after the next stable roll.

[dnfield]

Please link a github issue.

[dnfield]

ditto

[dnfield]

LGTM modulo having a github issue for the TODOs and a nitpick about adding one more todo.