From bbbdfb8f4c9ac1da5140d910cd0814bd7478791a Mon Sep 17 00:00:00 2001 From: MSP-Greg Date: Wed, 23 Sep 2020 09:28:23 -0500 Subject: [PATCH] Consolidate option handling in Server, Server small refactors, doc changes (#2373) * Update runner.rb * Update server.rb * Update test_busy_worker.rb * Update test_out_of_band_server.rb * Update test_persistent.rb * Update test_puma_server.rb * test_pumactl.rb - Fix CI warning: assigned but unused variable * test_integration_pumactl.rb - Fix CI warning: assigned but unused variable * Update History.md --- History.md | 3 ++ lib/puma/runner.rb | 21 +++-------- lib/puma/server.rb | 61 ++++++++++++++++++-------------- test/test_busy_worker.rb | 4 +-- test/test_integration_pumactl.rb | 1 + test/test_out_of_band_server.rb | 4 +-- test/test_persistent.rb | 8 ++--- test/test_puma_server.rb | 13 +++---- test/test_pumactl.rb | 2 +- 9 files changed, 59 insertions(+), 58 deletions(-) diff --git a/History.md b/History.md index 948ab3d5fa..e9731d8289 100644 --- a/History.md +++ b/History.md @@ -6,6 +6,9 @@ * Bugfixes * Your bugfix goes here (#Github Number) +* Refactor + * Remove unneeded attr_accessor statements from Server (#2373) + ## 5.0.0 * Features diff --git a/lib/puma/runner.rb b/lib/puma/runner.rb index 1a674fec07..e3e8cfdfbf 100644 --- a/lib/puma/runner.rb +++ b/lib/puma/runner.rb @@ -54,9 +54,8 @@ def start_control app = Puma::App::Status.new @launcher, token - control = Puma::Server.new app, @launcher.events - control.min_threads = 0 - control.max_threads = 1 + control = Puma::Server.new app, @launcher.events, + { min_threads: 0, max_threads: 1 } control.binder.parse [str], self, 'Starting control server' @@ -69,6 +68,7 @@ def close_control_listeners @control.binder.close_listeners if @control end + # @!attribute [r] ruby_engine def ruby_engine if !defined?(RUBY_ENGINE) || RUBY_ENGINE == "ruby" "ruby #{RUBY_VERSION}-p#{RUBY_PATCHLEVEL}" @@ -137,27 +137,14 @@ def load_and_bind @launcher.binder.parse @options[:binds], self end + # @!attribute [r] app def app @app ||= @launcher.config.app end def start_server - min_t = @options[:min_threads] - max_t = @options[:max_threads] - server = Puma::Server.new app, @launcher.events, @options - server.min_threads = min_t - server.max_threads = max_t server.inherit_binder @launcher.binder - - if @options[:early_hints] - server.early_hints = true - end - - unless development? || test? - server.leak_stack_on_error = false - end - server end end diff --git a/lib/puma/server.rb b/lib/puma/server.rb index d14b7fdfa4..37a552f35a 100644 --- a/lib/puma/server.rb +++ b/lib/puma/server.rb @@ -34,15 +34,21 @@ class Server attr_reader :thread attr_reader :events - attr_reader :requests_count # @version 5.0.0 + attr_reader :min_threads, :max_threads # for #stats + attr_reader :requests_count # @version 5.0.0 + + # the following may be deprecated in the future + attr_reader :auto_trim_time + attr_reader :first_data_timeout + attr_reader :persistent_timeout + attr_reader :reaping_time + attr_accessor :app + attr_accessor :binder - attr_accessor :min_threads - attr_accessor :max_threads - attr_accessor :persistent_timeout - attr_accessor :auto_trim_time - attr_accessor :reaping_time - attr_accessor :first_data_timeout + def_delegators :@binder, :add_tcp_listener, :add_ssl_listener, :add_unix_listener, :connected_ports + + ThreadLocalKey = :puma_server # Create a server for the rack app +app+. # @@ -52,6 +58,10 @@ class Server # Server#run returns a thread that you can join on to wait for the server # to do its work. # + # @note Several instance variables exist so they are available for testing, + # and have default values set via +fetch+. Normally the values are set via + # `::Puma::Configuration.puma_default_options`. + # def initialize(app, events=Events.stdio, options={}) @app = app @events = events @@ -59,24 +69,24 @@ def initialize(app, events=Events.stdio, options={}) @check, @notify = nil @status = :stop - @min_threads = 0 - @max_threads = 16 @auto_trim_time = 30 @reaping_time = 1 @thread = nil @thread_pool = nil - @early_hints = nil - @persistent_timeout = options.fetch(:persistent_timeout, PERSISTENT_TIMEOUT) - @first_data_timeout = options.fetch(:first_data_timeout, FIRST_DATA_TIMEOUT) + @options = options - @binder = Binder.new(events) + @early_hints = options.fetch :early_hints, nil + @first_data_timeout = options.fetch :first_data_timeout, FIRST_DATA_TIMEOUT + @min_threads = options.fetch :min_threads, 0 + @max_threads = options.fetch :max_threads , (Puma.mri? ? 5 : 16) + @persistent_timeout = options.fetch :persistent_timeout, PERSISTENT_TIMEOUT + @queue_requests = options.fetch :queue_requests, true - @leak_stack_on_error = true + @leak_stack_on_error = !!(@options[:environment] =~ /\A(development|test)\z/) - @options = options - @queue_requests = options[:queue_requests].nil? ? true : options[:queue_requests] + @binder = Binder.new(events) ENV['RACK_ENV'] ||= "development" @@ -87,15 +97,16 @@ def initialize(app, events=Events.stdio, options={}) @requests_count = 0 end - attr_accessor :binder, :leak_stack_on_error, :early_hints - - def_delegators :@binder, :add_tcp_listener, :add_ssl_listener, :add_unix_listener, :connected_ports - def inherit_binder(bind) @binder = bind end class << self + # @!attribute [r] current + def current + Thread.current[ThreadLocalKey] + end + # :nodoc: # @version 5.0.0 def tcp_cork_supported? @@ -170,10 +181,12 @@ def closed_socket?(socket) end end + # @!attribute [r] backlog def backlog @thread_pool and @thread_pool.backlog end + # @!attribute [r] running def running @thread_pool and @thread_pool.spawned end @@ -186,6 +199,7 @@ def running # there are 5 threads sitting idle ready to take # a request. If one request comes in, then the # value would be 4 until it finishes processing. + # @!attribute [r] pool_capacity def pool_capacity @thread_pool and @thread_pool.pool_capacity end @@ -989,12 +1003,6 @@ def fast_write(io, str) end private :fast_write - ThreadLocalKey = :puma_server - - def self.current - Thread.current[ThreadLocalKey] - end - def shutting_down? @status == :stop || @status == :restart end @@ -1010,6 +1018,7 @@ def possible_header_injection?(header_value) # Returns a hash of stats about the running server for reporting purposes. # @version 5.0.0 + # @!attribute [r] stats def stats STAT_METHODS.map {|name| [name, send(name) || 0]}.to_h end diff --git a/test/test_busy_worker.rb b/test/test_busy_worker.rb index ed5b85d93f..df34ab8a70 100644 --- a/test/test_busy_worker.rb +++ b/test/test_busy_worker.rb @@ -54,8 +54,8 @@ def with_server(**options, &app) end @server = Puma::Server.new request_handler, Puma::Events.strings, **options - @server.min_threads = options[:min_threads] || 0 - @server.max_threads = options[:max_threads] || 10 + @server.instance_variable_set :@min_threads, options[:min_threads] || 0 + @server.instance_variable_set :@max_threads, options[:max_threads] || 10 @port = (@server.add_tcp_listener '127.0.0.1', 0).addr[1] @server.run end diff --git a/test/test_integration_pumactl.rb b/test/test_integration_pumactl.rb index c496685f5e..f661163cfa 100644 --- a/test/test_integration_pumactl.rb +++ b/test/test_integration_pumactl.rb @@ -108,6 +108,7 @@ def test_prune_bundler_with_multiple_workers cli_pumactl "stop", unix: true _, status = Process.wait2(@pid) + assert_equal 0, status @server = nil end diff --git a/test/test_out_of_band_server.rb b/test/test_out_of_band_server.rb index bd537e81c8..aa0b9abc51 100644 --- a/test/test_out_of_band_server.rb +++ b/test/test_out_of_band_server.rb @@ -60,8 +60,8 @@ def oob_server(**options) end @server = Puma::Server.new app, Puma::Events.strings, out_of_band: [oob], **options - @server.min_threads = options[:min_threads] || 1 - @server.max_threads = options[:max_threads] || 1 + @server.instance_variable_set :@min_threads, options[:min_threads] || 1 + @server.instance_variable_set :@max_threads, options[:max_threads] || 1 @port = (@server.add_tcp_listener '127.0.0.1', 0).addr[1] @server.run end diff --git a/test/test_persistent.rb b/test/test_persistent.rb index cf160b8b4c..4da7a82f73 100644 --- a/test/test_persistent.rb +++ b/test/test_persistent.rb @@ -25,7 +25,7 @@ def setup @server = Puma::Server.new @simple @port = (@server.add_tcp_listener HOST, 0).addr[1] - @server.max_threads = 1 + @server.instance_variable_set :@max_threads, 1 @server.run @client = TCPSocket.new HOST, @port @@ -152,7 +152,7 @@ def test_one_with_keep_alive_header end def test_persistent_timeout - @server.persistent_timeout = 1 + @server.instance_variable_set :@persistent_timeout, 1 @client << @valid_request sz = @body[0].size.to_s @@ -189,7 +189,7 @@ def test_allow_app_to_chunk_itself def test_two_requests_in_one_chunk - @server.persistent_timeout = 3 + @server.instance_variable_set :@persistent_timeout, 3 req = @valid_request.to_s req += "GET /second HTTP/1.1\r\nHost: test.com\r\nContent-Type: text/plain\r\n\r\n" @@ -206,7 +206,7 @@ def test_two_requests_in_one_chunk end def test_second_request_not_in_first_req_body - @server.persistent_timeout = 3 + @server.instance_variable_set :@persistent_timeout, 3 req = @valid_request.to_s req += "GET /second HTTP/1.1\r\nHost: test.com\r\nContent-Type: text/plain\r\n\r\n" diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb index 66830ce7a0..c1b2d25187 100644 --- a/test/test_puma_server.rb +++ b/test/test_puma_server.rb @@ -24,7 +24,7 @@ def teardown def server_run(app: @app, early_hints: false) @server.app = app @port = (@server.add_tcp_listener @host, 0).addr[1] - @server.early_hints = true if early_hints + @server.instance_variable_set(:@early_hints, true) if early_hints @server.run end @@ -194,7 +194,7 @@ def test_early_hints_works EOF ).split("\n").join("\r\n") + "\r\n\r\n" - assert_equal true, @server.early_hints + assert_equal true, @server.instance_variable_get(:@early_hints) assert_equal expected_data, data end @@ -234,7 +234,7 @@ def test_early_hints_is_off_by_default EOF ).split("\n").join("\r\n") + "\r\n\r\n" - assert_nil @server.early_hints + assert_nil @server.instance_variable_get :@early_hints assert_equal expected_data, data end @@ -247,7 +247,7 @@ def test_GET_with_no_body_has_sane_chunking end def test_doesnt_print_backtrace_in_production - @server.leak_stack_on_error = false + @server.instance_variable_set :@leak_stack_on_error, false server_run app: ->(env) { raise "don't leak me bro" } data = send_http_and_read "GET / HTTP/1.0\r\n\r\n" @@ -273,7 +273,8 @@ def test_force_shutdown_custom_error_message end def test_force_shutdown_error_default - @server = Puma::Server.new @app, @events, {:force_shutdown_after => 2} + @server = Puma::Server.new @app, @events, {force_shutdown_after: 2} + @server.instance_variable_set :@leak_stack_on_error, true server_run app: ->(env) do @server.stop @@ -370,7 +371,7 @@ def test_status_hook_fires_when_server_changes_states end def test_timeout_in_data_phase - @server.first_data_timeout = 2 + @server.instance_variable_set :@first_data_timeout, 2 server_run sock = send_http "POST / HTTP/1.1\r\nHost: test.com\r\nContent-Type: text/plain\r\nContent-Length: 5\r\n\r\n" diff --git a/test/test_pumactl.rb b/test/test_pumactl.rb index 485adb832b..63bf4b8f52 100644 --- a/test/test_pumactl.rb +++ b/test/test_pumactl.rb @@ -182,7 +182,7 @@ def assert_command_cli_output(options, expected_out) out, _ = capture_subprocess_io do begin cmd.run - rescue SystemExit => e + rescue SystemExit end end assert_match expected_out, out