[Analyzer] Re-create progress tokens after each use + fix race

Change-Id: Ibc6dd59c71d40c5f0127a2881dade13cde8cfd87
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/173271
Commit-Queue: Danny Tuppeny <danny@tuppeny.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

Author: DanTup

pkg/analysis_server/lib/src/lsp/lsp_analysis_server.dart

@@ -155,9 +155,6 @@ class LspAnalysisServer extends AbstractAnalysisServer {
     channel.listen(handleMessage, onDone: done, onError: socketError);
     _pluginChangeSubscription =
         pluginManager.pluginsChanged.listen((_) => _onPluginsChanged());
-
-    analyzingProgressReporter =
-        ProgressReporter.serverCreated(this, analyzingProgressToken);
   }
 
   /// The capabilities of the LSP client. Will be null prior to initialization.
@@ -549,7 +546,7 @@ class LspAnalysisServer extends AbstractAnalysisServer {
 
   /// Send status notification to the client. The state of analysis is given by
   /// the [status] information.
-  void sendStatusNotification(nd.AnalysisStatus status) {
+  Future<void> sendStatusNotification(nd.AnalysisStatus status) async {
     // Send old custom notifications to clients that do not support $/progress.
     // TODO(dantup): Remove this custom notification (and related classes) when
     // it's unlikely to be in use by any clients.
@@ -563,9 +560,16 @@ class LspAnalysisServer extends AbstractAnalysisServer {
     }
 
     if (status.isAnalyzing) {
-      analyzingProgressReporter.begin('Analyzing…');
+      analyzingProgressReporter ??=
+          ProgressReporter.serverCreated(this, analyzingProgressToken)
+            ..begin('Analyzing…');
     } else {
-      analyzingProgressReporter.end();
+      if (analyzingProgressReporter != null) {
+        // Do not null this out until after end completes, otherwise we may try
+        // to create a new token before it's really completed.
+        await analyzingProgressReporter.end();
+        analyzingProgressReporter = null;
+      }
     }
   }
 

pkg/analysis_server/lib/src/lsp/progress.dart

@@ -2,6 +2,7 @@
 // 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.
 
+import 'dart:async';
 import 'dart:math';
 
 import 'package:analysis_server/lsp_protocol/protocol_generated.dart';
@@ -30,9 +31,9 @@ abstract class ProgressReporter {
   ProgressReporter._();
 
   // TODO(dantup): Add support for cancellable progress notifications.
-  void begin(String title, {String message});
+  FutureOr<void> begin(String title, {String message});
 
-  void end([String message]);
+  FutureOr<void> end([String message]);
 }
 
 class _NoopProgressReporter extends ProgressReporter {
@@ -45,7 +46,7 @@ class _NoopProgressReporter extends ProgressReporter {
 
 class _ServerCreatedProgressReporter extends _TokenProgressReporter {
   static final _random = Random();
-  Future<ResponseMessage> _tokenCreateRequest;
+  Future<bool> _tokenBeginRequest;
 
   _ServerCreatedProgressReporter(
     LspAnalysisServer server,
@@ -56,28 +57,39 @@ class _ServerCreatedProgressReporter extends _TokenProgressReporter {
         );
 
   @override
-  void begin(String title, {String message}) {
-    // Create the token lazily so we don't create it if it's not required.
-    _tokenCreateRequest ??= _server.sendRequest(
-        Method.window_workDoneProgress_create,
-        WorkDoneProgressCreateParams(token: _token));
-
-    // Chain onto the end of tokenCreateRequest so we do not try to use
-    // the token without the client accepting it.
-    _tokenCreateRequest.then((response) {
-      if (response.error != null) return;
+  Future<void> begin(String title, {String message}) async {
+    assert(_tokenBeginRequest == null,
+        'Begin should not be called more than once');
+
+    // Put the create/begin into a future so if end() is called before the
+    // begin is sent (which could happen because create is async), end will
+    // not be sent/return too early.
+    _tokenBeginRequest = _server
+        .sendRequest(Method.window_workDoneProgress_create,
+            WorkDoneProgressCreateParams(token: _token))
+        .then((response) {
+      // If the client did not create a token, do not send begin (and signal
+      // that we should also not send end).
+      if (response.error != null) return false;
       super.begin(title, message: message);
+      return true;
     });
+
+    await _tokenBeginRequest;
   }
 
   @override
-  void end([String message]) {
-    // Chain onto the end of tokenCreateRequest so we do not try to use
-    // the token without the client accepting it.
-    _tokenCreateRequest.then((response) {
-      if (response.error != null) return;
-      super.end(message);
-    });
+  Future<void> end([String message]) async {
+    // Only end the token after both create/begin have completed, and return
+    // a Future to indicate that has happened to callers know when it's safe
+    // to re-use the token identifier.
+    if (_tokenBeginRequest != null) {
+      final didBegin = await _tokenBeginRequest;
+      if (didBegin) {
+        super.end(message);
+      }
+      _tokenBeginRequest = null;
+    }
   }
 
   static String _randomTokenIdentifier() {

pkg/analysis_server/test/lsp/code_actions_refactor_test.dart

@@ -205,7 +205,7 @@ void newMethod() {
         findCommand(codeActions, Commands.performRefactor, extractMethodTitle);
     await verifyCodeActionEdits(
         codeAction, withoutMarkers(content), expectedContent,
-        workDoneToken: testWorkDoneToken);
+        workDoneToken: clientProvidedTestWorkDoneToken);
   }
 
   Future<void> test_progress_notSupported() async {

pkg/analysis_server/test/lsp/server_abstract.dart

@@ -51,8 +51,6 @@ abstract class AbstractLspAnalysisServerTest
   TestPluginManager pluginManager;
   LspAnalysisServer server;
 
-  final testWorkDoneToken = Either2<num, String>.t2('test');
-
   AnalysisServerOptions get serverOptions => AnalysisServerOptions();
 
   @override
@@ -474,6 +472,11 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
   static final allMarkersPattern =
       RegExp(allMarkers.map(RegExp.escape).join('|'));
 
+  /// A progress token used in tests where the client-provides the token, which
+  /// should not be validated as being created by the server first.
+  final clientProvidedTestWorkDoneToken =
+      Either2<num, String>.t2('client-test');
+
   int _id = 0;
   String projectFolderPath, mainFilePath, pubspecFilePath, analysisOptionsPath;
   Uri projectFolderUri, mainFileUri, pubspecFileUri, analysisOptionsUri;
@@ -1105,6 +1108,12 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
       }
     });
 
+    notificationsFromServer.listen((notification) async {
+      if (notification.method == Method.progress) {
+        await _handleProgress(notification);
+      }
+    });
+
     // Assume if none of the project options were set, that we want to default to
     // opening the test project folder.
     if (rootPath == null &&
@@ -1436,10 +1445,6 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
           }
 
           final params = ProgressParams.fromJson(message.params);
-          if (!validProgressTokens.contains(params.token)) {
-            throw Exception('Server sent a progress notification for a token '
-                'that has not been created: ${params.token}');
-          }
 
           // Skip unrelated progress notifications.
           if (params.token != analyzingProgressToken) {
@@ -1539,14 +1544,27 @@ mixin LspAnalysisServerTestMixin implements ClientCapabilitiesHelperMixin {
           T Function(Map<String, dynamic>) fromJson) =>
       (input) => input.cast<Map<String, dynamic>>().map(fromJson).toList();
 
+  Future<void> _handleProgress(NotificationMessage request) async {
+    final params = ProgressParams.fromJson(request.params);
+    if (params.token != clientProvidedTestWorkDoneToken &&
+        !validProgressTokens.contains(params.token)) {
+      throw Exception('Server sent a progress notification for a token '
+          'that has not been created: ${params.token}');
+    }
+
+    if (WorkDoneProgressEnd.canParse(params.value, nullLspJsonReporter)) {
+      validProgressTokens.remove(params.token);
+    }
+  }
+
   Future<void> _handleWorkDoneProgressCreate(RequestMessage request) async {
     if (_clientCapabilities.window?.workDoneProgress != true) {
       throw Exception('Server sent ${Method.window_workDoneProgress_create} '
           'but client capabilities do not allow');
     }
     final params = WorkDoneProgressCreateParams.fromJson(request.params);
     if (validProgressTokens.contains(params.token)) {
-      throw Exception('Server tried to create the same progress token twice');
+      throw Exception('Server tried to create already-active progress token');
     }
     validProgressTokens.add(params.token);
   }