Basic communication protocol for server

Basic communication protocol for server

R=devoncarew@google.com, scheglov@google.com

Committed: https://code.google.com/p/dart/source/detail?r=31951

[scheglov, 2014-01-19 19:26:20]

LGTM

[devoncarew, 2014-01-20 22:35:01]

lgtm

lgtm, w/ some things to consider

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/channel.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/channel.dart:25: void listen(void onRequest(Request
request), {void onError(), void onDone()});
My guess is that you want this to be a Stream. So something like:

  Stream<Request> get onRequest;

Implementations of this class would then have a StreamController field:

StreamController<Request> _controller = new StreamController.asBroadcast();
Stream<Request> get onRequest => _controller.stream;
void _add(Request request) => _controller.add(request);

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/channel.dart:30: void send(Object value);
Perhaps have separate methods for the two types? You then get support from the
tools, which is good.

sendResponse(Response r)
sendNotification(Notification n)

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:7: import 'dart:convert';
Perhaps, 

import 'dart:convert' show JSON;

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:46: static const JsonDecoder DECODER =
const JsonDecoder(null);
You can delete this line.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:72: var result =
DECODER.convert(data);
==>

var result = JSON.decode(data);

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:265: Map<String, Object> toJson() {
I have generally seen this as:

Map<String, dynamic> toJson() {

but the subtleties of the different escape me -

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/moc...
File pkg/analysis_server/test/mocks.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/moc...
pkg/analysis_server/test/mocks.dart:7: import '../lib/src/channel.dart';
You can also use a self-reference here:

import 'package:analysis_server/src/channel.dart';

There's no real semantic difference.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/pro...
File pkg/analysis_server/test/protocol_test.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/pro...
pkg/analysis_server/test/protocol_test.dart:7: import
'../lib/src/protocol.dart';
Can use a self-reference, or not.

nit: I generally see the file imports sorted below the package ones
(dart>package>file).

[Brian Wilkerson, 2014-01-21 03:21:04]

Committed patchset #1 manually as r31951 (presubmit successful).

[Brian Wilkerson, 2014-01-21 03:21:19]

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/channel.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/channel.dart:25: void listen(void onRequest(Request
request), {void onError(), void onDone()});
> My guess is that you want this to be a Stream.

I don't understand why that would be better. Let's talk FTF so you can explain
it to me.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/channel.dart:30: void send(Object value);
> Perhaps have separate methods for the two types?

I like that! Done.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:7: import 'dart:convert';
> import 'dart:convert' show JSON;

Added 'show JsonDecoder' pending resolution of the next two comments.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:46: static const JsonDecoder DECODER =
const JsonDecoder(null);
> You can delete this line.

Only if I make the next change.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:72: var result =
DECODER.convert(data);
> var result = JSON.decode(data);

I considered this, but this method creates a new decoder every time it's called.
I thought it would be more efficient to create one instance of re-use it,
especially given the number of requests that I expect most servers will get.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:265: Map<String, Object> toJson() {
> Map<String, dynamic> toJson() {

The difference primarily has to do with warnings. Using the type dynamic means
that the analyzer should assume that the object implements every possible method
(including getters, setters and operators). Using Object means that only methods
defined in Object should be assumed to be valid and everything else should
generate a warning (unless the value is cast to a more specific type). I think I
prefer getting more warnings.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/moc...
File pkg/analysis_server/test/mocks.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/moc...
pkg/analysis_server/test/mocks.dart:7: import '../lib/src/channel.dart';
> You can also use a self-reference here:

That does seem somewhat cleaner, though. What's the prefered style for imports
such as

import 'protocol.dart';

(such as the one in channel.dart)? Should it be

import 'package:analysis_server/src/protocol.dart';

Done.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/pro...
File pkg/analysis_server/test/protocol_test.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/pro...
pkg/analysis_server/test/protocol_test.dart:7: import
'../lib/src/protocol.dart';
Done.

[devoncarew, 2014-01-21 04:16:08]

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:72: var result =
DECODER.convert(data);
On 2014/01/21 03:21:20, Brian Wilkerson wrote:
> > var result = JSON.decode(data);
> 
> I considered this, but this method creates a new decoder every time it's
called.
> I thought it would be more efficient to create one instance of re-use it,
> especially given the number of requests that I expect most servers will get.

Hmm, I was not aware that a new decoder is created each time. I've always seen
the JSON.decode() form; it'd be a shame if it was less efficient.

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/moc...
File pkg/analysis_server/test/mocks.dart (right):

https://codereview.chromium.org/137853026/diff/1/pkg/analysis_server/test/moc...
pkg/analysis_server/test/mocks.dart:7: import '../lib/src/channel.dart';
On 2014/01/21 03:21:20, Brian Wilkerson wrote:
> > You can also use a self-reference here:
> 
> That does seem somewhat cleaner, though. What's the prefered style for imports
> such as
> 
> import 'protocol.dart';
> 
> (such as the one in channel.dart)? Should it be
> 
> import 'package:analysis_server/src/protocol.dart';
> 
> Done.

Within lib, I've only ever used relative file references. When referencing into
lib (from test, bin, web, ...) I generally use self references.