support exclusive locking of PQ dir access

fix agent and pipeline and specs for queue exclusive access

added comments and swapped all sleep 0.01 to 0.1

revert explicit pipeline close in specs using sample helper

fix multiple pipelines specs

use BasePipeline for config validation which does not instantiate a new queue

review modifications

improve queue exception message

logstash-core/lib/logstash/agent.rb

@@ -189,7 +189,22 @@ def running_pipelines?
     end
   end
 
+  def close_pipeline(id)
+    pipeline = @pipelines[id]
+    if pipeline
+      @logger.warn("closing pipeline", :id => id)
+      pipeline.close
+    end
+  end
+
+  def close_pipelines
+    @pipelines.each  do |id, _|
+      close_pipeline(id)
+    end
+  end
+
   private
+
   def start_webserver
     options = {:http_host => @http_host, :http_ports => @http_port, :http_environment => @http_environment }
     @webserver = LogStash::WebServer.new(@logger, self, options)
@@ -229,17 +244,17 @@ def collect_metrics?
     @collect_metric
   end
 
-  def increment_reload_failures_metrics(id, config, exception)
+  def increment_reload_failures_metrics(id, message, backtrace = nil)
     @instance_reload_metric.increment(:failures)
     @pipeline_reload_metric.namespace([id.to_sym, :reloads]).tap do |n|
       n.increment(:failures)
-      n.gauge(:last_error, { :message => exception.message, :backtrace => exception.backtrace})
+      n.gauge(:last_error, { :message => message, :backtrace =>backtrace})
       n.gauge(:last_failure_timestamp, LogStash::Timestamp.now)
     end
     if @logger.debug?
-      @logger.error("Cannot load an invalid configuration.", :reason => exception.message, :backtrace => exception.backtrace)
+      @logger.error("Cannot load an invalid configuration", :reason => message, :backtrace => backtrace)
     else
-      @logger.error("Cannot load an invalid configuration.", :reason => exception.message)
+      @logger.error("Cannot load an invalid configuration", :reason => message)
     end
   end
 
@@ -261,7 +276,7 @@ def create_pipeline(settings, config = nil)
     begin
       LogStash::Pipeline.new(config, settings, metric)
     rescue => e
-      increment_reload_failures_metrics(settings.get("pipeline.id"), config, e)
+      increment_reload_failures_metrics(settings.get("pipeline.id"), e.message, e.backtrace)
       return nil
     end
   end
@@ -294,15 +309,14 @@ def reload_pipeline!(id)
     begin
       pipeline_validator = LogStash::BasePipeline.new(new_config, old_pipeline.settings)
     rescue => e
-      increment_reload_failures_metrics(id, new_config, e)
+      increment_reload_failures_metrics(id, e.message, e.backtrace)
       return
     end
 
     # check if the new pipeline will be reloadable in which case we want to log that as an error and abort
     if !pipeline_validator.reloadable?
       @logger.error(I18n.t("logstash.agent.non_reloadable_config_reload"), :pipeline_id => id, :plugins => pipeline_validator.non_reloadable_plugins.map(&:class))
-      # TODO: in the original code the failure metrics were not incremented, should we do it here?
-      # increment_reload_failures_metrics(id, new_config, e)
+      increment_reload_failures_metrics(id, "non reloadable pipeline")
       return
     end
 
@@ -331,20 +345,28 @@ def upgrade_pipeline(pipeline_id, settings, new_config)
       # this is a scenario where the configuration is valid (compilable) but the new pipeline refused to start
       # and at this point NO pipeline is running
       @logger.error("failed to create the reloaded pipeline and no pipeline is currently running", :pipeline => pipeline_id)
+      increment_reload_failures_metrics(pipeline_id, "failed to create the reloaded pipeline")
       return
     end
 
+    ### at this point pipeline#close must be called if upgrade_pipeline does not succeed
+
     # check if the new pipeline will be reloadable in which case we want to log that as an error and abort. this should normally not
     # happen since the check should be done in reload_pipeline! prior to get here.
     if !new_pipeline.reloadable?
       @logger.error(I18n.t("logstash.agent.non_reloadable_config_reload"), :pipeline_id => pipeline_id, :plugins => new_pipeline.non_reloadable_plugins.map(&:class))
+      increment_reload_failures_metrics(pipeline_id, "non reloadable pipeline")
+      new_pipeline.close
       return
     end
 
+    # @pipelines[pipeline_id] must be initialized before #start_pipeline below which uses it
     @pipelines[pipeline_id] = new_pipeline
 
     if !start_pipeline(pipeline_id)
       @logger.error("failed to start the reloaded pipeline and no pipeline is currently running", :pipeline => pipeline_id)
+      # do not call increment_reload_failures_metrics here since #start_pipeline already does it on failure
+      new_pipeline.close
       return
     end
 
@@ -373,6 +395,8 @@ def start_pipeline(id)
           n.gauge(:last_failure_timestamp, LogStash::Timestamp.now)
         end
         @logger.error("Pipeline aborted due to error", :exception => e, :backtrace => e.backtrace)
+
+        # TODO: this is weird, why dont we return directly here? any reason we need to enter the while true loop below?!
       end
     end
     while true do

logstash-core/lib/logstash/pipeline.rb

@@ -26,7 +26,7 @@ module LogStash; class BasePipeline
 
   attr_reader :config_str, :config_hash, :inputs, :filters, :outputs, :pipeline_id
 
-  def initialize(config_str, settings)
+  def initialize(config_str, settings = SETTINGS)
     @logger = self.logger
     @config_str = config_str
     @config_hash = Digest::SHA1.hexdigest(@config_str)
@@ -59,8 +59,7 @@ def initialize(config_str, settings)
     begin
       eval(config_code)
     rescue => e
-      # TODO: the original code rescue e but does nothing with it, should we re-raise to have original exception details!?
-      raise
+      raise e
     end
   end
 
@@ -139,7 +138,13 @@ def initialize(config_str, settings = SETTINGS, namespaced_metric = nil)
 
     super(config_str, settings)
 
-    @queue = LogStash::QueueFactory.create(settings)
+    begin
+      @queue = LogStash::QueueFactory.create(settings)
+    rescue => e
+      @logger.error("Logstash failed to create queue", "exception" => e, "backtrace" => e.backtrace)
+      raise e
+    end
+
     @input_queue_client = @queue.write_client
     @filter_queue_client = @queue.read_client
     @signal_queue = Queue.new
@@ -216,15 +221,19 @@ def run
     shutdown_flusher
     shutdown_workers
 
-    @filter_queue_client.close
-    @queue.close
+    close
 
     @logger.debug("Pipeline #{@pipeline_id} has been shutdown")
 
     # exit code
     return 0
   end # def run
 
+  def close
+    @filter_queue_client.close
+    @queue.close
+  end
+
   def transition_to_running
     @running.make_true
   end
@@ -327,7 +336,8 @@ def filter_batch(batch)
     # plugin.
     @logger.error("Exception in pipelineworker, the pipeline stopped processing new events, please check your filter configuration and restart Logstash.",
                   "exception" => e, "backtrace" => e.backtrace)
-    raise
+
+    raise e
   end
 
   # Take an array of events and send them to the correct output

logstash-core/lib/logstash/runner.rb

@@ -249,7 +249,7 @@ def execute
       config_loader = LogStash::Config::Loader.new(logger)
       config_str = config_loader.format_config(setting("path.config"), setting("config.string"))
       begin
-        LogStash::Pipeline.new(config_str)
+        LogStash::BasePipeline.new(config_str)
         puts "Configuration OK"
         logger.info "Using config.test_and_exit mode. Config Validation Result: OK. Exiting Logstash"
         return 0

logstash-core/spec/conditionals_spec.rb

@@ -287,7 +287,7 @@ def multi_receive(events)
     conditional "!([message] <= 'sample')" do
       sample("apple") { expect(subject.get("tags")).not_to include("success") }
       sample("zebra") { expect(subject.get("tags")).not_to include("failure") }
-      sample("sample") { expect(subject.get("tags")).not_to include("success") }
+      sample("sample") { expect(subject.get("tags")).not_to include("success")}
     end
 
     conditional "!([message] >= 'sample')" do