BasicMessageChannel's with BinaryCodec aren't sending responses on Android

[gaaclarke]

Android Code:

class MainActivity: FlutterActivity() {

    override fun configureFlutterEngine(flutterEngine: FlutterEngine) {
        val basicBinary =
            BasicMessageChannel(flutterEngine.dartExecutor, "dev.flutter.echo.basic.binary", BinaryCodec.INSTANCE)
        basicBinary.setMessageHandler { message, reply -> reply.reply(message) }
        super.configureFlutterEngine(flutterEngine)
    }
}

Flutter Code:

  const BasicMessageChannel<ByteData> basicBinary =
      BasicMessageChannel<ByteData>(
    'dev.flutter.echo.basic.binary',
    BinaryCodec(),
  );

  int size = 0;
  for (int i = 0; i < _numMessages; ++i) {
    final ByteData result = await basicBinary.send(encodedLargeBuffer);
    size += (result == null) ? 0 : result.lengthInBytes;
  }
  if (size != encodedLargeBuffer.lengthInBytes * _numMessages) {
    throw Exception(
        'There is an error with the echo channel, the results don\'t add up: $size');
  }

Seen result:
result is not null, but the lengthInBytes is zero. The exception is getting thrown.

Expected result:
result should be equal to encodedLargeBuffer. This works on iOS, not on Android.

Debugging Info:
I was able to verify that the data is getting received correctly in the channel's handler. I followed the data all the way in the Java debugger and everything looks good. It seems the data is getting lost in the layer below that.

Reproduction Code:
I discovered the problem when authoring #81874

[gaaclarke]

This is happening because the reply messages size is being sent as the bytebuffer's position: https://github.com/flutter/engine/blob/ccaae8d9582b275002f79f46ade56f9c1b307234/shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java#L160

[gaaclarke]

Here is the correct implementation of the echo channel:

class MainActivity: FlutterActivity() {

    override fun configureFlutterEngine(flutterEngine: FlutterEngine) {
        val basicBinary = BasicMessageChannel(flutterEngine.dartExecutor, "dev.flutter.echo.basic.binary", BinaryCodec.INSTANCE)
        basicBinary.setMessageHandler { message, reply -> run {
            val result = ByteBuffer.allocateDirect(message!!.capacity())
            result.put(message)
            reply.reply(result)
        } }
        super.configureFlutterEngine(flutterEngine)
    }
}

Not only did I have to move the position of the buffer to the end. I had to make sure that I used a direct buffer. At the very least we should have an assert that the bytebuffer is direct.

[gaaclarke]

I verified that the following change fixes this issue. It is a breaking change though so I'm not sure if we want to do it.

diff --git a/shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java b/shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java
index fc03bb1f3..eb11f1325 100644
--- a/shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java
+++ b/shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java
@@ -157,7 +157,7 @@ class DartMessenger implements BinaryMessenger, PlatformMessageHandler {
       if (reply == null) {
         flutterJNI.invokePlatformMessageEmptyResponseCallback(replyId);
       } else {
-        flutterJNI.invokePlatformMessageResponseCallback(replyId, reply, reply.position());
+        flutterJNI.invokePlatformMessageResponseCallback(replyId, reply, reply.limit());
       }
     }
   }

[blasten]

It is a breaking change though so I'm not sure if we want to do it.

Do you know the use case? I can think of chunking/streaming of some sort. Is that the use case?

[gaaclarke]

@blasten I don't think there is a legitimately good reason to do it. You couldn't do chunking or something because you can't choose the start position of what gets copied, only the ending position (in a not straight forward way). Since it could break plugins in subtle ways though at runtime I'm thinking it might not be worth it. I wouldn't be surprised if the change has zero impact on the whole ecosystem though.

We could add an exception if the position of the buffer passed in is at zero though, that will always result in sending a zero length buffer to Flutter. I can't imagine anyone intended that.

[blasten]

sgtm. My hypothesis on why it used reply.position() instead of reply.limit() is because the common case is to construct your own buffer, write to it, and each write operation advances the position. Hence, the common case is that position() equals the whole thing.

[blasten]

It's mentioned here: https://github.com/flutter/engine/blob/138c91c614d742c52aa5432b4cb921f0ff9fdee2/shell/platform/android/io/flutter/plugin/common/BinaryMessenger.java#L97 - I still don't understand the use case 😅

[gaaclarke]

Closing this since it seems to be working as intended.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.