From 406234367544a4207141230683dddaccd98fb21a Mon Sep 17 00:00:00 2001 From: Azul Date: Fri, 21 Jul 2017 08:19:20 +0200 Subject: fix: filedescriptor leak from http_adapters Now we reuse a single adapter for all requests triggered by an incoming request. Then we .terminate the adapter. Includes a regression test. --- .gitlab-ci.yml | 2 + lib/nickserver/dispatcher.rb | 18 +++---- lib/nickserver/reel_server.rb | 29 +++++++++-- lib/nickserver/request_handlers/base.rb | 9 ++-- .../request_handlers/fingerprint_handler.rb | 2 +- .../request_handlers/hkp_email_handler.rb | 2 +- .../request_handlers/leap_email_handler.rb | 2 +- .../request_handlers/local_email_handler.rb | 2 +- lib/nickserver/source.rb | 6 +-- test/functional/sample_test.rb | 58 ++++++++++++++++++++++ test/remote/hkp_source_test.rb | 11 +++- test/remote/nicknym_source_test.rb | 4 +- test/support/functional_test.rb | 41 +++++++++++++++ test/support/http_adapter_helper.rb | 19 +++++++ test/support/http_stub_helper.rb | 4 +- 15 files changed, 175 insertions(+), 34 deletions(-) create mode 100644 test/functional/sample_test.rb create mode 100644 test/support/functional_test.rb create mode 100644 test/support/http_adapter_helper.rb 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 -- cgit v1.2.3