Server - Consolidate option handling, small refactors, doc changes (#…

…2389)

* Server - Consolidate option handling, small refactors, doc changes

Taken from PR #2373, added attr_writer's to maintain API, mark deprecated in v6.0.0.

* test_puma_server.rb - small timing fix for intermittent failures

History.md

@@ -6,6 +6,9 @@
 * Bugfixes
   * Your bugfix goes here <Most recent on the top, like GitHub> (#Github Number)
 
+* Refactor
+  * Consolidate option handling in Server, Server small refactors, doc chang (#2389)
+
 ## 5.0.2 / 2020-09-28
 
 * Bugfixes 

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

lib/puma/server.rb

@@ -34,15 +34,26 @@ 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
+
+    # @todo the following may be deprecated in the future
+    attr_reader :auto_trim_time, :early_hints, :first_data_timeout,
+      :leak_stack_on_error,
+      :persistent_timeout, :reaping_time
+
+    # @deprecated v6.0.0
+    attr_writer :auto_trim_time, :early_hints, :first_data_timeout,
+      :leak_stack_on_error, :min_threads, :max_threads,
+      :persistent_timeout, :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,31 +63,36 @@ 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
 
       @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
+      temp = !!(@options[:environment] =~ /\A(development|test)\z/)
+      @leak_stack_on_error = @options[:environment] ? temp : true
 
-      @options = options
-      @queue_requests = options[:queue_requests].nil? ? true : options[:queue_requests]
+      @binder = Binder.new(events)
 
       ENV['RACK_ENV'] ||= "development"
 
@@ -89,15 +105,16 @@ def initialize(app, events=Events.stdio, options={})
       @shutdown_mutex = Mutex.new
     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?
@@ -172,10 +189,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
@@ -188,6 +207,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
@@ -998,12 +1018,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
@@ -1019,6 +1033,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

test/test_puma_server.rb

@@ -987,7 +987,7 @@ def shutdown_requests(s1_complete: true, s1_response: nil, post: false, s2_respo
     assert_match(s1_response, s1.gets) if s1_response
 
     # Send s2 after shutdown begins
-    s2 << "\r\n" unless IO.select([s2], nil, nil, 0.1)
+    s2 << "\r\n" unless IO.select([s2], nil, nil, 0.2)
 
     assert IO.select([s2], nil, nil, 10), 'timeout waiting for response'
     s2_result = begin