Use 'print' to send responses/notifications to the client.

Use 'print' to send responses/notifications to the client.

Ensure that Java client reads lines in a separate thread,
so that server's stdout never overflows.

R=brianwilkerson@google.com, danrubel@google.com, paulberry@google.com
BUG=

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

[danrubel, 2015-01-23 14:16:54]

LGTM. Thanks!

https://codereview.chromium.org/872633002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/internal/remote/ByteResponseStream.java
(right):

https://codereview.chromium.org/872633002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/internal/remote/ByteResponseStream.java:57:
if (!line.contains("\"result\":{\"version\":\"0.0.1\"}")) {
I believe that this check can now be removed. We are not using getVersion to
check health.

https://codereview.chromium.org/872633002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/internal/remote/ByteResponseStream.java:109:
String line = lineQueue.take();
Do we need to check for EOF_LINE after take ? Is this a potential race
condition?

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

https://codereview.chromium.org/872633002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:472: jsonObject['time'] = new
DateTime.now().millisecondsSinceEpoch;
Good idea, but I already have a pending CL to capture the request time.
https://codereview.chromium.org/865383002/
Please remove this.

https://codereview.chromium.org/872633002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:833: jsonObject['time'] = new
DateTime.now().millisecondsSinceEpoch;
Please remove this as described above.

[Paul Berry, 2015-01-23 15:17:34]

I believe this will only work if the client supplies the
"--internal-print-to-console" flag when starting the server.  Under normal
operating conditions (without that flag), print() is intercepted by the
_captureExceptions function in driver.dart, which records it for use by the http
server, so the messages will never reach the client.

Did you consider simply going back to using stdout rather than
stdout.nonBlocking?  That would avoid the problem with print, and would be more
in line with the way the code used to work before we made it asynchronous.

I'm also confused as to why it's necessary to create a brand new
PrintServerChannel rather than simply revert ByteStreamServerChannel back to the
blocking behavior it used to have.  As the CL is written, it leaves
ByteStreamServerChannel as dead code.

Also, please add some text to the API specification to clarify that it is now
the client's responsibility to avoid deadlock.

[Brian Wilkerson, 2015-01-23 16:08:10]

Paul's understanding matches mine. I wouldn't have expected that this code would
work, but I'm guessing that you tested it, so I'm confused.

[scheglov, 2015-01-23 16:27:40]

PTAL


@Paul:

1. You are right, we need to enable "print" function always and get rid of the
corresponding server option.

2. No, using blocking stdout is not enough. We still don't get server status
notifications when we start running pub. So, user does not know what happens.

3. The existing ByteStreamServerChannel class is not used currently, but it
might be used it the future. I can remove it in the following CL.

4. I've added a specification tweak.

https://codereview.chromium.org/872633002/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/internal/remote/ByteResponseStream.java
(right):

https://codereview.chromium.org/872633002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/internal/remote/ByteResponseStream.java:57:
if (!line.contains("\"result\":{\"version\":\"0.0.1\"}")) {
On 2015/01/23 14:16:53, danrubel wrote:
> I believe that this check can now be removed. We are not using getVersion to
> check health.

Done.

https://codereview.chromium.org/872633002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.server/src/com/google/dart/server/internal/remote/ByteResponseStream.java:109:
String line = lineQueue.take();
On 2015/01/23 14:16:53, danrubel wrote:
> Do we need to check for EOF_LINE after take ? Is this a potential race
> condition?

No, it isn't.
This function is the only reader from the queue.
And this queue works as FIFO.

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

https://codereview.chromium.org/872633002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:472: jsonObject['time'] = new
DateTime.now().millisecondsSinceEpoch;
On 2015/01/23 14:16:53, danrubel wrote:
> Good idea, but I already have a pending CL to capture the request time.
> https://codereview.chromium.org/865383002/
> Please remove this.

Done.

https://codereview.chromium.org/872633002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:833: jsonObject['time'] = new
DateTime.now().millisecondsSinceEpoch;
On 2015/01/23 14:16:53, danrubel wrote:
> Please remove this as described above.

Done.

[Brian Wilkerson, 2015-01-23 16:39:31]

LGTM

> 3. The existing ByteStreamServerChannel class is not used currently, but it
> might be used it the future. I can remove it in the following CL.

I'd like to leave the code in until we're talked to the VM team to understand
what our long term approach to IO ought to be.

[scheglov, 2015-01-23 21:04:57]

PTAL

It seems from my experiment on Linux with a big Dart project that using
non-blocking stdout works for sending "pub" notification.
So, there are no reasons to use "print".
But we need to get rid of the local buffering in the channel.

[Paul Berry, 2015-01-23 21:26:00]

lgtm, with one comment below.  Thanks for looking into this, Konstantin.

https://codereview.chromium.org/872633002/diff/60001/pkg/analysis_server/lib/...
File pkg/analysis_server/lib/src/channel/byte_stream_channel.dart (right):

https://codereview.chromium.org/872633002/diff/60001/pkg/analysis_server/lib/...
pkg/analysis_server/lib/src/channel/byte_stream_channel.dart:148:
stdout.writeln(s);
Did you mean to do _output.writeln()?

[danrubel, 2015-01-23 21:27:15]

LGTM.

[scheglov, 2015-01-23 21:33:46]

Committed patchset #4 (id:60001) manually as 43123 (presubmit successful).