From 48c0eadb86a8e182efee4ae02c509f39100278ff Mon Sep 17 00:00:00 2001 From: Konstantin Shcheglov Date: Fri, 2 Apr 2021 23:36:52 +0000 Subject: [PATCH] Migrate more of analysis_server_client Added a few new methods to the listener, tests. Change-Id: I477a17da3fd31db04c4960effee076680db62301 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/193895 Reviewed-by: Brian Wilkerson Commit-Queue: Konstantin Shcheglov --- .../example/example.dart | 11 +- .../lib/handler/connection_handler.dart | 8 +- .../lib/listener/server_listener.dart | 16 +- pkg/analysis_server_client/lib/server.dart | 55 +++---- .../lib/src/protocol/protocol_base.dart | 4 +- .../lib/src/server_base.dart | 79 ++++++---- .../test/live_test.dart | 2 - .../test/server_test.dart | 147 +++++++++++++----- 8 files changed, 201 insertions(+), 121 deletions(-) diff --git a/pkg/analysis_server_client/example/example.dart b/pkg/analysis_server_client/example/example.dart index faddfd58747..9790d7818ce 100644 --- a/pkg/analysis_server_client/example/example.dart +++ b/pkg/analysis_server_client/example/example.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -// @dart = 2.9 - import 'dart:async'; import 'dart:io' show Directory, Platform, ProcessSignal, exit; @@ -37,7 +35,7 @@ void main(List args) async { AnalysisSetAnalysisRootsParams([target], const []).toJson()); // Continue to watch for analysis until the user presses Ctrl-C - StreamSubscription subscription; + late StreamSubscription subscription; subscription = ProcessSignal.sigint.watch().listen((_) async { print('Exiting...'); // ignore: unawaited_futures @@ -109,15 +107,14 @@ class _Handler with NotificationHandler, ConnectionHandler { } else { print('Server Error: ${params.message}'); } - if (params.stackTrace != null) { - print(params.stackTrace); - } + print(params.stackTrace); super.onServerError(params); } @override void onServerStatus(ServerStatusParams params) { - if (!params.analysis.isAnalyzing) { + var analysisStatus = params.analysis; + if (analysisStatus != null && !analysisStatus.isAnalyzing) { // Whenever the server stops analyzing, // print a brief summary of what issues have been found. if (errorCount == 0) { diff --git a/pkg/analysis_server_client/lib/handler/connection_handler.dart b/pkg/analysis_server_client/lib/handler/connection_handler.dart index 96045c028ff..f754fb588cc 100644 --- a/pkg/analysis_server_client/lib/handler/connection_handler.dart +++ b/pkg/analysis_server_client/lib/handler/connection_handler.dart @@ -1,12 +1,6 @@ // Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -// -// This file has been automatically generated. Please do not edit it manually. -// To regenerate the file, use the script -// "pkg/analysis_server/tool/spec/generate_files". - -// @dart = 2.9 import 'dart:async'; @@ -60,7 +54,7 @@ mixin ConnectionHandler implements NotificationHandler { /// Return a future that completes with a `bool` indicating whether /// a connection was successfully established with the server. - Future serverConnected({Duration timeLimit}) { + Future serverConnected({Duration? timeLimit}) { var future = _connected.future; if (timeLimit != null) { future = future.timeout(timeLimit, onTimeout: () { diff --git a/pkg/analysis_server_client/lib/listener/server_listener.dart b/pkg/analysis_server_client/lib/listener/server_listener.dart index 3f1b074cad1..60049963ac9 100644 --- a/pkg/analysis_server_client/lib/listener/server_listener.dart +++ b/pkg/analysis_server_client/lib/listener/server_listener.dart @@ -43,16 +43,28 @@ mixin ServerListener { /// Called when the [Server] receives an unexpected message /// which is not a notification or response. - void unexpectedMessage(Map message) { + void unexpectedMessage(Map message) { log('Unexpected message from server:', '$message'); } + /// Called when the [Server] received a response, which is not an error, + /// but the result is not an JSON object. + void unexpectedNotificationFormat(Map message) { + log('Unexpected notification format from server', '$message'); + } + /// Called when the [Server] recieved an unexpected response /// where the [id] does not match the [id] of an outstanding request. - void unexpectedResponse(Map message, id) { + void unexpectedResponse(Map message, Object id) { log('Unexpected response from server', 'id=$id'); } + /// Called when the [Server] received a response, which is not an error, + /// but the result is not an JSON object. + void unexpectedResponseFormat(Map message) { + log('Unexpected response format from server', '$message'); + } + /// Called when the server process unexpectedly exits /// with a non-zero exit code. void unexpectedStop(int exitCode) { diff --git a/pkg/analysis_server_client/lib/server.dart b/pkg/analysis_server_client/lib/server.dart index ae6d8f0819f..3d9dbb43353 100644 --- a/pkg/analysis_server_client/lib/server.dart +++ b/pkg/analysis_server_client/lib/server.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -// @dart = 2.9 - import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -23,18 +21,20 @@ export 'package:analysis_server_client/src/server_base.dart' class Server extends ServerBase { /// Server process object, or `null` if server hasn't been started yet /// or if the server has already been stopped. - Process _process; + Process? _process; /// The stderr subscription or `null` if either /// [listenToOutput] has not been called or [stop] has been called. - StreamSubscription _stderrSubscription; + StreamSubscription? _stderrSubscription; /// The stdout subscription or `null` if either /// [listenToOutput] has not been called or [stop] has been called. - StreamSubscription _stdoutSubscription; + StreamSubscription? _stdoutSubscription; Server( - {ServerListener listener, Process process, bool stdioPassthrough = false}) + {ServerListener? listener, + Process? process, + bool stdioPassthrough = false}) : _process = process, super(listener: listener, stdioPassthrough: stdioPassthrough); @@ -42,7 +42,7 @@ class Server extends ServerBase { @override Future kill({String reason = 'none'}) { listener?.killingServerProcess(reason); - final process = _process; + final process = _process!; _process = null; process.kill(); return process.exitCode; @@ -51,12 +51,12 @@ class Server extends ServerBase { /// Start listening to output from the server, /// and deliver notifications to [notificationProcessor]. @override - void listenToOutput({NotificationProcessor notificationProcessor}) { - _stdoutSubscription = _process.stdout + void listenToOutput({NotificationProcessor? notificationProcessor}) { + _stdoutSubscription = _process!.stdout .transform(utf8.decoder) .transform(LineSplitter()) .listen((line) => outputProcessor(line, notificationProcessor)); - _stderrSubscription = _process.stderr + _stderrSubscription = _process!.stderr .transform(utf8.decoder) .transform(LineSplitter()) .listen((line) => errorProcessor(line, notificationProcessor)); @@ -70,9 +70,9 @@ class Server extends ServerBase { /// If the server acknowledges the command with an error response, /// the future will be completed with an error. @override - Future> send( - String method, Map params) => - sendCommandWith(method, params, _process.stdin.add); + Future?> send( + String method, Map? params) => + sendCommandWith(method, params, _process!.stdin.add); /// Start the server. /// @@ -88,14 +88,14 @@ class Server extends ServerBase { /// locally for debugging. @override Future start({ - String clientId, - String clientVersion, - int diagnosticPort, - String instrumentationLogFile, + String? clientId, + String? clientVersion, + int? diagnosticPort, + String? instrumentationLogFile, bool profileServer = false, - String sdkPath, - String serverPath, - int servicesPort, + String? sdkPath, + String? serverPath, + int? servicesPort, bool suppressAnalytics = true, bool useAnalysisHighlight2 = false, bool enableAsserts = false, @@ -154,9 +154,10 @@ class Server extends ServerBase { useAnalysisHighlight2: useAnalysisHighlight2)); listener?.startingServer(dartBinary, arguments); - _process = await Process.start(dartBinary, arguments); + final process = await Process.start(dartBinary, arguments); + _process = process; // ignore: unawaited_futures - _process.exitCode.then((int code) { + process.exitCode.then((int code) { if (code != 0 && _process != null) { // Report an error if server abruptly terminated listener?.unexpectedStop(code); @@ -167,19 +168,21 @@ class Server extends ServerBase { /// Attempt to gracefully shutdown the server. /// If that fails, then kill the process. @override - Future stop({Duration timeLimit}) async { + Future stop({Duration? timeLimit}) async { timeLimit ??= const Duration(seconds: 5); - if (_process == null) { + + final process = _process; + if (process == null) { // Process already exited return -1; } + final future = send(SERVER_REQUEST_SHUTDOWN, null); - final process = _process; _process = null; await future // fall through to wait for exit .timeout(timeLimit, onTimeout: () { - return null; + return {}; }).whenComplete(() async { await _stderrSubscription?.cancel(); _stderrSubscription = null; diff --git a/pkg/analysis_server_client/lib/src/protocol/protocol_base.dart b/pkg/analysis_server_client/lib/src/protocol/protocol_base.dart index 139d71c0e7a..017d34cc4a0 100644 --- a/pkg/analysis_server_client/lib/src/protocol/protocol_base.dart +++ b/pkg/analysis_server_client/lib/src/protocol/protocol_base.dart @@ -34,7 +34,7 @@ class Notification { /// A table mapping the names of notification parameters to their values, or /// `null` if there are no notification parameters. - final Map? params; + final Map? params; /// Initialize a newly created [Notification] to have the given [event] name. /// If [params] is provided, it will be used as the params; otherwise no @@ -44,7 +44,7 @@ class Notification { /// Initialize a newly created instance based on the given JSON data. factory Notification.fromJson(Map json) { return Notification(json[Notification.EVENT], - json[Notification.PARAMS] as Map); + json[Notification.PARAMS] as Map); } /// Return a table representing the structure of the Json object that will be diff --git a/pkg/analysis_server_client/lib/src/server_base.dart b/pkg/analysis_server_client/lib/src/server_base.dart index 7aea73ef86f..dd9668c493f 100644 --- a/pkg/analysis_server_client/lib/src/server_base.dart +++ b/pkg/analysis_server_client/lib/src/server_base.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -// @dart = 2.9 - import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -17,12 +15,12 @@ import 'package:analysis_server_client/protocol.dart'; /// TODO(danrubel): Consider moving all cmdline argument consts /// out of analysis_server and into analysis_server_client. List getServerArguments({ - String clientId, - String clientVersion, - int diagnosticPort, - String instrumentationLogFile, - String sdkPath, - bool suppressAnalytics, + String? clientId, + String? clientVersion, + int? diagnosticPort, + String? instrumentationLogFile, + String? sdkPath, + bool suppressAnalytics = true, bool useAnalysisHighlight2 = false, }) { var arguments = []; @@ -74,24 +72,24 @@ abstract class ServerBase { /// If not `null`, [_listener] will be sent information /// about interactions with the server. - final ServerListener _listener; + final ServerListener? _listener; /// Commands that have been sent to the server but not yet acknowledged, /// and the [Completer] objects which should be completed /// when acknowledgement is received. - final _pendingCommands = >>{}; + final _pendingCommands = ?>>{}; - ServerBase({ServerListener listener, bool stdioPassthrough = false}) + ServerBase({ServerListener? listener, bool stdioPassthrough = false}) : _listener = listener, _stdioPassthrough = stdioPassthrough; - ServerListener get listener => _listener; + ServerListener? get listener => _listener; /// If the implementation of [ServerBase] captures an error stream, /// it can use this to forward the errors to [listener] and [stderr] if /// appropriate. void errorProcessor( - String line, NotificationProcessor notificationProcessor) { + String line, NotificationProcessor? notificationProcessor) { if (_stdioPassthrough) stderr.writeln(line); var trimmedLine = line.trim(); listener?.errorMessage(trimmedLine); @@ -110,7 +108,7 @@ abstract class ServerBase { /// decoding or message synchronization using [listener], and replicates /// raw data to [stdout] as appropriate. void outputProcessor( - String line, NotificationProcessor notificationProcessor) { + String line, NotificationProcessor? notificationProcessor) { if (_stdioPassthrough) stdout.writeln(line); var trimmedLine = line.trim(); @@ -127,7 +125,7 @@ abstract class ServerBase { } listener?.messageReceived(trimmedLine); - Map message; + Map message; try { message = json.decoder.convert(trimmedLine); } catch (exception) { @@ -135,31 +133,47 @@ abstract class ServerBase { return; } + // Handle response. final id = message[Response.ID]; if (id != null) { - // Handle response final completer = _pendingCommands.remove(id); if (completer == null) { listener?.unexpectedResponse(message, id); + return; } - if (message.containsKey(Response.ERROR)) { - completer.completeError(RequestError.fromJson( - ResponseDecoder(null), '.error', message[Response.ERROR])); - } else { - completer.complete(message[Response.RESULT]); + + final errorJson = message[Response.ERROR]; + if (errorJson != null) { + completer.completeError( + RequestError.fromJson(ResponseDecoder(null), '.error', errorJson)); + return; } - } else { - // Handle notification - final String event = message[Notification.EVENT]; - if (event != null) { + + final resultJson = message[Response.RESULT]; + if (resultJson is Map?) { + completer.complete(resultJson); + return; + } + + listener?.unexpectedResponseFormat(message); + return; + } + + // Handle notification. + final event = message[Notification.EVENT]; + if (event is String) { + final paramsJson = message[Notification.PARAMS]; + if (paramsJson is Map) { if (notificationProcessor != null) { - notificationProcessor( - Notification(event, message[Notification.PARAMS])); + notificationProcessor(Notification(event, paramsJson)); } } else { - listener?.unexpectedMessage(message); + listener?.unexpectedNotificationFormat(message); } + return; } + + listener?.unexpectedMessage(message); } /// Send a command to the server. An 'id' will be automatically assigned. @@ -169,18 +183,19 @@ abstract class ServerBase { /// the future will be completed with the 'result' field from the response. /// If the server acknowledges the command with an error response, /// the future will be completed with an error. - Future> send(String method, Map params); + Future?> send( + String method, Map? params); /// Encodes a request for transmission and sends it as a utf8 encoded byte /// string with [sendWith]. - Future> sendCommandWith( - String method, Map params, CommandSender sendWith) { + Future?> sendCommandWith( + String method, Map? params, CommandSender sendWith) { var id = '${_nextId++}'; var command = {Request.ID: id, Request.METHOD: method}; if (params != null) { command[Request.PARAMS] = params; } - final completer = Completer>(); + final completer = Completer?>(); _pendingCommands[id] = completer; var line = json.encode(command); listener?.requestSent(line); diff --git a/pkg/analysis_server_client/test/live_test.dart b/pkg/analysis_server_client/test/live_test.dart index 80b593d761b..e9ed329b8cc 100644 --- a/pkg/analysis_server_client/test/live_test.dart +++ b/pkg/analysis_server_client/test/live_test.dart @@ -2,8 +2,6 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -// @dart = 2.9 - import 'package:analysis_server_client/handler/connection_handler.dart'; import 'package:analysis_server_client/handler/notification_handler.dart'; import 'package:analysis_server_client/listener/server_listener.dart'; diff --git a/pkg/analysis_server_client/test/server_test.dart b/pkg/analysis_server_client/test/server_test.dart index 32aadc10392..286938de77f 100644 --- a/pkg/analysis_server_client/test/server_test.dart +++ b/pkg/analysis_server_client/test/server_test.dart @@ -2,23 +2,24 @@ // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. -// @dart = 2.9 - import 'dart:async'; import 'dart:convert'; import 'dart:io'; +import 'package:analysis_server_client/listener/server_listener.dart'; import 'package:analysis_server_client/protocol.dart'; import 'package:analysis_server_client/server.dart'; import 'package:test/test.dart'; void main() { - MockProcess process; - Server server; + late _ServerListener listener; + late MockProcess process; + late Server server; setUp(() async { process = MockProcess(); - server = Server(process: process); + listener = _ServerListener(); + server = Server(process: process, listener: listener); }); group('listenToOutput', () { @@ -29,7 +30,7 @@ void main() { final future = server.send('blahMethod', null); server.listenToOutput(); - final response = await future; + final response = (await future)!; expect(response['foo'], 'bar'); }); @@ -45,6 +46,7 @@ void main() { expect(error.code, RequestErrorCode.UNKNOWN_REQUEST); expect(error.message, 'something went wrong'); expect(error.stackTrace, 'some long stack trace'); + return {}; }); server.listenToOutput(); }); @@ -56,9 +58,10 @@ void main() { final completer = Completer(); void eventHandler(Notification notification) { expect(notification.event, 'fooEvent'); - expect(notification.params.length, 2); - expect(notification.params['foo'] as String, 'bar'); - expect(notification.params['baz'] as String, 'bang'); + var params = notification.params!; + expect(params.length, 2); + expect(params['foo'] as String, 'bar'); + expect(params['baz'] as String, 'bang'); completer.complete(); } @@ -67,6 +70,61 @@ void main() { server.listenToOutput(notificationProcessor: eventHandler); await completer.future; }); + + test('unexpected message', () async { + // No 'id', so not a response. + // No 'event', so not a notification. + process.stdout = Stream.value( + utf8.encoder.convert(json.encode({'foo': 'bar'})), + ); + process.stderr = _noMessage(); + + server.listenToOutput(); + + // Must happen for the test to pass. + await listener.unexpectedMessageController.stream.first; + }); + + test('unexpected notification format', () async { + process.stdout = Stream.value( + utf8.encoder.convert(json.encode({'event': 'foo', 'noParams': '42'})), + ); + process.stderr = _noMessage(); + + server.listenToOutput(); + + // Must happen for the test to pass. + await listener.unexpectedNotificationFormatCompleter.stream.first; + }); + + test('unexpected response', () async { + // We have no asked anything, but got a response. + process.stdout = Stream.value( + utf8.encoder.convert(json.encode({'id': '0'})), + ); + process.stderr = _noMessage(); + + server.listenToOutput(); + + // Must happen for the test to pass. + await listener.unexpectedResponseCompleter.stream.first; + }); + + test('unexpected response format', () async { + // We expect that the first request has id `0`. + // The response is invalid - the "result" field is not an object. + process.stdout = Stream.value( + utf8.encoder.convert(json.encode({'id': '0', 'result': '42'})), + ); + process.stderr = _noMessage(); + + // ignore: unawaited_futures + server.send('blahMethod', null); + server.listenToOutput(); + + // Must happen for the test to pass. + await listener.unexpectedResponseFormatCompleter.stream.first; + }); }); group('stop', () { @@ -151,16 +209,13 @@ class MockProcess implements Process { bool killed = false; @override - Stream> stderr; + late Stream> stderr; @override - Stream> stdout; + late Stream> stdout; @override - Future exitCode; - - @override - int get pid => null; + late Future exitCode; @override IOSink get stdin => mockin; @@ -171,43 +226,49 @@ class MockProcess implements Process { killed = true; return !wasKilled; } + + @override + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); } class MockStdin implements IOSink { final controller = StreamController(); - @override - Encoding encoding; - - @override - Future get done => null; - @override void add(List data) { controller.add(utf8.decode(data)); } @override - void addError(Object error, [StackTrace stackTrace]) {} - - @override - Future addStream(Stream> stream) => null; - - @override - Future close() => null; - - @override - Future flush() => null; - - @override - void write(Object obj) {} - - @override - void writeAll(Iterable objects, [String separator = '']) {} - - @override - void writeCharCode(int charCode) {} - - @override - void writeln([Object obj = '']) {} + dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation); +} + +class _ServerListener with ServerListener { + final unexpectedMessageController = StreamController(); + final unexpectedNotificationFormatCompleter = StreamController(); + final unexpectedResponseCompleter = StreamController(); + final unexpectedResponseFormatCompleter = StreamController(); + + @override + void log(String prefix, String details) {} + + @override + void unexpectedMessage(Map message) { + unexpectedMessageController.add(message); + } + + @override + void unexpectedNotificationFormat(Map message) { + unexpectedNotificationFormatCompleter.add(message); + } + + @override + void unexpectedResponse(Map message, Object id) { + unexpectedResponseCompleter.add(message); + } + + @override + void unexpectedResponseFormat(Map message) { + unexpectedResponseFormatCompleter.add(message); + } }