Add ServerOperation and queue.

pkg/analysis_server/lib/src/analysis_server.dart

 // Copyright (c) 2014, 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.
 
 library analysis.server;
 
 import 'dart:async';
 
 import 'package:analysis_server/src/analysis_logger.dart';
 import 'package:analysis_server/src/channel.dart';
 import 'package:analysis_server/src/constants.dart';
 import 'package:analysis_server/src/context_directory_manager.dart';
 import 'package:analysis_server/src/domain_analysis.dart';
+import 'package:analysis_server/src/operation/operation.dart';
+import 'package:analysis_server/src/operation/operation_queue.dart';
 import 'package:analysis_server/src/protocol.dart';
 import 'package:analysis_server/src/resource.dart';
 import 'package:analyzer/src/generated/ast.dart';
 import 'package:analyzer/src/generated/engine.dart';
 import 'package:analyzer/src/generated/error.dart';
 import 'package:analyzer/src/generated/java_core.dart';
 import 'package:analyzer/src/generated/source.dart';
 import 'package:analyzer/src/generated/sdk.dart';
 import 'package:analyzer/src/generated/sdk_io.dart';
 import 'package:analyzer/src/generated/source_io.dart';
 import 'package:analysis_server/src/computers.dart';
 
 
 /**
  * An instance of [DirectoryBasedDartSdk] that is shared between
  * [AnalysisServer] instances to improve performance.
  */
 final DirectoryBasedDartSdk SHARED_SDK = DirectoryBasedDartSdk.defaultSdk;
 
 class AnalysisServerContextDirectoryManager extends ContextDirectoryManager {
   final AnalysisServer analysisServer;
 
   AnalysisServerContextDirectoryManager(this.analysisServer, ResourceProvider resourceProvider)
       : super(resourceProvider);
 
   void addContext(Folder folder, File pubspecFile) {
     ContextDirectory contextDirectory = new ContextDirectory(
         analysisServer.defaultSdk, folder, pubspecFile);
     analysisServer.folderMap[folder] = contextDirectory;
-    analysisServer.addContextToWorkQueue(contextDirectory.context);
+    analysisServer.schedulePerformAnalysisOperation(contextDirectory.context, false);
   }
 
   void applyChangesToContext(Folder contextFolder, ChangeSet changeSet) {
     analysisServer.folderMap[contextFolder].context.applyChanges(changeSet);
   }
 }
 
 /**
  * Instances of the class [AnalysisServer] implement a server that listens on a
  * [CommunicationChannel] for analysis requests and process them.
@@ skipping 17 matching lines @@
    * discard any tasks it finds on [contextWorkQueue].
    */
   bool running;
 
   /**
    * A list of the request handlers used to handle the requests sent to this
    * server.
    */
   List<RequestHandler> handlers;
 
-  // TODO(scheglov) remove once setAnalysisRoots() is completely implemented
-//  /**
-//   * A table mapping context id's to the analysis contexts associated with them.
-//   */
-//  final Map<String, AnalysisContext> contextMap = new Map<String, AnalysisContext>();
-//
-//  /**
-//   * A table mapping analysis contexts to the context id's associated with them.
-//   */
-//  final Map<AnalysisContext, String> contextIdMap = new Map<AnalysisContext, String>();
-
   /**
    * The current default [DartSdk].
    */
   DartSdk defaultSdk = SHARED_SDK;
 
   /**
    * A table mapping [Folder]s to the [ContextDirectory]s associated with them.
    */
   final Map<Folder, ContextDirectory> folderMap = <Folder, ContextDirectory>{};
 
   /**
    * The context identifier used in the last status notification.
    */
   String lastStatusNotificationContextId = null;
 
-  /**
-   * A list of the analysis contexts for which analysis work needs to be
-   * performed.
-   *
-   * Invariant: when this list is non-empty, there is exactly one pending call

[Paul Berry, 2014/06/02 16:04:41]

Why is this comment being deleted?  Is your intent to establish a different
invariant?  If so, what?  I think this invariant was a good one.

[scheglov, 2014/06/02 17:56:59]

Restored for the operation queue field.

-   * to [performTask] on the event queue.  When this list is empty, there are
-   * no calls to [performTask] on the event queue.
-   */
-  final List<AnalysisContext> contextWorkQueue = new List<AnalysisContext>();
+  final ServerOperationQueue operationQueue = new ServerOperationQueue();
 
   /**
    * A set of the [ServerService]s to send notifications for.
    */
   Set<ServerService> serverServices = new Set<ServerService>();
 
   /**
    * A table mapping [AnalysisService]s to the file paths for which these
    * notifications should be sent.
    */
   Map<AnalysisService, Set<String>> analysisServices = <AnalysisService, Set<String>>{};
 
   /**
    * Initialize a newly created server to receive requests from and send
    * responses to the given [channel].
    */
   AnalysisServer(this.channel, ResourceProvider resourceProvider) {
     contextDirectoryManager = new AnalysisServerContextDirectoryManager(this, resourceProvider);
     AnalysisEngine.instance.logger = new AnalysisLogger();
     running = true;
     Notification notification = new Notification(NOTIFICATION_CONNECTED);
     channel.sendNotification(notification);
     channel.listen(handleRequest, onDone: done, onError: error);
   }
 
   /**
-   * If [running] is true, add the given [context] to the list of analysis
-   * contexts for which analysis work needs to be performed, and ensure that
-   * the work will be performed.
+   * Schedules analysis of the given context.
+   *
+   * [isContinue] is `true` if the new operation is continuation of analysis of
+   * the same context which was analyzed before.
    */
-  void addContextToWorkQueue(AnalysisContext context) {
-    if (!running) {
-      return;
-    }
-    if (!contextWorkQueue.contains(context)) {
-      contextWorkQueue.add(context);
-      if (contextWorkQueue.length == 1) {
-        // Work queue was previously empty, so schedule analysis.
-        _scheduleTask();
-      }
-    }
+  void schedulePerformAnalysisOperation(AnalysisContext context, bool isContinue) {
+//    bool isPriority = priorityContexts.contains(contextId);
+    // TODO(scheglov) support for priority sources
+    bool isPriority = false;
+    operationQueue.add(new PerformAnalysisOperation(context, isPriority, isContinue));
+    _scheduleTask();

[Brian Wilkerson, 2014/06/02 14:32:18]

I believe that this will cause the event queue to become flooded with calls to
performTask (because performTask also schedules itself).

I think the better design is to have a single method that adds an operation to
the queue and conditionally schedules performTask to be run if the queue was
previously empty. Perhaps we should pass a closure (_scheduleTask) into "add"
that gets run iff the queue was previously empty.

[Paul Berry, 2014/06/02 16:04:41]

I'm not sure we even need a closure.  I think it is sufficient to do:

bool wasEmpty = operationQueue.isEmpty;
operationQueue.add(...);
if (wasEmpty) {
  _scheduleTask();
}

(which is essentially the logic we had before this CL, except that previously we
had a .length method so we could simply check whether length was 1 after adding
the task).

   }
 
   /**
    * The socket from which requests are being read has been closed.
    */
   void done() {
     running = false;
   }
 
   /**
@@ skipping 18 matching lines @@
         }
       } on RequestFailure catch (exception) {
         channel.sendResponse(exception.response);
         return;
       }
     }
     channel.sendResponse(new Response.unknownRequest(request));
   }
 
   /**
-   * Perform the next available task. If a request was received that has not yet
-   * been performed, perform it next. Otherwise, look for some analysis that
-   * needs to be done and do that. Otherwise, do nothing.
+   * Perform the next available [ServerOperation].
    */
   void performTask() {
     if (!running) {
-      // An error has occurred, or the connection to the client has been
-      // closed, since performTask() was scheduled on the event queue.  So
-      // don't do any analysis.  Instead clear the work queue.
-      contextWorkQueue.clear();

[Paul Berry, 2014/06/02 16:04:41]

Why are we getting rid of this code?  Was it incorrect?

[scheglov, 2014/06/02 17:56:59]

Restored.

-    }
-    if (contextWorkQueue.isEmpty) {
-      // Nothing to do.
       return;
     }
-    //
-    // Look for a context that has work to be done and then perform one task.
-    //
-    List<ChangeNotice> notices = null;
-//    String contextId;
+    // prepare next operation
+    ServerOperation operation = operationQueue.take();
+    if (operation == null) {
+      return;
+    }
+    // perform the operation
     try {
-      AnalysisContext context = contextWorkQueue[0];
-//      contextId = contextIdMap[context];
-      // TODO(danrubel): Replace with context identifier or similar
-      sendStatusNotification(context.toString());
-      AnalysisResult result = context.performAnalysisTask();
-      notices = result.changeNotices;
-    } finally {

[Paul Berry, 2014/06/02 16:04:41]

Why are we dropping this logic?  I believe that scheduling the next task in a
finally block is the right thing to do.  If we don't, then it means that a
single exception will effectively grind the whole server to a halt, because it
breaks the invariant.

[scheglov, 2014/06/02 17:56:59]

Restored in performTask/performOperation.

-      if (notices == null) {
-        // Either we have no more work to do for this context, or there was an
-        // unhandled exception trying to perform the analysis.  In either case,
-        // remove the context form the work queue so we won't try to do more
-        // analysis on it.
-        contextWorkQueue.removeAt(0);
-      }
-      //
-      // Schedule this method to be run again if there is any more work to be
-      // done.
-      //
-      if (!contextWorkQueue.isEmpty) {
-        _scheduleTask();
-      }
+      operation.perform(this);
+    } catch (e) {
+      // TODO(scheglov) decide how to handle exceptions
     }
+    // schedule this method again
+    _scheduleTask();

[Brian Wilkerson, 2014/06/02 14:32:18]

This method should only get scheduled if the queue is not empty.

[scheglov, 2014/06/02 17:56:59]

Done.

+  }
+
+  /**
+   * Perform analysis in the given [AnalysisContext].
+   */
+  void internalPerformAnalysis(AnalysisContext context) {

[Brian Wilkerson, 2014/06/02 14:32:18]

I would prefer a style in which the code to perform an operation were in the
operation's perform() method rather than having to implement an internal method
for every operation.

[scheglov, 2014/06/02 17:56:59]

Me too, but I'm afraid we have to expose too much internals of AnalysisServer to
do so.

[Brian Wilkerson, 2014/06/02 18:21:03]

I'd like to start with doing it the better way until we know that it won't work.

+    // prepare results
+    AnalysisResult result = context.performAnalysisTask();
+    List<ChangeNotice> notices = result.changeNotices;
+    if (notices == null) {
+      return;
+    }
+    // TODO(scheglov) remember known sources
+    // TODO(scheglov) index units
+    // TODO(scheglov) schedule notifications
     if (notices != null) {
       sendNotices(notices);
     } else {
       sendStatusNotification(null);
     }
+    // schedule analysis again
+    schedulePerformAnalysisOperation(context, true);

[Brian Wilkerson, 2014/06/02 14:32:18]

We need to take the "continuation" flag into account when scheduling analysis
operations. Otherwise we're going to be using more memory than necessary at any
given time and for longer periods of time.

[scheglov, 2014/06/02 17:56:59]

We do use it - PerformAnalysisOperation returns different priority depending on
if it is a continuation or not.

[Brian Wilkerson, 2014/06/02 18:21:03]

I meant, if we are continuing analysis of a context then that operation should
be added at the head of the queue rather than the tail.

[scheglov, 2014/06/02 18:32:25]

Well, it is added to the tail of the ANALYSIS_CONTINUE queue, which is
practically always has 0 or 1 item.

   }
 
   /**
    * Send the information in the given list of notices back to the client.
    */
   void sendNotices(List<ChangeNotice> notices) {
     for (int i = 0; i < notices.length; i++) {
       ChangeNotice notice = notices[i];
       Source source = notice.source;
       CompilationUnit dartUnit = notice.compilationUnit;
@@ skipping 81 matching lines @@
     changes.forEach((file, change) {
       AnalysisContext analysisContext = _getAnalysisContext(file);
       if (analysisContext != null) {
         Source source = _getSource(file);
         if (change.offset == null) {
           analysisContext.setContents(source, change.content);
         } else {
           analysisContext.setChangedContents(source, change.content,
               change.offset, change.oldLength, change.newLength);
         }
-        addContextToWorkQueue(analysisContext);
+        schedulePerformAnalysisOperation(analysisContext, false);
       }
     });
   }
 
   /**
    * Implementation for `analysis.setSubscriptions`.
    */
   void setAnalysisSubscriptions(Map<AnalysisService, Set<String>> subscriptions) {
     // send notifications for already analyzed sources
     subscriptions.forEach((service, Set<String> newFiles) {
@@ skipping 56 matching lines @@
       return null;
     }
     // get a resolved unit
     return context.getResolvedCompilationUnit2(unitSource, librarySources[0]);
   }
 
   /**
    * Return `true` if all tasks are finished in this [AnalysisServer].
    */
   bool test_areTasksFinished() {
-    return contextWorkQueue.isEmpty;
+    return operationQueue.isEmpty;
   }
 
   static Map<String, Object> errorToJson(AnalysisError analysisError) {
     // TODO(paulberry): move this function into the AnalysisError class.
     ErrorCode errorCode = analysisError.errorCode;
     Map<String, Object> result = {
       'file': analysisError.source.fullName,
       // TODO(scheglov) add Enum.fullName ?
       'errorCode': '${errorCode.runtimeType}.${(errorCode as Enum).name}',
       'offset': analysisError.offset,
@@ skipping 84 matching lines @@
 /**
  * An enumeration of the services provided by the server domain.
  */
 class ServerService extends Enum2<ServerService> {
   static const ServerService STATUS = const ServerService('STATUS', 0);
 
   static const List<ServerService> VALUES = const [STATUS];
 
   const ServerService(String name, int ordinal) : super(name, ordinal);
 }