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.

Sign up for GitHub

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

Jump to bottom

Reapply: Android background platform channels #29346

Merged
merged 4 commits into from
Oct 27, 2021
Merged

Reapply: Android background platform channels #29346

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
47409d5
Revert "Revert "Android Background Platform Channels (#29147)""
gaaclarke Oct 26, 2021
4d4cc68
Made this PR less of a breaking change by keeping setMessageHandler's…
gaaclarke Oct 26, 2021
4622a04
switched to overloading setMessageHandler
gaaclarke Oct 27, 2021
d59fb6b
updated comments
gaaclarke Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions shell/platform/android/io/flutter/embedding/engine/dart/DartExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public DartExecutor(@NonNull FlutterJNI flutterJNI, @NonNull AssetManager assetM
this.flutterJNI = flutterJNI;
this.assetManager = assetManager;
this.dartMessenger = new DartMessenger(flutterJNI);
dartMessenger.setMessageHandler("flutter/isolate", isolateChannelMessageHandler, null);
dartMessenger.setMessageHandler("flutter/isolate", isolateChannelMessageHandler);
this.binaryMessenger = new DefaultBinaryMessenger(dartMessenger);
// The JNI might already be attached if coming from a spawned engine. If so, correctly report
// that this DartExecutor is already running.
Expand Down Expand Up @@ -200,10 +200,16 @@ public void send(
@Override
@UiThread
public void setMessageHandler(
@NonNull String channel,
@Nullable BinaryMessenger.BinaryMessageHandler handler,
@Nullable TaskQueue taskQueue) {
binaryMessenger.setMessageHandler(channel, handler, taskQueue);
@NonNull String channel, @Nullable BinaryMessenger.BinaryMessageHandler handler) {
binaryMessenger.setMessageHandler(channel, handler);
}

/** @deprecated Use {@link #getBinaryMessenger()} instead. */
@Deprecated
@Override
@UiThread
public void setTaskQueue(@NonNull String channel, @Nullable TaskQueue taskQueue) {
binaryMessenger.setTaskQueue(channel, taskQueue);
}
// ------ END BinaryMessenger -----

Expand Down Expand Up @@ -427,10 +433,14 @@ public void send(
@Override
@UiThread
public void setMessageHandler(
@NonNull String channel,
@Nullable BinaryMessenger.BinaryMessageHandler handler,
@Nullable TaskQueue taskQueue) {
messenger.setMessageHandler(channel, handler, taskQueue);
@NonNull String channel, @Nullable BinaryMessenger.BinaryMessageHandler handler) {
messenger.setMessageHandler(channel, handler);
}

@Override
@UiThread
public void setTaskQueue(@NonNull String channel, @Nullable TaskQueue taskQueue) {
messenger.setTaskQueue(channel, taskQueue);
}
}
}
36 changes: 25 additions & 11 deletions shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ class DartMessenger implements BinaryMessenger, PlatformMessageHandler {

@NonNull private final ConcurrentHashMap<String, HandlerInfo> messageHandlers;

@NonNull private final HashMap<String, DartMessengerTaskQueue> taskQueues;

@NonNull private final Map<Integer, BinaryMessenger.BinaryReply> pendingReplies;
private int nextReplyId = 1;

Expand All @@ -49,6 +51,7 @@ class DartMessenger implements BinaryMessenger, PlatformMessageHandler {
this.pendingReplies = new HashMap<>();
this.createdTaskQueues = new WeakHashMap<TaskQueue, DartMessengerTaskQueue>();
this.taskQueueFactory = taskQueueFactory;
this.taskQueues = new HashMap<>();
}

DartMessenger(@NonNull FlutterJNI flutterJNI) {
Expand Down Expand Up @@ -112,26 +115,37 @@ public TaskQueue makeBackgroundTaskQueue() {

@Override
public void setMessageHandler(
@NonNull String channel,
@Nullable BinaryMessenger.BinaryMessageHandler handler,
@Nullable TaskQueue taskQueue) {
@NonNull String channel, @Nullable BinaryMessenger.BinaryMessageHandler handler) {
if (handler == null) {
Log.v(TAG, "Removing handler for channel '" + channel + "'");
messageHandlers.remove(channel);
} else {
DartMessengerTaskQueue dartMessengerTaskQueue = null;
if (taskQueue != null) {
dartMessengerTaskQueue = createdTaskQueues.get(taskQueue);
if (dartMessengerTaskQueue == null) {
throw new IllegalArgumentException(
"Unrecognized TaskQueue, use BinaryMessenger to create your TaskQueue (ex makeBackgroundTaskQueue).");
}
}
DartMessengerTaskQueue dartMessengerTaskQueue = taskQueues.get(channel);
Log.v(TAG, "Setting handler for channel '" + channel + "'");
messageHandlers.put(channel, new HandlerInfo(handler, dartMessengerTaskQueue));
}
}

@Override
public void setTaskQueue(@NonNull String channel, @Nullable BinaryMessenger.TaskQueue taskQueue) {
DartMessengerTaskQueue dartMessengerTaskQueue = null;
if (taskQueue == null) {
taskQueues.remove(channel);
} else {
dartMessengerTaskQueue = createdTaskQueues.get(taskQueue);
if (dartMessengerTaskQueue == null) {
throw new IllegalArgumentException(
"Unrecognized TaskQueue, use BinaryMessenger to create your TaskQueue (ex makeBackgroundTaskQueue).");
}
taskQueues.put(channel, dartMessengerTaskQueue);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}
HandlerInfo existingHandlerInfo = messageHandlers.get(channel);
if (existingHandlerInfo != null) {
messageHandlers.put(
channel, new HandlerInfo(existingHandlerInfo.handler, dartMessengerTaskQueue));
}
}

@Override
@UiThread
public void send(@NonNull String channel, @NonNull ByteBuffer message) {
Expand Down
4 changes: 2 additions & 2 deletions shell/platform/android/io/flutter/plugin/common/BasicMessageChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ public void send(@Nullable T message, @Nullable final Reply<T> callback) {
*/
@UiThread
public void setMessageHandler(@Nullable final MessageHandler<T> handler) {
messenger.setMessageHandler(
name, handler == null ? null : new IncomingMessageHandler(handler), taskQueue);
messenger.setMessageHandler(name, handler == null ? null : new IncomingMessageHandler(handler));
messenger.setTaskQueue(name, handler == null ? null : taskQueue);
}

/**
Expand Down
18 changes: 14 additions & 4 deletions shell/platform/android/io/flutter/plugin/common/BinaryMessenger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,24 @@ public interface TaskQueue {}
*
* @param channel the name {@link String} of the channel.
* @param handler a {@link BinaryMessageHandler} to be invoked on incoming messages, or null.
*/
@UiThread
void setMessageHandler(@NonNull String channel, @Nullable BinaryMessageHandler handler);

/**
* Registers a TaskQueue for a specified channel which controls the threading model to follow when
* invoking the message handler.
*
* <p>A null value for taskQueue means to execute the handler on the platform thread.
*
* <p>See also: {@link BinaryMessenger#makeBackgroundTaskQueue()}
*
* @param channel the name {@link String} of the channel.
* @param taskQueue a {@link BinaryMessenger.TaskQueue} that specifies what thread will execute
* the handler. Specifying null means execute on the platform thread.
*/
@UiThread
void setMessageHandler(
@NonNull String channel,
@Nullable BinaryMessageHandler handler,
@Nullable TaskQueue taskQueue);
void setTaskQueue(@NonNull String channel, @Nullable TaskQueue taskQueue);

/** Handler for incoming binary messages from Flutter. */
interface BinaryMessageHandler {
Expand Down
3 changes: 2 additions & 1 deletion shell/platform/android/io/flutter/plugin/common/EventChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ public EventChannel(
@UiThread
public void setStreamHandler(final StreamHandler handler) {
messenger.setMessageHandler(
name, handler == null ? null : new IncomingStreamRequestHandler(handler), taskQueue);
name, handler == null ? null : new IncomingStreamRequestHandler(handler));
messenger.setTaskQueue(name, handler == null ? null : taskQueue);
}

/**
Expand Down
3 changes: 2 additions & 1 deletion shell/platform/android/io/flutter/plugin/common/MethodChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ public void invokeMethod(String method, @Nullable Object arguments, @Nullable Re
@UiThread
public void setMethodCallHandler(final @Nullable MethodCallHandler handler) {
messenger.setMessageHandler(
name, handler == null ? null : new IncomingMethodCallHandler(handler), taskQueue);
name, handler == null ? null : new IncomingMethodCallHandler(handler));
messenger.setTaskQueue(name, handler == null ? null : taskQueue);
}

/**
Expand Down
10 changes: 8 additions & 2 deletions shell/platform/android/io/flutter/view/FlutterNativeView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,14 @@ public void send(String channel, ByteBuffer message, BinaryReply callback) {

@Override
@UiThread
public void setMessageHandler(String channel, BinaryMessageHandler handler, TaskQueue taskQueue) {
dartExecutor.getBinaryMessenger().setMessageHandler(channel, handler, taskQueue);
public void setMessageHandler(String channel, BinaryMessageHandler handler) {
dartExecutor.getBinaryMessenger().setMessageHandler(channel, handler);
}

@Override
@UiThread
public void setTaskQueue(String channel, TaskQueue taskQueue) {
dartExecutor.getBinaryMessenger().setTaskQueue(channel, taskQueue);
}

/*package*/ FlutterJNI getFlutterJNI() {
Expand Down
10 changes: 8 additions & 2 deletions shell/platform/android/io/flutter/view/FlutterView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,8 +846,14 @@ public void send(String channel, ByteBuffer message, BinaryReply callback) {

@Override
@UiThread
public void setMessageHandler(String channel, BinaryMessageHandler handler, TaskQueue taskQueue) {
mNativeView.setMessageHandler(channel, handler, taskQueue);
public void setMessageHandler(String channel, BinaryMessageHandler handler) {
mNativeView.setMessageHandler(channel, handler);
}

@Override
@UiThread
public void setTaskQueue(String channel, TaskQueue taskQueue) {
mNativeView.setTaskQueue(channel, taskQueue);
}

/** Listener will be called on the Android UI thread once when Flutter renders the first frame. */
Expand Down
55 changes: 49 additions & 6 deletions shell/platform/android/test/io/flutter/embedding/engine/dart/DartMessengerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.flutter.plugin.common.BinaryMessenger;
import io.flutter.plugin.common.BinaryMessenger.BinaryMessageHandler;
import java.nio.ByteBuffer;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mockito;
Expand All @@ -23,7 +24,7 @@
@Config(manifest = Config.NONE)
@RunWith(RobolectricTestRunner.class)
public class DartMessengerTest {
SynchronousTaskQueue synchronousTaskQueue = new SynchronousTaskQueue();
SynchronousTaskQueue synchronousTaskQueue = null;

private static class ReportingUncaughtExceptionHandler
implements Thread.UncaughtExceptionHandler {
Expand All @@ -36,9 +37,21 @@ public void uncaughtException(Thread t, Throwable e) {
}

private static class SynchronousTaskQueue implements DartMessengerTaskQueue {
private boolean didRun = false;

public void dispatch(Runnable runnable) {
didRun = true;
runnable.run();
}

public boolean getDidRun() {
return didRun;
}
}

@Before
public void setUp() {
synchronousTaskQueue = new SynchronousTaskQueue();
}

@Test
Expand All @@ -59,7 +72,8 @@ public void itHandlesErrors() {
.when(throwingHandler)
.onMessage(any(ByteBuffer.class), any(DartMessenger.Reply.class));
BinaryMessenger.TaskQueue taskQueue = messenger.makeBackgroundTaskQueue();
messenger.setMessageHandler("test", throwingHandler, taskQueue);
messenger.setMessageHandler("test", throwingHandler);
messenger.setTaskQueue("test", taskQueue);
messenger.handleMessageFromDart("test", ByteBuffer.allocate(0), 0, 0);
assertNotNull(reportingHandler.latestException);
assertTrue(reportingHandler.latestException instanceof AssertionError);
Expand All @@ -78,7 +92,8 @@ public void givesDirectByteBuffer() {
wasDirect[0] = message.isDirect();
};
BinaryMessenger.TaskQueue taskQueue = messenger.makeBackgroundTaskQueue();
messenger.setMessageHandler(channel, handler, taskQueue);
messenger.setMessageHandler(channel, handler);
messenger.setTaskQueue(channel, taskQueue);
final ByteBuffer message = ByteBuffer.allocateDirect(4 * 2);
message.rewind();
message.putChar('a');
Expand All @@ -87,6 +102,31 @@ public void givesDirectByteBuffer() {
message.putChar('d');
messenger.handleMessageFromDart(channel, message, /*replyId=*/ 123, 0);
assertTrue(wasDirect[0]);
assertTrue(synchronousTaskQueue.didRun);
}

@Test
public void setTaskQueueFirst() {
// The same test as givesDirectByteBuffer, but calls setTaskQueue before setMessageHandler.
final FlutterJNI fakeFlutterJni = mock(FlutterJNI.class);
final DartMessenger messenger = new DartMessenger(fakeFlutterJni, () -> synchronousTaskQueue);
final String channel = "foobar";
final boolean[] wasDirect = {false};
final BinaryMessenger.BinaryMessageHandler handler =
(message, reply) -> {
wasDirect[0] = message.isDirect();
};
BinaryMessenger.TaskQueue taskQueue = messenger.makeBackgroundTaskQueue();
messenger.setTaskQueue(channel, taskQueue);
messenger.setMessageHandler(channel, handler);
final ByteBuffer message = ByteBuffer.allocateDirect(4 * 2);
message.rewind();
message.putChar('a');
message.putChar('b');
message.putChar('c');
message.putChar('d');
messenger.handleMessageFromDart(channel, message, /*replyId=*/ 123, 0);
assertTrue(synchronousTaskQueue.didRun);
}

@Test
Expand All @@ -103,7 +143,8 @@ public void directByteBufferLimitZeroAfterUsage() {
assertEquals(bufferSize, byteBuffers[0].limit());
};
BinaryMessenger.TaskQueue taskQueue = messenger.makeBackgroundTaskQueue();
messenger.setMessageHandler(channel, handler, taskQueue);
messenger.setMessageHandler(channel, handler);
messenger.setTaskQueue(channel, taskQueue);
final ByteBuffer message = ByteBuffer.allocateDirect(bufferSize);
message.rewind();
message.putChar('a');
Expand Down Expand Up @@ -161,7 +202,8 @@ public void cleansUpMessageData() throws InterruptedException {
(ByteBuffer message, BinaryMessenger.BinaryReply reply) -> {
reply.reply(null);
};
messenger.setMessageHandler(channel, handler, taskQueue);
messenger.setMessageHandler(channel, handler);
messenger.setTaskQueue(channel, taskQueue);
final ByteBuffer message = ByteBuffer.allocateDirect(4 * 2);
int replyId = 1;
long messageData = 1234;
Expand All @@ -179,7 +221,8 @@ public void cleansUpMessageDataOnError() throws InterruptedException {
(ByteBuffer message, BinaryMessenger.BinaryReply reply) -> {
throw new RuntimeException("hello");
};
messenger.setMessageHandler(channel, handler, taskQueue);
messenger.setMessageHandler(channel, handler);
messenger.setTaskQueue(channel, taskQueue);
final ByteBuffer message = ByteBuffer.allocateDirect(4 * 2);
int replyId = 1;
long messageData = 1234;
Expand Down
6 changes: 3 additions & 3 deletions shell/platform/android/test/io/flutter/plugin/editing/TextInputPluginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1884,7 +1884,7 @@ public void respondsToInputChannelMessages() {
textInputChannel.setTextInputMethodHandler(mockHandler);

verify(mockBinaryMessenger, times(1))
.setMessageHandler(any(String.class), binaryMessageHandlerCaptor.capture(), eq(null));
.setMessageHandler(any(String.class), binaryMessageHandlerCaptor.capture());

BinaryMessenger.BinaryMessageHandler binaryMessageHandler =
binaryMessageHandlerCaptor.getValue();
Expand Down Expand Up @@ -1917,7 +1917,7 @@ public void sendAppPrivateCommand_dataIsEmpty() throws JSONException {
new TextInputPlugin(testView, textInputChannel, mock(PlatformViewsController.class));

verify(mockBinaryMessenger, times(1))
.setMessageHandler(any(String.class), binaryMessageHandlerCaptor.capture(), eq(null));
.setMessageHandler(any(String.class), binaryMessageHandlerCaptor.capture());

JSONObject arguments = new JSONObject();
arguments.put("action", "actionCommand");
Expand Down Expand Up @@ -1948,7 +1948,7 @@ public void sendAppPrivateCommand_hasData() throws JSONException {
new TextInputPlugin(testView, textInputChannel, mock(PlatformViewsController.class));

verify(mockBinaryMessenger, times(1))
.setMessageHandler(any(String.class), binaryMessageHandlerCaptor.capture(), eq(null));
.setMessageHandler(any(String.class), binaryMessageHandlerCaptor.capture());

JSONObject arguments = new JSONObject();
arguments.put("action", "actionCommand");
Expand Down
5 changes: 1 addition & 4 deletions ...nd_image/android/app/src/main/java/dev/flutter/android_background_image/MainActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,7 @@ public void onMessage(ByteBuffer message, final BinaryMessenger.BinaryReply call

@Override
public void configureFlutterEngine(@NonNull FlutterEngine flutterEngine) {
flutterEngine
.getDartExecutor()
.getBinaryMessenger()
.setMessageHandler("finish", finishHandler, null);
flutterEngine.getDartExecutor().getBinaryMessenger().setMessageHandler("finish", finishHandler);

final boolean moved = moveTaskToBack(true);
if (!moved) {
Expand Down
3 changes: 1 addition & 2 deletions ...g/scenario_app/android/app/src/main/java/dev/flutter/scenarios/SpawnedEngineActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@ public FlutterEngine provideFlutterEngine(@NonNull Context context) {

secondEngine
.getDartExecutor()
.setMessageHandler(
"take_screenshot", (byteBuffer, binaryReply) -> notifyFlutterRendered(), null);
.setMessageHandler("take_screenshot", (byteBuffer, binaryReply) -> notifyFlutterRendered());

return secondEngine;
}
Expand Down
3 changes: 1 addition & 2 deletions ...scenario_app/android/app/src/main/java/dev/flutter/scenarios/TestableFlutterActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ public void configureFlutterEngine(@NonNull FlutterEngine flutterEngine) {
// registration will fail and print a scary exception in the logs.
flutterEngine
.getDartExecutor()
.setMessageHandler(
"take_screenshot", (byteBuffer, binaryReply) -> notifyFlutterRendered(), null);
.setMessageHandler("take_screenshot", (byteBuffer, binaryReply) -> notifyFlutterRendered());
}

protected void notifyFlutterRendered() {
Expand Down