Merge branch 'fix/fd-leak' into 'master'

fix: filedescriptor leak from http_adapters

See merge request !13

15 files changed, 175 insertions, 34 deletions

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 28975f9..806f2c1 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -22,6 +22,8 @@ before_script:
   - gem install bundler  --no-ri --no-rdoc
   - bundle install -j $(nproc) --path vendor
   - bundle exec ruby /builds/leap/nickserver/bin/nickserver version
+  - apt update -y
+  - apt install -y lsof
 
 test:
   script:
diff --git a/lib/nickserver/dispatcher.rb b/lib/nickserver/dispatcher.rb
index 71e71cf..dfd53e5 100644
--- a/lib/nickserver/dispatcher.rb
+++ b/lib/nickserver/dispatcher.rb
@@ -24,21 +24,23 @@ require 'nickserver/request_handlers/fingerprint_handler'
 module Nickserver
   class Dispatcher
 
-
-    def initialize(responder)
+    def initialize(responder, adapter = nil)
       @responder = responder
+      @adapter = adapter
     end
 
     def respond_to(params, headers)
       request = Nickserver::Request.new params, headers
       response = handle request
-      send_response response
+      responder.respond response.status, response.content
     end
 
     protected
 
+    attr_reader :responder, :adapter
+
     def handle(request)
-      handler_chain.handle request
+      handler_chain.handle request, adapter
     end
 
     def handler_chain
@@ -51,7 +53,7 @@ module Nickserver
         RequestHandlers::LeapEmailHandler,
         RequestHandlers::HkpEmailHandler,
         RequestHandlers::FingerprintHandler,
-        Proc.new {|_req| proxy_error_response },
+        Proc.new { proxy_error_response },
         Proc.new { Nickserver::Response.new(404, "404 Not Found\n") }
       chain.continue_on HTTP::ConnectionError
       return chain
@@ -65,11 +67,5 @@ module Nickserver
       end
     end
 
-    def send_response(response)
-      responder.respond response.status, response.content
-    end
-
-    attr_reader :responder
-
   end
 end
diff --git a/lib/nickserver/reel_server.rb b/lib/nickserver/reel_server.rb
index c378aca..9626466 100644
--- a/lib/nickserver/reel_server.rb
+++ b/lib/nickserver/reel_server.rb
@@ -10,6 +10,8 @@ require 'nickserver/logging_responder'
 module Nickserver
   class ReelServer < Reel::Server::HTTP
 
+    DEFAULT_ADAPTER_CLASS = Nickserver::Adapters::CelluloidHttp
+
     def self.start(options = {})
       new(options[:host], options[:port])
     end
@@ -35,20 +37,37 @@ module Nickserver
     protected
 
     def handle_request(request)
+      logging_request(request) do
+        with_http_adapter do |adapter|
+          handler = handler_for(request, adapter)
+          handler.respond_to params(request), request.headers
+        end
+      end
+    rescue StandardError => e
+      request.respond 500, "{}"
+    end
+
+    def logging_request(request)
       logger.info "#{request.method} #{request.uri}"
       logger.debug "  #{params(request)}"
-      handler = handler_for(request)
-      handler.respond_to params(request), request.headers
+      yield
     rescue StandardError => e
       logger.error e
       logger.error e.backtrace.join "\n  "
-      request.respond 500, "{}"
+      raise
+    end
+
+    def with_http_adapter
+      adapter = DEFAULT_ADAPTER_CLASS.new
+      yield adapter
+    ensure
+      adapter.terminate if adapter.respond_to? :terminate
     end
 
-    def handler_for(request)
+    def handler_for(request, adapter)
       # with reel the request is the responder
       responder = LoggingResponder.new(request, logger)
-      Dispatcher.new(responder)
+      Dispatcher.new(responder, adapter)
     end
 
     def params(request)
diff --git a/lib/nickserver/request_handlers/base.rb b/lib/nickserver/request_handlers/base.rb
index e5d8992..495a6da 100644
--- a/lib/nickserver/request_handlers/base.rb
+++ b/lib/nickserver/request_handlers/base.rb
@@ -2,16 +2,17 @@ module Nickserver
   module RequestHandlers
     class Base
 
-      def self.call(request)
-        new(request).handle
+      def self.call(request, adapter = nil)
+        new(request, adapter).handle
       end
 
-      def initialize(request)
+      def initialize(request, adapter)
         @request = request
+        @adapter = adapter
       end
 
       protected
-      attr_reader :request
+      attr_reader :request, :adapter
     end
   end
 end
diff --git a/lib/nickserver/request_handlers/fingerprint_handler.rb b/lib/nickserver/request_handlers/fingerprint_handler.rb
index 5b2dc7d..ac3c3c8 100644
--- a/lib/nickserver/request_handlers/fingerprint_handler.rb
+++ b/lib/nickserver/request_handlers/fingerprint_handler.rb
@@ -22,7 +22,7 @@ module Nickserver
       end
 
       def source
-        Nickserver::Hkp::Source.new
+        Nickserver::Hkp::Source.new adapter
       end
 
     end
diff --git a/lib/nickserver/request_handlers/hkp_email_handler.rb b/lib/nickserver/request_handlers/hkp_email_handler.rb
index 2f73773..393ef87 100644
--- a/lib/nickserver/request_handlers/hkp_email_handler.rb
+++ b/lib/nickserver/request_handlers/hkp_email_handler.rb
@@ -16,7 +16,7 @@ module Nickserver
       end
 
       def source
-        Nickserver::Hkp::Source.new
+        Nickserver::Hkp::Source.new adapter
       end
 
     end
diff --git a/lib/nickserver/request_handlers/leap_email_handler.rb b/lib/nickserver/request_handlers/leap_email_handler.rb
index bdebc23..bc3ddef 100644
--- a/lib/nickserver/request_handlers/leap_email_handler.rb
+++ b/lib/nickserver/request_handlers/leap_email_handler.rb
@@ -13,7 +13,7 @@ module Nickserver
       protected
 
       def source
-        @source ||= Nicknym::Source.new
+        @source ||= Nicknym::Source.new adapter
       end
 
       def remote_email?
diff --git a/lib/nickserver/request_handlers/local_email_handler.rb b/lib/nickserver/request_handlers/local_email_handler.rb
index 9165ef2..08147a0 100644
--- a/lib/nickserver/request_handlers/local_email_handler.rb
+++ b/lib/nickserver/request_handlers/local_email_handler.rb
@@ -21,7 +21,7 @@ module Nickserver
       end
 
       def source
-        Nickserver::CouchDB::Source.new
+        Nickserver::CouchDB::Source.new adapter
       end
 
     end
diff --git a/lib/nickserver/source.rb b/lib/nickserver/source.rb
index dc0669a..934407a 100644
--- a/lib/nickserver/source.rb
+++ b/lib/nickserver/source.rb
@@ -1,11 +1,7 @@
-require 'nickserver/adapters/celluloid_http'
-
 module Nickserver
   class Source
 
-    DEFAULT_ADAPTER_CLASS = Nickserver::Adapters::CelluloidHttp
-
-    def initialize(adapter = DEFAULT_ADAPTER_CLASS.new)
+    def initialize(adapter = nil)
       @adapter = adapter
     end
 
diff --git a/test/functional/sample_test.rb b/test/functional/sample_test.rb
new file mode 100644
index 0000000..5886349
--- /dev/null
+++ b/test/functional/sample_test.rb
@@ -0,0 +1,58 @@
+require 'support/functional_test'
+
+class SampleTest < FunctionalTest
+  # don't parallize me... Hard to get that right with nickserver start & stop
+
+  def run(*args)
+    nickserver :start
+    super
+  ensure
+    nickserver :stop
+  end
+
+  def test_running
+    assert_running
+  end
+
+  # def test_invalid
+  #   assert_lookup_status 400, 'invalid'
+  # end
+
+  def test_nicknym
+    assert_lookup_status 200, 'test@mail.bitmask.net'
+  end
+
+  # Regression Tests
+
+  def test_no_file_descriptors_leak
+    lookup 'test@mail.bitmask.net'
+    before = open_files_count
+    lookup 'test@mail.bitmask.net'
+    assert_equal before, open_files_count, 'Filedescriptors leaked'
+  end
+
+  protected
+
+  def assert_lookup_status(status, address)
+    assert_equal status, lookup(address).to_i
+  end
+
+  def lookup(address)
+    run_command %Q(curl localhost:6425 #{curl_opts} -d "address=#{address}")
+  end
+
+  def curl_opts
+    '--silent -w "%{http_code}" -o /dev/null'
+  end
+
+  def open_files_count
+    `lsof | grep " #{nickserver_pid} " | wc -l`.to_i
+  end
+
+  def run_command(command)
+    `#{command} 2>&1`.tap do |out|
+      assert ($?.exitstatus == 0),
+        "failed to run '#{command}':\n #{out}"
+    end
+  end
+end
diff --git a/test/remote/hkp_source_test.rb b/test/remote/hkp_source_test.rb
index 103b8ad..ff61513 100644
--- a/test/remote/hkp_source_test.rb
+++ b/test/remote/hkp_source_test.rb
@@ -1,8 +1,10 @@
 require 'test_helper'
 require 'support/celluloid_test'
+require 'support/http_adapter_helper'
 require 'nickserver/hkp/source'
 
 class RemoteHkpSourceTest < CelluloidTest
+  include HttpAdapterHelper
 
   def test_key_info
     uid = 'elijah@riseup.net'
@@ -31,12 +33,17 @@ class RemoteHkpSourceTest < CelluloidTest
 
   protected
 
-  def assert_key_info_for_uid(uid, &block)
-    Nickserver::Hkp::Source.new.search uid do |status, keys|
+  def assert_key_info_for_uid(uid)
+    source.search uid do |status, keys|
       assert_equal 200, status
       yield keys
     end
   rescue HTTP::ConnectionError => e
     skip "could not talk to hkp server: #{e}"
   end
+
+  def source
+    Nickserver::Hkp::Source.new adapter
+  end
+
 end
diff --git a/test/remote/nicknym_source_test.rb b/test/remote/nicknym_source_test.rb
index 7840e10..4ca3033 100644
--- a/test/remote/nicknym_source_test.rb
+++ b/test/remote/nicknym_source_test.rb
@@ -1,5 +1,6 @@
 require 'test_helper'
 require 'support/celluloid_test'
+require 'support/http_adapter_helper'
 require 'nickserver/nicknym/source'
 require 'nickserver/email_address'
 
@@ -7,6 +8,7 @@ require 'nickserver/email_address'
 # Please note the Readme.md file in this directory
 #
 class RemoteNicknymSourceTest < CelluloidTest
+  include HttpAdapterHelper
 
   def test_availablility_check
     source.available_for? 'mail.bitmask.net'
@@ -42,7 +44,7 @@ class RemoteNicknymSourceTest < CelluloidTest
   end
 
   def source
-    @source ||= Nickserver::Nicknym::Source.new
+    @source ||= Nickserver::Nicknym::Source.new adapter
   end
 
   def email_with_key
diff --git a/test/support/functional_test.rb b/test/support/functional_test.rb
new file mode 100644
index 0000000..4ebc40a
--- /dev/null
+++ b/test/support/functional_test.rb
@@ -0,0 +1,41 @@
+require 'minitest/autorun'
+require 'minitest/pride'
+
+class FunctionalTest < Minitest::Test
+
+  protected
+
+  def nickserver_pid
+    status = nickserver "status"
+    /process id (\d*)\./.match(status)[1]
+  end
+
+  def assert_running
+    status = nickserver "status"
+    assert_includes status, "Nickserver running"
+  end
+
+  def assert_stopped
+    status = nickserver "status"
+    assert_includes status, "No nickserver processes are running."
+  end
+
+  def assert_command_runs(command)
+    out = nickserver command
+    assert ($?.exitstatus == 0),
+      "failed to run 'nickserver #{command}':\n #{out}"
+  end
+
+  def nickserver(command)
+    self.class.nickserver command
+  end
+
+  def self.nickserver(command)
+    `#{path_to_executable} #{command} 2>&1`
+  end
+
+  def self.path_to_executable
+    File.expand_path(File.dirname(__FILE__) + '/../../bin/nickserver')
+  end
+
+end
diff --git a/test/support/http_adapter_helper.rb b/test/support/http_adapter_helper.rb
new file mode 100644
index 0000000..6817e1e
--- /dev/null
+++ b/test/support/http_adapter_helper.rb
@@ -0,0 +1,19 @@
+require 'nickserver/adapters/celluloid_http'
+
+module HttpAdapterHelper
+
+  def setup
+    super
+    @adapter = Nickserver::Adapters::CelluloidHttp.new
+  end
+
+  def teardown
+    @adapter.terminate
+    super
+  end
+
+  protected
+
+  attr_reader :adapter
+
+end
diff --git a/test/support/http_stub_helper.rb b/test/support/http_stub_helper.rb
index dd3d1b2..c9f2bfa 100644
--- a/test/support/http_stub_helper.rb
+++ b/test/support/http_stub_helper.rb
@@ -1,9 +1,9 @@
-require 'nickserver/source'
+require 'nickserver/reel_server'
 
 module HttpStubHelper
 
   def stubbing_http
-    Nickserver::Source::DEFAULT_ADAPTER_CLASS.stub :new, adapter do
+    Nickserver::ReelServer::DEFAULT_ADAPTER_CLASS.stub :new, adapter do
       yield
     end
     adapter.verify