Merge pull request #15 from yaauie/fix-recovery-from-webserver-crashes-etc

Improve malformed-input handling, restarting, and crash-recovery

Author: yaauie

CHANGELOG.md

@@ -1,3 +1,8 @@
+## 3.0.6
+  - Improve malformed-input handling by using updated FTW
+  - Improve webserver crash recovery
+  - Properly support plugin stopping & reloading
+
 ## 3.0.5
   - Update gemspec summary
 

lib/logstash/inputs/github.rb

@@ -42,6 +42,13 @@ def run(output_queue)
         response.body = "Accepted!"
     end
     @server.run
+  rescue Exception => original_exception
+    # If our server crashes, it may not have cleaned up after itself;
+    # since `FTW::WebServer#stop` is idempotent, make one last attempt
+    # before propagating the original exception.
+    @server && @server.stop rescue logger.error("Error while stopping FTW::WebServer", exception: $!.message, backtrace: $!.backtrace)
+
+    raise original_exception
   end # def run
 
   def build_event_from_request(body, headers)
@@ -69,8 +76,8 @@ def verify_signature(event,body)
     return is_valid
   end
 
-  def close
-    @server.stop
-  end # def close
+  def stop
+    @server && @server.stop
+  end # def stop
 
 end # class LogStash::Inputs::Github

logstash-input-github.gemspec

@@ -1,7 +1,7 @@
 Gem::Specification.new do |s|
 
   s.name            = 'logstash-input-github'
-  s.version         = '3.0.5'
+  s.version         = '3.0.6'
   s.licenses        = ['Apache License (2.0)']
   s.summary         = "Reads events from a GitHub webhook"
   s.description     = "This gem is a Logstash plugin required to be installed on top of the Logstash core pipeline using $LS_HOME/bin/logstash-plugin install gemname. This gem is not a stand-alone program"
@@ -24,7 +24,7 @@ Gem::Specification.new do |s|
 
   s.add_runtime_dependency 'addressable'
   s.add_runtime_dependency 'logstash-codec-plain'
-  s.add_runtime_dependency 'ftw', '~> 0.0.42'
+  s.add_runtime_dependency 'ftw', '~> 0.0.48'
 
   s.add_development_dependency 'logstash-devutils'
 end

spec/inputs/github_spec.rb

@@ -55,4 +55,51 @@
     end
 
   end
+
+  describe 'graceful shutdown' do
+    context 'when underlying webserver crashes' do
+
+      # Stubbing out our FTW::WebServer allows us to force it to raise an exception when we try to run it.
+      let(:mock_webserver_class) { double('FTW::WebServer::class').as_null_object }
+      let(:mock_webserver) { double('FTW::WebServer').as_null_object }
+      before(:each) do
+        stub_const('FTW::WebServer', mock_webserver_class)
+        allow(mock_webserver_class).to receive(:new).and_return(mock_webserver)
+        expect(mock_webserver).to receive(:run).and_raise('testing: intentional uncaught exception')
+      end
+
+      it 'makes an attempt to stop the webserver' do
+        expect(mock_webserver).to receive(:stop)
+
+        plugin.run([]) rescue nil
+      end
+
+      it 'propagates the original exception' do
+        expect do
+          plugin.run([])
+        end.to raise_exception('testing: intentional uncaught exception')
+      end
+
+      context 'and an attempt to stop the webserver also crashes' do
+        let(:mock_logger) { double('Logger').as_null_object }
+        before(:each) do
+          allow(plugin).to receive(:logger).and_return(mock_logger)
+          allow(mock_webserver).to receive(:stop).and_raise('yo dawg')
+        end
+
+        it 'logs helpfully' do
+          expect(mock_logger).to receive(:error).with("Error while stopping FTW::WebServer",
+                                                      exception: 'yo dawg', backtrace: instance_of(Array))
+
+          plugin.run([]) rescue nil
+        end
+
+        it 'propagates the original exception' do
+          expect do
+            plugin.run([])
+          end.to raise_exception('testing: intentional uncaught exception')
+        end
+      end
+    end
+  end
 end