(GH-231) Make document validation asynchronous

The document validation/diagnostics part of the language server is executed on
every document open or change event. While normally this is ok, when typing fast
on large documents, this can create slowness. Particularly as we add more
features which require longer running diagnostic processes.  This commit uses
a thread safe queue system which will only allow the document to exist in
the queue once.  Safeguards are also added which will ignore validation results
if the document is changed mid-validation.

Author: glennsarti

client/CHANGELOG.md

@@ -7,6 +7,7 @@ Check [Keep a Changelog](http://keepachangelog.com/) for recommendations on how
 ## Unreleased
 
 - ([GH-225](https://github.com/jpogran/puppet-vscode/issues/225)) Readd Local Workspace comand line option
+- ([GH-231](https://github.com/jpogran/puppet-vscode/issues/231)) Make Document Validation asynchronous
 
 ## 0.9.0 - 2018-02-01
 

server/lib/puppet-languageserver.rb

@@ -5,7 +5,7 @@
   require 'languageserver/languageserver'
   require 'puppet-vscode'
 
-  %w[json_rpc_handler message_router server_capabilities document_validator puppet_parser_helper puppet_helper
+  %w[json_rpc_handler message_router validation_queue server_capabilities document_validator puppet_parser_helper puppet_helper
      facter_helper completion_provider hover_provider definition_provider puppet_monkey_patches].each do |lib|
     begin
       require "puppet-languageserver/#{lib}"

server/lib/puppet-languageserver/message_router.rb

@@ -297,14 +297,7 @@ def receive_notification(method, params)
         content = params['textDocument']['text']
         doc_version = params['textDocument']['version']
         documents.set_document(file_uri, content, doc_version)
-        case document_type(file_uri)
-        when :manifest
-          reply_diagnostics(file_uri, PuppetLanguageServer::DocumentValidator.validate(content, @workspace))
-        when :epp
-          reply_diagnostics(file_uri, PuppetLanguageServer::DocumentValidator.validate_epp(content, @workspace))
-        else
-          reply_diagnostics(file_uri, [])
-        end
+        PuppetLanguageServer::ValidationQueue.enqueue(file_uri, doc_version, @workspace, self)
 
       when 'textDocument/didClose'
         PuppetLanguageServer.log_message(:info, 'Received textDocument/didClose notification.')
@@ -317,14 +310,7 @@ def receive_notification(method, params)
         content = params['contentChanges'][0]['text'] # TODO: Bad hardcoding zero
         doc_version = params['textDocument']['version']
         documents.set_document(file_uri, content, doc_version)
-        case document_type(file_uri)
-        when :manifest
-          reply_diagnostics(file_uri, PuppetLanguageServer::DocumentValidator.validate(content, @workspace))
-        when :epp
-          reply_diagnostics(file_uri, PuppetLanguageServer::DocumentValidator.validate_epp(content, @workspace))
-        else
-          reply_diagnostics(file_uri, [])
-        end
+        PuppetLanguageServer::ValidationQueue.enqueue(file_uri, doc_version, @workspace, self)
 
       when 'textDocument/didSave'
         PuppetLanguageServer.log_message(:info, 'Received textDocument/didSave notification.')

server/lib/puppet-languageserver/validation_queue.rb

@@ -0,0 +1,135 @@
+module PuppetLanguageServer
+  # Module for enqueing and running document level validation asynchronously
+  # When adding a document to be validation, it will remove any validation requests for the same
+  # document in the queue so that only the latest document needs to be processed.
+  #
+  # It will also ignore sending back validation results to the client if the document is
+  # updated during the validation process
+  module ValidationQueue
+    @queue = []
+    @queue_mutex = Mutex.new
+    @queue_thread = nil
+
+    # Enqueue a file to be validated
+    def self.enqueue(file_uri, doc_version, workspace, connection_object)
+      document_type = connection_object.document_type(file_uri)
+
+      unless %i[manifest epp].include?(document_type)
+        # Can't validate these types so just emit an empty validation result
+        connection_object.reply_diagnostics(file_uri, [])
+        return
+      end
+
+      @queue_mutex.synchronize do
+        @queue.reject! { |item| item['file_uri'] == file_uri }
+
+        @queue << {
+          'file_uri'          => file_uri,
+          'doc_version'       => doc_version,
+          'document_type'     => document_type,
+          'workspace'         => workspace,
+          'connection_object' => connection_object
+        }
+      end
+
+      if @queue_thread.nil? || !@queue_thread.alive?
+        @queue_thread = Thread.new do
+          begin
+            worker
+          rescue => err # rubocop:disable Style/RescueStandardError
+            PuppetLanguageServer.log_message(:error, "Error in ValidationQueue Thread: #{err}")
+            raise
+          end
+        end
+      end
+
+      nil
+    end
+
+    # Synchronously validate a file
+    def self.validate_sync(file_uri, doc_version, workspace, connection_object)
+      document_type = connection_object.document_type(file_uri)
+      content = documents.document(file_uri, doc_version)
+      return nil if content.nil?
+      result = validate(document_type, content, workspace)
+
+      # Send the response
+      connection_object.reply_diagnostics(file_uri, result)
+    end
+
+    # Helper method to the Document Store
+    def self.documents
+      PuppetLanguageServer::DocumentStore
+    end
+
+    # Wait for the queue to become empty
+    def self.drain_queue
+      return if @queue_thread.nil? || !@queue_thread.alive?
+      @queue_thread.join
+      nil
+    end
+
+    # Testing helper resets the queue and prepopulates it with
+    # a known arbitrary configuration.
+    # ONLY USE THIS FOR TESTING!
+    def self.reset_queue(initial_state = [])
+      @queue_mutex.synchronize do
+        @queue = initial_state
+      end
+    end
+
+    # Validate a document
+    def self.validate(document_type, content, workspace)
+      # Perform validation
+      case document_type
+      when :manifest
+        PuppetLanguageServer::DocumentValidator.validate(content, workspace)
+      when :epp
+        PuppetLanguageServer::DocumentValidator.validate_epp(content, workspace)
+      else
+        []
+      end
+    end
+    private_class_method :validate
+
+    # Thread worker which processes all jobs in the queue and validates each document
+    # serially
+    def self.worker
+      work_item = nil
+      loop do
+        @queue_mutex.synchronize do
+          return if @queue.empty?
+          work_item = @queue.shift
+        end
+        return if work_item.nil?
+
+        file_uri          = work_item['file_uri']
+        doc_version       = work_item['doc_version']
+        connection_object = work_item['connection_object']
+        document_type     = work_item['document_type']
+        workspace         = work_item['workspace']
+
+        # Check if the document is the latest version
+        content = documents.document(file_uri, doc_version)
+        if content.nil?
+          PuppetLanguageServer.log_message(:debug, "ValidationQueue Thread: Ignoring #{work_item['file_uri']} as it is not the latest version or has been removed")
+          return
+        end
+
+        # Perform validation
+        result = validate(document_type, content, workspace)
+
+        # Check if the document is still latest version
+        current_version = documents.document_version(file_uri)
+        if current_version != doc_version
+          PuppetLanguageServer.log_message(:debug, "ValidationQueue Thread: Ignoring #{work_item['file_uri']} as has changed version from #{doc_version} to #{current_version}")
+          return
+        end
+
+        # Send the response
+        connection_object.reply_diagnostics(file_uri, result)
+      end
+    end
+    private_class_method :worker
+  end
+end

server/spec/languageserver/unit/puppet-languageserver/message_router_spec.rb

@@ -515,9 +515,7 @@
 
     # textDocument/didOpen - https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#textDocument_didOpen
     context 'given a textDocument/didOpen notification' do
-      let(:validation_result) { ['validation_result'] }
-
-      shared_examples_for "an opened document with validation errors" do |file_uri, file_content|
+      shared_examples_for "an opened document with enqueued validation" do |file_uri, file_content|
         let(:notification_method) { 'textDocument/didOpen' }
         let(:notification_params) { {
           'textDocument' => {
@@ -533,30 +531,8 @@
           expect(subject.documents.document(file_uri)).to eq(file_content)
         end
 
-        it 'should reply with diagnostic information on the file' do
-          expect(subject).to receive(:reply_diagnostics).with(file_uri,validation_result).and_return(true)
-          subject.receive_notification(notification_method, notification_params)
-        end
-      end
-
-      shared_examples_for "an opened document with no validation errors" do |file_uri, file_content|
-        let(:notification_method) { 'textDocument/didOpen' }
-        let(:notification_params) { {
-          'textDocument' => {
-            'uri' => file_uri,
-            'languageId' => 'puppet',
-            'version' => 1,
-            'text' => file_content,
-          }
-        }}
-
-        it 'should add the document to the document store' do
-          subject.receive_notification(notification_method, notification_params)
-          expect(subject.documents.document(file_uri)).to eq(file_content)
-        end
-
-        it 'should reply with empty diagnostic information on the file' do
-          expect(subject).to receive(:reply_diagnostics).with(file_uri,[]).and_return(true)
+        it 'should enqueue the file for validation' do
+          expect(PuppetLanguageServer::ValidationQueue).to receive(:enqueue).with(file_uri, 1, Object, Object)
           subject.receive_notification(notification_method, notification_params)
         end
       end
@@ -566,33 +542,19 @@
       end
 
       context 'for a puppet manifest file' do
-        before(:each) do
-          expect(PuppetLanguageServer::DocumentValidator).to receive(:validate).and_return(validation_result)
-          allow(subject).to receive(:reply_diagnostics).and_return(true)
-        end
-        it_should_behave_like "an opened document with validation errors", MANIFEST_FILENAME, ERROR_CAUSING_FILE_CONTENT
+        it_should_behave_like "an opened document with enqueued validation", MANIFEST_FILENAME, ERROR_CAUSING_FILE_CONTENT
       end
 
       context 'for a Puppetfile file' do
-        before(:each) do
-          allow(subject).to receive(:reply_diagnostics).and_return(true)
-        end
-        it_should_behave_like "an opened document with no validation errors", PUPPETFILE_FILENAME, ERROR_CAUSING_FILE_CONTENT
+        it_should_behave_like "an opened document with enqueued validation", PUPPETFILE_FILENAME, ERROR_CAUSING_FILE_CONTENT
       end
 
       context 'for an EPP template file' do
-        before(:each) do
-          expect(PuppetLanguageServer::DocumentValidator).to receive(:validate_epp).and_return(validation_result)
-          allow(subject).to receive(:reply_diagnostics).and_return(true)
-        end
-        it_should_behave_like "an opened document with validation errors", EPP_FILENAME, ERROR_CAUSING_FILE_CONTENT
+        it_should_behave_like "an opened document with enqueued validation", EPP_FILENAME, ERROR_CAUSING_FILE_CONTENT
       end
 
       context 'for an unknown file' do
-        before(:each) do
-          allow(subject).to receive(:reply_diagnostics).and_return(true)
-        end
-        it_should_behave_like "an opened document with no validation errors", UNKNOWN_FILENAME, ERROR_CAUSING_FILE_CONTENT
+        it_should_behave_like "an opened document with enqueued validation", UNKNOWN_FILENAME, ERROR_CAUSING_FILE_CONTENT
       end
     end
 
@@ -618,37 +580,7 @@
 
     # textDocument/didChange - https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#didchangetextdocument-notification
     context 'given a textDocument/didChange notification and a TextDocumentSyncKind of Full' do
-      let(:validation_result) { ['validation_result'] }
-
-      shared_examples_for "a changed document with validation errors" do |file_uri, new_file_content|
-        let(:notification_params) { {
-          'textDocument' => {
-            'uri' => file_uri,
-            'version' => 2,
-          },
-          'contentChanges' => [
-            {
-              'range' => nil,
-              'rangeLength' => nil,
-              'text' => new_file_content,
-            }
-          ]
-        }}
-        let(:notification_method) { 'textDocument/didChange' }
-        let(:new_file_content ) { 'new_file_content' }
-
-        it 'should update the document in the document store' do
-          subject.receive_notification(notification_method, notification_params)
-          expect(subject.documents.document(file_uri)).to eq(new_file_content)
-        end
-
-        it 'should reply with diagnostic information on the file' do
-          expect(subject).to receive(:reply_diagnostics).with(file_uri, validation_result).and_return(true)
-          subject.receive_notification(notification_method, notification_params)
-        end
-      end
-
-      shared_examples_for "a changed document with no validation errors" do |file_uri, new_file_content|
+      shared_examples_for "a changed document with enqueued validation" do |file_uri, new_file_content|
         let(:notification_params) { {
           'textDocument' => {
             'uri' => file_uri,
@@ -670,8 +602,8 @@
           expect(subject.documents.document(file_uri)).to eq(new_file_content)
         end
 
-        it 'should reply with empty diagnostic information on the file' do
-          expect(subject).to receive(:reply_diagnostics).with(file_uri, []).and_return(true)
+        it 'should enqueue the file for validation' do
+          expect(PuppetLanguageServer::ValidationQueue).to receive(:enqueue).with(file_uri, 2, Object, Object)
           subject.receive_notification(notification_method, notification_params)
         end
       end
@@ -681,34 +613,19 @@
       end
 
       context 'for a puppet manifest file' do
-        before(:each) do
-          expect(PuppetLanguageServer::DocumentValidator).to receive(:validate).and_return(validation_result)
-          allow(subject).to receive(:reply_diagnostics).and_return(true)
-        end
-        it_should_behave_like "a changed document with validation errors", MANIFEST_FILENAME, ERROR_CAUSING_FILE_CONTENT
+        it_should_behave_like "a changed document with enqueued validation", MANIFEST_FILENAME, ERROR_CAUSING_FILE_CONTENT
       end
 
       context 'for a Puppetfile file' do
-        before(:each) do
-          allow(subject).to receive(:reply_diagnostics).and_return(true)
-        end
-        it_should_behave_like "a changed document with no validation errors", PUPPETFILE_FILENAME, ERROR_CAUSING_FILE_CONTENT
+        it_should_behave_like "a changed document with enqueued validation", PUPPETFILE_FILENAME, ERROR_CAUSING_FILE_CONTENT
       end
 
       context 'for an EPP template file' do
-        before(:each) do
-          expect(PuppetLanguageServer::DocumentValidator).to receive(:validate_epp).and_return(validation_result)
-          allow(subject).to receive(:reply_diagnostics).and_return(true)
-        end
-        it_should_behave_like "a changed document with validation errors", EPP_FILENAME, ERROR_CAUSING_FILE_CONTENT
+        it_should_behave_like "a changed document with enqueued validation", EPP_FILENAME, ERROR_CAUSING_FILE_CONTENT
       end
 
       context 'for a file the server does not understand' do
-        before(:each) do
-          expect(PuppetLanguageServer::DocumentValidator).to receive(:validate).exactly(0).times
-          allow(subject).to receive(:reply_diagnostics).and_return(true)
-        end
-        it_should_behave_like "a changed document with no validation errors", UNKNOWN_FILENAME, ERROR_CAUSING_FILE_CONTENT
+        it_should_behave_like "a changed document with enqueued validation", UNKNOWN_FILENAME, ERROR_CAUSING_FILE_CONTENT
       end
     end