Skip to content

Added platform protocols to flutter/services#7924

Closed
mravn-google wants to merge 5 commits intoflutter:masterfrom
mravn-google:platform_protocols
Closed

Added platform protocols to flutter/services#7924
mravn-google wants to merge 5 commits intoflutter:masterfrom
mravn-google:platform_protocols

Conversation

@mravn-google
Copy link
Contributor

Added two simple communication protocols on top of PlatformMessages:

PlatformProtocols.invokeJSONFunction (returns Future<T>)
PlatformProtocols.createJSONBroadcastStream (returns Stream<T>)

They allow client code to communicate with platform end-points in a slightly higher-level way than do PlatformMessages, including specified JSON envelopes for error handling. The main driver for adding such facilities is that PlatformProtocols are intended to be the Dart counterpart of an Android and iOS plugin concept (upcoming) that expects this particular protocol, and that should make it easier to write the native side in a stream-lined way that lends itself to tool support, e.g. for consuming a published plugin by declaring a dependency on it.

For preliminary native code and sample Dart client code, see the mit-mit/flutter fork.

More pull-requests cherry-picking from that fork are in the works.

///
/// Malformed JSON or envelopes, or data that the [decoder] cannot handle,
/// will make the returned Future complete with a [FormatException].
static Future<T> invokeJSONFunction<T>({
Copy link
Member

Choose a reason for hiding this comment

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

Having JSON in the method names makes it really hard to read. invokeFunction is so much more pleasing to the eye than invokeJSONFunction. Isn't the JSON part sufficiently clear from the arguments?

Copy link
Contributor

Choose a reason for hiding this comment

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

The JSON part seems like an implementation detail, also. We'll probably have a similar wrapper on the other side, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. We modeled these after PlatformMessages where the payload format used is also explicit in method names. I'll look into how best to hide that as an implementation detail while still supporting a decently documented decoder parameter. A single signature like invokeFunction basically promises that we can deal with any kind of message encoding, and that in turn means less constraints on what the decoder should be able to handle.

/// Returns a [Future] representing an asynchronous invocation of a named
/// platform function with JSON-encoded arguments, results, and errors.
///
/// If specified, the optional [args] map must contain valid JSON values
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't abbreviate words (args->arguments)

Copy link
Contributor

Choose a reason for hiding this comment

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

this applies throughout this file

/// If specified, the optional [args] map must contain valid JSON values
/// according to [PlatformMessages.sendJSON].
///
/// The platform function must be implemented on the native side by a handler
Copy link
Contributor

Choose a reason for hiding this comment

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

please don't use the word "native", it means different things to different people. Android is the "host platform" to a Flutter application running on an Android device.

Copy link
Contributor

Choose a reason for hiding this comment

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

(this applies generally to all the occurrences of this term in this file)

///
/// { "name": <function name>,
/// "args": <arguments json>
/// }
Copy link
Contributor

Choose a reason for hiding this comment

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

this is odd wrapping. I'd probably put the } on the end of the arguments line, or put the { on a line on its own.

/// "args": <arguments json>
/// }
///
/// If [name] is not specified, [channel] is used in its place.
Copy link
Contributor

Choose a reason for hiding this comment

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

by whom?

///
/// In both cases, `data` is optional and treated as `null` when missing.
/// The other envelope fields are mandatory. Additional fields, if any, are
/// silently discarded.
Copy link
Contributor

Choose a reason for hiding this comment

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

"mandatory" meaning what? Is an exception thrown? Message ignored? Program aborted?

'args': args,
});
return _interpretJSON<T>(reply, decoder);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this differ from PlatformMessages.invokeMethod ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll reconsider this method in light of the discussion above about hiding the payload encoding as an implementation detail as well as the discussion below about refactoring PlatformMessages. I don't think there should be two invokeMethod-like methods in the SDK.

To answer the question: The main difference is the use of the specified JSON envelope for responses, making the success/error status explicit as in HTTP. Beyond that, we use an args map rather than an args list, basically for the same reason that Dart has named parameters (it also provides a place for the eventChannel field in createJSONBroadcastStream). And then there's the decoder parameter producing a custom T rather than a JSON-like value.

exception: exception,
stack: stack,
library: 'services library',
context: 'during de-activating platform stream on channel $channel',
Copy link
Contributor

Choose a reason for hiding this comment

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

s/during/while/

try {
await invokeJSONFunction<Null>(channel: channel, name: 'listen', args: args);
}
catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: catch should be on the same line as the }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blast! Why are old habits so hard to unlearn?

try {
await invokeJSONFunction<Null>(channel: channel, name: 'cancel', args: args);
}
catch (exception, stack) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

final dynamic status = json['status'];
final dynamic message = json['message'];
final dynamic data = json['data'];
if (status == 'ok') {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no braces around one-line blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is part of a chain? I've rewritten the chain so that it now contains one-liners in all branches, and dispensed with the braces.

if (status == 'ok') {
return decoder?.call(data);
}
else if (status is String && message is String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no newline between } and else

details: data,
);
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto, and below

PlatformException({
@required this.status,
@required this.message,
this.details
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: prefer a trailing comma in places like this

/// Malformed JSON or envelopes, or data that the [decoder] cannot handle,
/// will make the returned Stream produce an error event containing a
/// [FormatException].
static Stream<T> createJSONBroadcastStream<T>({
Copy link
Contributor

@Hixie Hixie Feb 7, 2017

Choose a reason for hiding this comment

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

I would maybe name this as what it does, rather than the implementation behind it. For example, PlatformProtocols.receiveStream<bool>(channel, { arguments... }, decoder...).

@Hixie
Copy link
Contributor

Hixie commented Feb 7, 2017

This seems fine, but I wonder if JSON really is the best transport for this stuff. Having to convert an int into a string then insert it into JSON then encode it as UTF-8, then transfer it over, then parse the bytes as UTF-8 then JSON then parsing the int, seems like a lot of effort when we could just pass the four bytes as-is.

JSON also limits us to what we can send over. For example, how would you send an image? Are we going to use this mechanism for a stream of data from the camera?


/// Protocols for platform communication, implemented on top of
/// [PlatformMessages].
class PlatformProtocols {
Copy link
Contributor

Choose a reason for hiding this comment

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

if this shouldn't be constructed, make sure it has a private named constructor.

@Hixie
Copy link
Contributor

Hixie commented Feb 7, 2017

Would it make more sense to put this on PlatformMessages rather than creating yet another static?

@Hixie
Copy link
Contributor

Hixie commented Feb 7, 2017

Another option which might be more usable would be to refactor the PlatformMessages stuff so that there is a PlatformChannel object that represents a channel, and then on that you can send and receive messages, set up a stream, etc.

@mravn-google
Copy link
Contributor Author

mravn-google commented Feb 8, 2017

Plan:

  1. refactor PlatformProtocols into PlatformMessages
  2. introduce PlatformChannel concept
  3. unify PlatformMessages.invokeMethod and PlatformProtocols.invokeJSONFunction
  4. suppress JSON into an implementation detail, then enable more general and performant types of payload encoding

@mravn-google
Copy link
Contributor Author

WIP. I'll upload a pull request once I have docs and tests in place.

/// A named channel for efficiently communicating with the host platform using
/// structured messages. The message types supported are non-cyclic structures
/// of the following kinds:
///
/// * `null`
/// * [bool]
/// * [int], will be communicated using 32-bit two's complement binary form
/// * [double], will be communicated using IEEE 754 double-precision binary
///   floating-point format (binary64)
/// * [String]
/// * [ByteData]
/// * [List] of payload values
/// * [Map] from strings to payload values
///
/// Values of these types will be referred to as 'payload values' below.
/// In function signatures, the type of a payload value is `dynamic`.
class PlatformChannel {
  final String channel;

  PlatformChannel(this.channel);

  Future<dynamic> send(dynamic payload) async {
    return _decode(await PlatformMessages.sendBinary(channel, _encode(payload)));
  }

  void setMessageHandler(Future<dynamic> handler(dynamic payload)) {
    PlatformMessages.setBinaryMessageHandler(channel, (ByteData bytes) async {
      return _encode(await handler(_decode(bytes)));
    });
  }

  void setMockMessageHandler(Future<dynamic> handler(dynamic payload)) {
    if (handler == null) {
      PlatformMessages.setMockBinaryMessageHandler(channel, null);
    } else {
      PlatformMessages.setMockBinaryMessageHandler(channel, (ByteData bytes) async {
        return _encode(await handler(_decode(bytes)));
      });
    }
  }

  Future<T> invokeFunction<T>({
    String name,
    Map<String, dynamic> arguments,
    T interpreter(dynamic payload),
  }) async {
    return _interpretPayload(
      send(_encode(<String, dynamic>{
        'name': name,
        'args': arguments,
      })),
      interpreter);
  }

  Stream<T> receiveStream<T>({
    Map<String, dynamic> arguments,
    T interpreter(dynamic payload),
  }) {
    assert(arguments == null || arguments['eventChannel'] is String);
    final String eventChannel = arguments == null ? channel : arguments['eventChannel'];
    StreamController<T> controller;
    controller = new StreamController<T>.broadcast(
      onListen: () async {
        PlatformMessages.setBinaryMessageHandler(
          eventChannel,
          (ByteData reply) async {
            try {
              controller.add(_interpretPayload<T>(_decode(reply), interpreter));
            } catch (e) {
              controller.addError(e);
            }
          }
        );
        try {
          await invokeFunction<Null>(
            name: 'listen',
            arguments: arguments,
          );
        } catch (e) {
          PlatformMessages.setBinaryMessageHandler(eventChannel, null);
          controller.addError(e);
        }
      }, onCancel: () async {
        PlatformMessages.setBinaryMessageHandler(eventChannel, null);
        try {
          await invokeFunction<Null>(
            name: 'cancel',
            arguments: arguments,
          );
        } catch (exception, stack) {
          FlutterError.reportError(new FlutterErrorDetails(
            exception: exception,
            stack: stack,
            library: 'services library',
            context: 'while de-activating platform stream on channel $channel',
          ));
        }
      }
    );
    return controller.stream;
  }

  T _interpretPayload<T>(dynamic payload, T interpreter(dynamic payload)) {
    if (payload is Map) {
      final dynamic status = payload['status'];
      final dynamic message = payload['message'];
      final dynamic data = payload['data'];
      if (status == 'ok')
        return interpreter?.call(data);
      else if (status is String && message is String)
        throw new PlatformException(status: status, message: message, details: data);
      else
        throw new FormatException('Invalid envelope: $payload');
    } else {
      throw new FormatException('Expected envelope Map, got $payload');
    }
  }

  // TODO(mravn): consider replacing this homegrown codec by something existing (protobuf?).
  // TODO(mravn): consider making the codec pluggable.
  ByteData _encode(dynamic payload) { ... }
  dynamic _decode(ByteData bytes) { ... }
}

/// Thrown to indicate that a platform interaction resulted in an error.
class PlatformException implements Exception {
  PlatformException({
    @required this.status,
    @required this.message,
    this.details,
  }) {
    assert(status != null);
    assert(message != null);
  }

  /// A non-`null` status string, such as 'error' or 'not found'.
  final String status;

  /// A non-`null` human-readable error message.
  final String message;

  /// A payload value providing custom details about the error, maybe `null`.
  final dynamic details;

  @override
  String toString() => 'PlatformException($status, $message, $details)';
}

@mravn-google
Copy link
Contributor Author

WIP. Created a PlatformChannel concept, a named communication channel with pluggable message codec.

I'd very much appreciate comments on the new design, before I take it much further.

TODOs:

  • platform-side message codec implementation
  • dart-side tests
  • replace calls to deprecated PlatformMessages methods with use of PlatformChannel.
  • reconsider the current standard codec implementation, maybe use protobufs or some other established standard.

@mit-mit
Copy link
Member

mit-mit commented Feb 15, 2017

Update: Mikkel is out this week; he will return to this next week.

@Hixie
Copy link
Contributor

Hixie commented Feb 17, 2017

I'm going to close this PR to get it off the review queue since it's not ready for checkin yet, but please don't hesitate to submit a new PR (or reopen this one) when you're ready.

@Hixie Hixie closed this Feb 17, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants