Refactor platform message logic #21572

Hixie · 2020-10-02T22:15:13Z

This laids the groundwork for sending messages through ChannelBuffers with channel-specific callbacks rather than a single onPlatformMessage callback.

This allows us to remove the logic from the framework that puts data back into the channel buffers. Right now (before this PR) the logic for messages from plugins to the framework is bidirectional:

                  **                 *
Plugins -> Engine -> ChannelBuffers <- Framework <---+-.
               |              |                      | |
               |              '------> via drain ----' |
               |                                       |
               '----------------- onPlatformMessage ---'

* = when the listener is null on the framework side
** = when onPlatformMessage is null

This ends up with weird race conditions and is generally less than completely clear. With this PR, we lay the groundwork for eventually reaching this model:

Plugins -> Engine -> ChannelBuffers -> Framework

...which is significantly simpler.

Hixie · 2020-10-02T22:15:29Z

I'm expecting tests to fail, I didn't run them locally.

Hixie · 2020-10-06T21:27:19Z

Fixes flutter/flutter#66699. (Well, it doesn't really fix it, it lays the groundwork for a replacement approach. But it's related to it.)

gaaclarke

This looks good for the most part the biggest points I have are:

The behavior of clearing the listener while draining
Never drop messages without telling the developer, bugs created by this are difficult to debug
An explicit call to drain gives us a place to put logic that happens after the drain

gaaclarke · 2020-10-06T22:00:05Z

lib/ui/channel_buffers.dart

-      // the buffer once that is available to users and print in all engine builds
-      // after we verify that dropping messages isn't part of normal execution.
-      _printDebug('Overflow on channel: $channel. '
-                  'Messages on this channel are being discarded in FIFO fashion. '


You can't get rid of this warning. We can't just drop messages without warning developers. It's particularly an issue with MethodChannels that multiplex multiple messages onto one channel.

My understanding is that dropping messages here is part of the expected behaviour of the API. Is that not correct? The documentation talks about how you can legitimately want to only be holding on to the newest message (e.g. because you're a level-trigger API).

We have a policy of only logging messages that are actionable (see style guide). If the messages are not actionable, we shouldn't log the message. I don't know what action one should take with this message.

I agree that if you're dropping messages when you don't expect to be (e.g. you're listening to the wrong channel) then this message would make debugging the issue much easier. Is there some way we can determine when the message is actionable and when it's just logspam?

Nope, it's not expected. Running the engine is an asynchronous operation and we don't provide any mechanism for users to determine if the engine is running before we can send messages.

This code will fail to deliver a message randomly:

[engine run]; usleep(rand() * 1000000); [channel sendMessage:@"hello"]; [channel sendMessage:@"goodbye"];

One action that could fix the problem is to chose a more appropriate buffer size, which is mentioned. If you are using method channels you'd probably want to base that calculus off of the number of unique messages you are sending over that channel.

It also mentions that the engine isn't running which admittedly is a hint about the asynchronous nature of running engines. With that information they could solve the problem a few other ways.

When I first implemented channel buffers we saw there were all kinds of messages routinely being dropped during the initialization of an engine. They were not expected.

Ok, we should fix the documentation then, because right now it clearly states that there are cases where dropping messages is the appropriate behavior.

Maybe we should show the message for any message that is buffered, if the buffer size hasn't been set? I guess in that case we probably would want to default to a buffer size of zero, too, since we'd be saying all plugins need to set their buffer size if they send messages before the framework is ready.

Another option would be to leave the default as now, but have a specific control message to silence the message (and have the warning log say to send this control message).

Can you point out the documentation that says dropping messages is appropriate? One other caveat we haven't discussed is that there are 2 types of channels and the semantics aren't explicit, channels where dropping messages is fine (think sending the gyro readings to the engine) and channels where it isn't (think sending entries in a database). In other words, is the message interpreted as set or add? Is the documentation referring to that?

Buffer size zero by default would be a big breaking change. I wouldn't recommend that.

The worse case scenario is that users are losing data when they don't expect it. The print statement is meant to warn users of that scenario. I worry moving the print statement out of that scenario may cause it to be spammy and less likely heeded when it is appropriate.

However we slice things, not printing out the error despite having set the buffer size wouldn't be ideal since you'd have no idea that you may have chosen poorly for your use case.

since we'd be saying all plugins need to set their buffer size if they send messages before the framework is ready.

Remember there is no way to know when the framework is ready clients can't meaningfully know that they may be doing something that may not work.

have a specific control message to silence the message

That's cool with me. A bigger change would be if we could know the semantics of the channel and only print out the error if it has add semantics instead of set semantics. The ship might have sailed on that though.

https://master-api.flutter.dev/flutter/dart-ui/ChannelBuffers/kDefaultBufferSize-constant.html is the documentation I was talking about (I expand on it in this PR). It says "Size 1 implies that you only care about the most recent value." and says size 1 is the default, which implies that dropping messages is fine by default.

We could improve the control channel API in general, making it support a way to specify if it's being used for an event sequence (where dropping is bad) or where it's being used as a level-trigger (where dropping is fine). That would also give us the opportunity to make it use the standard binary format instead of its current inefficent bespoke text format with '\r' as the separator. Let me see what I can do.

gaaclarke · 2020-10-06T22:10:43Z

lib/ui/hooks.dart

  }
 }

+/// Invokes [callback] inside the given [zone] passing it [arg1] and [arg2].


nit: I'd note arity 2 inside the docstring to immediately communicate the meaning of 2 in the name.

gaaclarke · 2020-10-06T22:21:29Z

lib/ui/channel_buffers.dart

+  ///
+  /// When there is no listener, messages are queued, up to [capacity],
+  /// and then discarded in a first-in-first-out fashion.
+  void clearListener() {


Clearing the listener while draining will lead to a null pointer exception. I thought we said we would drop the messages.

I will add a test for this case.

My intention was for removing a listener to just cause the messages to stop being drained, without losing them.

test seems to confirm that clearListener() just pauses the draining.

Ahh yep, thanks I missed the null check before its usage.

gaaclarke · 2020-10-06T22:26:18Z

lib/ui/channel_buffers.dart

+  /// The messages are processed by calling the given `callback`. Each message
+  /// is processed in its own microtask.
+  // TODO(ianh): deprecate once framework uses [setListener].
+  Future<void> drain(String name, DrainChannelCallback callback) async {


I'm not sure that anyone ever took advantage of the future that was returned here, but now there is no way for a client to know when the draining process is done with the setListener API. There isn't a place to put it either. I would rather keep the explicit drain call, maybe replace this with _drainAsynchronously?

I'd just say launching any asynchronous operation without returning a handle to that operation is a red flag since it hides the fact that something is asynchronous and provides no mechanism for someone to synchronize if they want to.

Right now the drain API only has one client, the framework, and it ignores the future.

That said, I think that exposing the end of the drain is a dangerous affordance. The point of this API is to buffer the messages so that when the plugin is ready to receive them, it gets them. The sender has no way to know if the plugin is already listening or not when it sends the message, so by definition the messages it sends cannot depend on whether or not the plugin is listeneing. If we allow messages received before the plugin subscribes to be distinguished to those received after the plugin subscribes, we allow the plugin to behave differently for events sent before and after subscription, but since we just said the sender is not distinguishing, that implies that we are allowing behavior variance along an axis that should have none (i.e., it would be a bug vector).

It also would encourage race conditions. Consider these two cases:

A:

t=0ms plugin requests buffer size 3

t=0ms message 1

t=2ms message 2

t=3ms plugin subscribes

t=4ms message 3

B:

t=0ms plugin requests buffer size 3

t=0ms message 1

t=1ms plugin subscribes

t=2ms message 2

t=4ms message 3

Why would we want the plugin to react differently to message 2 in the first case vs the second case?

To put it another way, what's the use case for knowing when the drain is complete?

I admit it is hypothetical. Hiding an asynchronous operation in a synchronous signature can cause bugs. It's not as helpful in Dart though because Dart allows you to drop return values implicitly.

Someday someone might write this code:

_drain(); clearListener();

This is a nonsensical operation because the clearListener would immediately disable the drain before anything happens.

If _drain advertises itself as an asynchronous operation by returning a Future<void> that mistake is less likely. I don't know what code some future developer might write but I'd error on the side of giving them best hope of writing correct code.

Sorry for not being clear. My medium term plan is to remove the drain API, it just becomes an implicit behavior of setListener. The idea is to remove the concept of draining from the API -- it just behaves exactly as if messages were only sent while the listener was set, and they get processed the same as if there were messages in fact only being sent while the listener was set.

Yea, I got that. I think it's a worse design because it's hiding asynchronous logic inside of the setter. This is hiding side effects and temporal dependencies which is a liability. If you are keen on this design naming setListener to setListenerAndDrain would be an improvement.

I don't really understand the concern here. Can you elaborate?

The point of this design is to make the draining entirely transparent, indistinguishable from not draining. There should be no way to distinguish someone connecting to a plugin before the plugin sends messages from connecting after the plugin sends messages. There's no "draining" anymore, conceptually, it's just that the messages weren't delivered until there was a listener.

gaaclarke · 2020-10-06T22:34:23Z

lib/ui/channel_buffers.dart

+  /// See [drain] for more details.
+  void _drainStep() {
+    assert(_draining);
+    if (_queue.isNotEmpty && _channelCallbackRecord != null) {
+      final _StoredMessage message = pop();
+      _channelCallbackRecord!.invoke(message.data, message.callback);
+      scheduleMicrotask(_drainStep);
+    } else {
+      _draining = false;
+    }
+  }
+}


The previous implementation of drain would handle the messages serially, this will potentially parallelize asynchronous operations. I think that is good since that behavior will match channels when they aren't buffering, correct?

The callbacks are synchronous, so they are guaranteed to be complete before the next microtask is scheduled. This matches the way messages are handled outside of drain.

Yea, they might spawn asynchronous operations but I think it was erroneous to wait for those operations previously. This sounds good to me.

yjbanov

/cc @mdebbar @nturgut for changes in navigation and text input.

yjbanov · 2020-10-06T21:23:37Z

lib/ui/channel_buffers.dart


-  _RingBuffer(this._capacity)
-    : _queue = collection.ListQueue<T>(_capacity);
+  /// The underlying data for the RingBuffer.


What does "RingBuffer" refer to? This PR seems to be removing this class.

yjbanov · 2020-10-06T21:25:57Z

lib/ui/channel_buffers.dart

+  /// messages are discarded until the capacity is reached.
+  ///
+  /// Returns the number of discarded messages resulting from resize.
+  int resize(int newSize) {


Why not set capacity? (a setter would remove the need to refer to capacity using two terms, "capacity" and "size").

yjbanov · 2020-10-06T21:28:13Z

lib/ui/channel_buffers.dart

-    : _queue = collection.ListQueue<T>(_capacity);
+  /// The underlying data for the RingBuffer.
+  ///
+  /// [ListQueue]s dynamically resize, but [_Channel]s do not.


What does "resize" mean in this sentence? _Channel do seem to resize (this PR even introduces the resize method).

removed this sentence

yjbanov · 2020-10-06T21:30:58Z

lib/ui/channel_buffers.dart

+  /// If the new size is smaller than the [length], the oldest
+  /// messages are discarded until the capacity is reached.
+  ///
+  /// Returns the number of discarded messages resulting from resize.


I'm not a native English speaker, but I feel like there should be either "a" or "the" in front of "resize".

Since this is now a setter, this line has been removed.

yjbanov · 2020-10-06T21:42:08Z

lib/ui/channel_buffers.dart

-typedef DrainChannelCallback = Future<void> Function(ByteData?, PlatformMessageResponseCallback);
+  /// Drains a single message and then reinvokes itself asynchronously.
+  ///
+  /// See [drain] for more details.


s/drain/_drain/

yjbanov · 2020-10-07T16:14:14Z

lib/ui/channel_buffers.dart

+  /// A mapping between a channel name and its associated [_Channel].
+  final Map<String, _Channel> _channels = <String, _Channel>{};

+  /// Adds a message (`data`) to the named channel buffer (`channel`).


Did you mean name instead of channel?

yjbanov · 2020-10-07T16:15:46Z

lib/ui/channel_buffers.dart

+  /// ## Draining
+  ///
+  /// If any messages were queued before the listener is added,
+  /// they are drained asynchronously after this method returns.


Consider linking to the drain method at the end of the paragraph. A reader may not know what "drain" means.

yjbanov · 2020-10-07T16:18:07Z

lib/ui/channel_buffers.dart

+  ///
+  /// The messages are processed by calling the given `callback`. Each message
+  /// is processed in its own microtask.
+  // TODO(ianh): deprecate once framework uses [setListener].


If this method is going away we should explain the term "drain" in other dartdocs.

The docs for setListener have a section on this.

yjbanov · 2020-10-07T16:20:12Z

lib/ui/hooks.dart


 /// Invokes [callback] inside the given [zone] passing it [arg].
+///
+/// The 1 in the name refers to the number of arguments expected by the callback.


...as well as the number of arguments passed to this function, not counting callback and zone.

yjbanov · 2020-10-07T16:30:14Z

lib/web_ui/lib/src/engine/window.dart

+      try {
+        channelBuffers.handleMessage(data!);
+      } catch (ex) {
+        print('Message to "$name" caused exception $ex');


This should probably log to https://api.dart.dev/stable/2.10.1/dart-html/Console/error.html, and should also include the stack trace.

Yeah I was wondering about this. Maybe we shouldn't even catch the exception here, and let the usual uncaught exception logic do its thing? I've removed the catch entirely for now. cc @gaaclarke who added this catch originally.

yjbanov · 2020-10-07T16:35:15Z

testing/dart/channel_buffers_test.dart

 // found in the LICENSE file.

 // @dart = 2.6
+


We currently do not run these tests against the web version and instead maintain separate tests. Can you please copy this test into lib/web_ui/test?

/cc @nturgut how feasible is it to run these tests against the web version? One difference between these tests and ours is that these tests input dart:ui, while web tests import package:ui/ui.dart.

I had actually already ported the tests but forgot to git add them, oops.

mdebbar

Changes related to keyboard, history, text and text editing look good to me. The rest was covered by @yjbanov's review I believe.

nturgut · 2020-10-09T00:56:19Z

Changes related to keyboard, history, text and text editing look good to me. The rest was covered by @yjbanov's review I believe.

our integration tests will fail if text_editing breaks :) They are testing almost every command that passes through platform channels.

nturgut · 2020-10-09T01:09:47Z

I'm a little confused, I have a question, if the end goal is this and we want to remove logic "data back into the channel buffers"

Plugins -> Engine -> ChannelBuffers -> Framework

what changes we need for clipboard(The equivalent web engine code would this )

nturgut · 2020-10-09T01:11:51Z

lib/web_ui/test/channel_buffers_test.dart

+import 'package:test/bootstrap/browser.dart';
+import 'package:test/test.dart';
+
+void main() {


our tests needs a different main function example

Hixie · 2020-10-09T02:36:00Z

I'm a little confused, I have a question, if the end goal is this and we want to remove logic "data back into the channel buffers"
Plugins -> Engine -> ChannelBuffers -> Framework
what changes we need for clipboard(The equivalent web engine code would this )

The change will be entirely transparent to plugins. It only affects a little bit of code deep in the services library.

chinmaygarde · 2020-10-15T21:39:28Z

The presubmit failures look like linter issues. Can you please fix and land this patch?

yjbanov

LGTM if LGT @gaaclarke

gaaclarke · 2020-10-16T22:21:49Z

Yea LGTM. The big thing for me was keeping the warning on overflow, thanks.

fluttergithubbot · 2020-10-19T18:42:02Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite build_and_test_linux_unopt_debug has failed. Please fix the issues identified (or deflake) before re-applying this label.

Hixie · 2020-10-19T20:09:07Z

(i'll repush when the tree failure is fixed)

This laids the groundwork for sending messages through ChannelBuffers with channel-specific callbacks rather than a single onPlatformMessage callback. This allows us to remove the logic from the framework that puts data back into the channel buffers. Right now (before this PR) the logic for messages from plugins to the framework is bidirectional: ``` ** * Plugins -> Engine -> ChannelBuffers <- Framework <---+-. | | | | | '------> via drain ----' | | | '----------------- onPlatformMessage ---' * = when the listener is null on the framework side ** = when onPlatformMessage is null ``` This ends up with weird race conditions and is generally less than completely clear. With this PR, we lay the groundwork for eventually reaching this model: ``` Plugins -> Engine -> ChannelBuffers -> Framework ``` ...which is significantly simpler.

...but add a control message to disable it. Also while we're at it move the control messages to the standard codec.

chinmaygarde · 2020-10-22T22:09:07Z

@Hixie Tree is open now. Let's land this ASAP before more conflicts creep in.

chinmaygarde · 2020-10-22T22:10:06Z

I see he is out this week. Will followup when he returns.

Hixie · 2020-10-22T23:33:27Z

I was going to try to land this but given the conflicts it'll have to take longer since I'll have to figure out how to merge it in.

chinmaygarde · 2020-10-23T01:18:17Z

Sounds good. I'll add the WIP tag so I don't keep nagging you during PR triage.

Hixie · 2020-10-29T00:22:02Z

I'll close this in favour of a merged version here: #22181

Hixie · 2020-10-30T02:02:06Z

Ok, the merge is ready in #22181. It can land once it has a review.

googlebot added the cla: yes label Oct 2, 2020

Hixie force-pushed the ChannelBuffers branch 8 times, most recently from 40ac949 to efcfc04 Compare October 6, 2020 19:15

Hixie marked this pull request as ready for review October 6, 2020 21:16

auto-assign bot requested a review from jason-simmons October 6, 2020 21:16

jason-simmons requested a review from gaaclarke October 6, 2020 21:33

gaaclarke reviewed Oct 6, 2020

View reviewed changes

Hixie force-pushed the ChannelBuffers branch from efcfc04 to 041f8e0 Compare October 6, 2020 23:56

yjbanov reviewed Oct 7, 2020

View reviewed changes

Hixie force-pushed the ChannelBuffers branch 3 times, most recently from 0c616e0 to 4cbd2ad Compare October 9, 2020 00:10

mdebbar approved these changes Oct 9, 2020

View reviewed changes

nturgut reviewed Oct 9, 2020

View reviewed changes

Hixie mentioned this pull request Oct 9, 2020

Can't see "Linux Fuchsia" logs flutter/flutter#67760

Closed

Hixie force-pushed the ChannelBuffers branch 3 times, most recently from 28d6953 to 6eef49d Compare October 9, 2020 21:39

Hixie force-pushed the ChannelBuffers branch from 6eef49d to 06e3255 Compare October 9, 2020 22:22

yjbanov approved these changes Oct 16, 2020

View reviewed changes

Hixie force-pushed the ChannelBuffers branch 2 times, most recently from f0b8889 to c6f0444 Compare October 19, 2020 17:41

Hixie added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Oct 19, 2020

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Oct 19, 2020

Hixie force-pushed the ChannelBuffers branch from c6f0444 to 87961ca Compare October 21, 2020 19:50

Hixie added 3 commits October 21, 2020 19:33

Show warning on overflow

2d18483

...but add a control message to disable it. Also while we're at it move the control messages to the standard codec.

Reduce the differences between Web and non-Web versions

fa78902

Hixie force-pushed the ChannelBuffers branch from 87961ca to fa78902 Compare October 22, 2020 02:33

Hixie mentioned this pull request Oct 22, 2020

Reland: Migration to PlatformDispatcher and multi-window #21932

Merged

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Oct 23, 2020

Hixie marked this pull request as draft October 29, 2020 00:21

Hixie closed this Oct 29, 2020

Hixie mentioned this pull request Nov 2, 2020

Refactor platform message logic #22181

Merged

Refactor platform message logic #21572

Refactor platform message logic #21572

Uh oh!

Conversation

Hixie commented Oct 2, 2020

Uh oh!

Hixie commented Oct 2, 2020

Uh oh!

Hixie commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Hixie commented Oct 6, 2020 •

edited

Loading

gaaclarke Oct 6, 2020 •

edited

Loading