ronantakizawa/codereview-bench huggingface dataset:
Repository: puma/puma
File: test/test_plugin_systemd.rb (Ruby)

Code before:
    assert_message "STATUS=Puma #{Puma::Const::VERSION}: worker: #{THREAD_LOG}"

    stop_server
    assert_message "STOPPING=1"
  end

  def test_systemd_cluster_notify
    skip_unless :fork
    cli_server "-w2 test/rackup/hello.ru", env: @env
    assert_message "READY=1"

    assert_message(
      "STATUS=Puma #{Puma::Const::VERSION}: cluster: 2/2, worker_status: [#{THREAD_LOG},#{THREAD_LOG}]")

    stop_server
    assert_message "STOPPING=1"
  end

  private

  def assert_restarts_with_systemd(signal, workers: 2)
    skip_unless(:fork) unless workers.zero?
    cli_server "-w#{workers} test/rackup/hello.ru", env: @env
    assert_message 'READY=1'

    Process.kill signal, @pid
    connect.write "GET / HTTP/1.1\r\n\r\n"
    assert_message 'RELOADING=1'
    assert_message 'READY=1'

    Process.kill signal, @pid
    connect.write "GET / HTTP/1.1\r\n\r\n"
    assert_message 'RELOADING=1'
    assert_message 'READY=1'

    stop_server
    assert_message 'STOPPING=1'
  end

  def assert_message(msg)
    @socket.wait_readable 1
    assert_equal msg, @socket.recvfrom(msg.bytesize)[0]
  end
end


Review comment (refactor):
So we do this `get_worker_pids` call to wait / ensure the server has booted far enough

https://github.com/puma/puma/blob/edd33bf2a9d108ffacb6e1ab06846fb88b8d5bea/test/helpers/integration.rb#L315-L325

Code after:
    assert_message "READY=1"

    assert_message "STATUS=Puma #{Puma::Const::VERSION}: worker: #{THREAD_LOG}"

    stop_server
    assert_message "STOPPING=1"
  end

  def test_systemd_cluster_notify
    skip_unless :fork
    cli_server "-w2 test/rackup/hello.ru", env: @env
    assert_message "READY=1"

    assert_message(
      "STATUS=Puma #{Puma::Const::VERSION}: cluster: 2/2, worker_status: [#{THREAD_LOG},#{THREAD_LOG}]")

    stop_server
    assert_message "STOPPING=1"
  end

  private

  def assert_restarts_with_systemd(signal, workers: 2)
    skip_unless(:fork) unless workers.zero?
    cli_server "-w#{workers} test/rackup/hello.ru", env: @env
    get_worker_pids(0, workers) if workers == 2
    assert_message 'READY=1'

    phase_ary = signal == :USR1 ? [1,2] : [0,0]

    Process.kill signal, @pid
    get_worker_pids(phase_ary[0], workers) if workers == 2
    assert_message 'RELOADING=1'
    assert_message 'READY=1'

    Process.kill signal, @pid
    get_worker_pids(phase_ary[1], workers) if workers == 2
    assert_message 'RELOADING=1'
    assert_message 'READY=1'

    stop_server
    assert_message 'STOPPING=1'
  end

  def assert_message(msg)
    @socket.wait_readable 1
    read = @socket.sysread(msg.bytesize)
    # below is kind of hacky, but seems to work correctly when slow CI systems
    # write partial status messages
    if read.start_with?('STATUS=') && !msg.start_with?('STATUS=')
      read << @socket.sysread(512) while @socket.wait_readable(1) && !read.end_with?(msg)
Repository: puma/puma
File: lib/rack/handler/puma.rb (Ruby)

Code before:

      launcher = ::Puma::Launcher.new(conf, :log_writer => log_writer, events: @events)

      yield launcher if block_given?
      begin
        launcher.run
      rescue Interrupt
        puts "* Gracefully stopping, waiting for requests to finish"
        launcher.stop
        puts "* Goodbye!"
      end
    end

    def valid_options
      {
        "Host=HOST"       => "Hostname to listen on (default: localhost)",
        "Port=PORT"       => "Port to listen on (default: 8080)",
        "Threads=MIN:MAX" => "min:max threads to use (default 0:16)",
        "Verbose"         => "Don't report each request (default: false)"
      }
    end

    def set_host_port_to_config(host, port, config)
      config.clear_binds! if host || port

      if host && (host[0,1] == '.' || host[0,1] == '/' || host[0,1] == '@')
        config.bind "unix://#{host}"
      elsif host && host =~ /^ssl:\/\//
        uri = URI.parse(host)
        uri.port ||= port || ::Puma::Configuration::DEFAULTS[:tcp_port]
        config.bind uri.to_s
      else

        if host
          port ||= ::Puma::Configuration::DEFAULTS[:tcp_port]
        end

        if port
          host ||= ::Puma::Configuration::DEFAULTS[:tcp_host]
          config.port port, host
        end
      end
    end
  end
end

# rackup was removed in Rack 3, it is now a separate gem
if Object.const_defined? :Rackup
  module Rackup
    module Handler
      module Puma

Review comment (suggestion):
Maybe change to:
```ruby
if host&.start_with? '.', '/', '@'
```

Code after:

      launcher = ::Puma::Launcher.new(conf, :log_writer => log_writer, events: @events)

      yield launcher if block_given?
      begin
        launcher.run
      rescue Interrupt
        puts "* Gracefully stopping, waiting for requests to finish"
        launcher.stop
        puts "* Goodbye!"
      end
    end

    def valid_options
      {
        "Host=HOST"       => "Hostname to listen on (default: localhost)",
        "Port=PORT"       => "Port to listen on (default: 8080)",
        "Threads=MIN:MAX" => "min:max threads to use (default 0:16)",
        "Verbose"         => "Don't report each request (default: false)"
      }
    end

    def set_host_port_to_config(host, port, config)
      config.clear_binds! if host || port

      if host&.start_with? '.', '/', '@'
        config.bind "unix://#{host}"
      elsif host && host =~ /^ssl:\/\//
        uri = URI.parse(host)
        uri.port ||= port || ::Puma::Configuration::DEFAULTS[:tcp_port]
        config.bind uri.to_s
      else

        if host
          port ||= ::Puma::Configuration::DEFAULTS[:tcp_port]
        end

        if port
          host ||= ::Puma::Configuration::DEFAULTS[:tcp_host]
          config.port port, host
        end
      end
    end
  end
end

# rackup was removed in Rack 3, it is now a separate gem
if Object.const_defined? :Rackup
  module Rackup
    module Handler
      module Puma
Repository: puma/puma
File: lib/puma/cluster/worker_handle.rb (Ruby)

Code before:

      # @version 5.0.0
      attr_writer :pid, :phase

      def booted?
        @stage == :booted
      end

      def uptime
        Time.now - started_at
      end

      def boot!
        @last_checkin = Time.now
        @stage = :booted
      end

      def term!
        @term = true
      end

      def term?
        @term
      end

      regex_string = Puma::Server::STAT_METHODS.map { |s| "\"#{s}\":(?<#{s}>\\d*)"  }.join(', ')
      STATUS_PATTERN = /{ #{regex_string} }/

      private_constant :STATUS_PATTERN

      def ping!(status)
        @last_checkin = Time.now
        @last_status = status.match(STATUS_PATTERN).named_captures.map { |c_name, c| [c_name.to_sym, c.to_i] }.to_h
      end

      # @see Puma::Cluster#check_workers
      # @version 5.0.0
      def ping_timeout
        @last_checkin +
          (booted? ?
            @options[:worker_timeout] :
            @options[:worker_boot_timeout]
          )
      end

      def term
        begin
          if @first_term_sent && (Time.now - @first_term_sent) > @options[:worker_shutdown_timeout]
            @signal = "KILL"
          else
            @term ||= true

Review comment (performance):
I like this code, it's much cleaner and it's not on the hot path. 

However, I think this will also have to be reverted if you change the above method of stats collection unless we want to normalize the order of the values (such as alphabetical). I'm assuming we have tests that exercise this status pattern.


Code after:

      # @version 5.0.0
      attr_writer :pid, :phase

      def booted?
        @stage == :booted
      end

      def uptime
        Time.now - started_at
      end

      def boot!
        @last_checkin = Time.now
        @stage = :booted
      end

      def term!
        @term = true
      end

      def term?
        @term
      end

      STATUS_PATTERN = /{ "backlog":(?<backlog>\d*), "running":(?<running>\d*), "pool_capacity":(?<pool_capacity>\d*), "max_threads":(?<max_threads>\d*), "requests_count":(?<requests_count>\d*), "busy_threads":(?<busy_threads>\d*) }/
      private_constant :STATUS_PATTERN

      def ping!(status)
        @last_checkin = Time.now
        @last_status = status.match(STATUS_PATTERN).named_captures.map { |c_name, c| [c_name.to_sym, c.to_i] }.to_h
      end

      # @see Puma::Cluster#check_workers
      # @version 5.0.0
      def ping_timeout
        @last_checkin +
          (booted? ?
            @options[:worker_timeout] :
            @options[:worker_boot_timeout]
          )
      end

      def term
        begin
          if @first_term_sent && (Time.now - @first_term_sent) > @options[:worker_shutdown_timeout]
            @signal = "KILL"
          else
            @term ||= true
            @first_term_sent ||= Time.now
          end
Repository: puma/puma
File: test/test_plugin_systemd.rb (Ruby)

Code before:
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/integration"

class TestPluginSystemd < TestIntegration
  parallelize_me! if ::Puma::IS_MRI

  THREAD_LOG = TRUFFLE ? "{ 0/16 threads, 16 available, 0 backlog }" :
    "{ 0/5 threads, 5 available, 0 backlog }"

  def setup
    skip_unless :linux
    skip_unless :unix
    skip_unless_signal_exist? :TERM
    skip_if :jruby

    super

    @sockaddr = tmp_path '.systemd'
    @socket = Socket.new(:UNIX, :DGRAM, 0)
    socket_ai = Addrinfo.unix(@sockaddr)
    @socket.bind Addrinfo.unix(@sockaddr)
    @env = {"NOTIFY_SOCKET" => @sockaddr }
    @message = +''
  end

  def teardown
    return if skipped?
    @socket&.close
    @socket = nil
    super
  end

  def test_systemd_notify_usr1_phased_restart_cluster
    skip_unless :fork
    assert_restarts_with_systemd :USR1
  end

  def test_systemd_notify_usr2_hot_restart_cluster
    skip_unless :fork
    assert_restarts_with_systemd :USR2
  end

  def test_systemd_notify_usr2_hot_restart_single
    assert_restarts_with_systemd :USR2, workers: 0
  end

  def test_systemd_watchdog

Review comment (suggestion):
```suggestion
    @env = { "NOTIFY_SOCKET" => @sockaddr }
```

Code after:
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/integration"

class TestPluginSystemd < TestIntegration
  parallelize_me! if ::Puma::IS_MRI

  THREAD_LOG = TRUFFLE ? "{ 0/16 threads, 16 available, 0 backlog }" :
    "{ 0/5 threads, 5 available, 0 backlog }"

  def setup
    skip_unless :linux
    skip_unless :unix
    skip_unless_signal_exist? :TERM
    skip_if :jruby

    super

    @sockaddr = tmp_path '.systemd'
    @socket = Socket.new(:UNIX, :DGRAM, 0)
    socket_ai = Addrinfo.unix(@sockaddr)
    @socket.bind Addrinfo.unix(@sockaddr)
    @env = { "NOTIFY_SOCKET" => @sockaddr }
    @message = +''
  end

  def teardown
    return if skipped?
    @socket&.close
    @socket = nil
    super
  end

  def test_systemd_notify_usr1_phased_restart_cluster
    skip_unless :fork
    assert_restarts_with_systemd :USR1
  end

  def test_systemd_notify_usr2_hot_restart_cluster
    skip_unless :fork
    assert_restarts_with_systemd :USR2
  end

  def test_systemd_notify_usr2_hot_restart_single
    assert_restarts_with_systemd :USR2, workers: 0
  end

  def test_systemd_watchdog
Repository: puma/puma
File: lib/puma/server.rb (Ruby)

Code before:
      stats = @thread_pool&.stats || {}
      stats[:max_threads]    = @max_threads
      stats[:requests_count] = @requests_count
      stats
    end

    # below are 'delegations' to binder
    # remove in Puma 7?


    def add_tcp_listener(host, port, optimize_for_latency = true, backlog = 1024)
      @binder.add_tcp_listener host, port, optimize_for_latency, backlog
    end

    def add_ssl_listener(host, port, ctx, optimize_for_latency = true,
                         backlog = 1024)
      @binder.add_ssl_listener host, port, ctx, optimize_for_latency, backlog
    end

    def add_unix_listener(path, umask = nil, mode = nil, backlog = 1024)
      @binder.add_unix_listener path, umask, mode, backlog
    end

    def update_thread_pool_min_max(min: @thread_pool.min, max: @thread_pool.max)
      @thread_pool.min, @thread_pool.max = min, max
      @min_threads, @max_threads = min, max
    end

    # @!attribute [r] connected_ports
    def connected_ports
      @binder.connected_ports
    end
  end
end


Review comment (suggestion):
```suggestion
      if min <= max and min > 0
        @thread_pool.min, @thread_pool.max = min, max
        @min_threads, @max_threads = min, max
      end
```

Code after:
    end
    private :notify_safely

    # Stops the acceptor thread and then causes the worker threads to finish
    # off the request queue before finally exiting.

    def stop(sync=false)
      notify_safely(STOP_COMMAND)
      @thread.join if @thread && sync
    end

    def halt(sync=false)
      notify_safely(HALT_COMMAND)
      @thread.join if @thread && sync
    end

    def begin_restart(sync=false)
      notify_safely(RESTART_COMMAND)
      @thread.join if @thread && sync
    end

    def shutting_down?
      @status == :stop || @status == :restart
    end

    # List of methods invoked by #stats.
    # @version 5.0.0
    STAT_METHODS = [
      :backlog,
      :running,
      :pool_capacity,
      :busy_threads,
      :backlog_max,
      :max_threads,
      :requests_count,
      :reactor_max,
    ].freeze

    # Returns a hash of stats about the running server for reporting purposes.
    # @version 5.0.0
    # @!attribute [r] stats
    # @return [Hash] hash containing stat info from `Server` and `ThreadPool`
    def stats
      stats = @thread_pool&.stats || {}
      stats[:max_threads]    = @max_threads
      stats[:requests_count] = @requests_count
      stats[:reactor_max] = @reactor.reactor_max if @reactor
      reset_max
      stats
    end
Repository: puma/puma
File: lib/puma/thread_pool.rb (Ruby)

Code before:
          backlog_max: temp
        }
      end
    end

    def reset_max
      with_mutex { @backlog_max = 0 }
    end

    # How many objects have yet to be processed by the pool?
    #
    def backlog
      with_mutex { @todo.size }
    end

    # The maximum size of the backlog
    #
    def backlog_max
      with_mutex { @backlog_max }
    end

    # @!attribute [r] pool_capacity
    def pool_capacity
      capacity = waiting + (@max - spawned)

      capacity < 0 ? 0 : capacity
    end

    # @!attribute [r] busy_threads
    # @version 5.0.0
    def busy_threads
      with_mutex { @spawned - @waiting + @todo.size }
    end

    # :nodoc:
    #
    # Must be called with @mutex held!
    #
    def spawn_thread
      @spawned += 1

      trigger_before_thread_start_hooks
      th = Thread.new(@spawned) do |spawned|
        Puma.set_thread_name '%s tp %03i' % [@name, spawned]
        todo  = @todo
        block = @block
        mutex = @mutex
        not_empty = @not_empty
        not_full = @not_full

        while true

Review comment (suggestion):
Optional suggestion, I think this is a little more clear:

```suggestion
      capacity.clamp(0, Float::INFINITY)
```

Code after:
      with_mutex do
        temp = @backlog_max
        @backlog_max = 0
        { backlog: @todo.size,
          running: @spawned,
          pool_capacity: pool_capacity,
          busy_threads: @spawned - @waiting + @todo.size,
          backlog_max: temp
        }
      end
    end

    def reset_max
      with_mutex { @backlog_max = 0 }
    end

    # How many objects have yet to be processed by the pool?
    #
    def backlog
      with_mutex { @todo.size }
    end

    # The maximum size of the backlog
    #
    def backlog_max
      with_mutex { @backlog_max }
    end

    # @!attribute [r] pool_capacity
    def pool_capacity
      (waiting + (@max - spawned)).clamp(0, Float::INFINITY)
    end

    # @!attribute [r] busy_threads
    # @version 5.0.0
    def busy_threads
      with_mutex { @spawned - @waiting + @todo.size }
    end

    # :nodoc:
    #
    # Must be called with @mutex held!
    #
    def spawn_thread
      @spawned += 1

      trigger_before_thread_start_hooks
      th = Thread.new(@spawned) do |spawned|
        Puma.set_thread_name '%s tp %03i' % [@name, spawned]
        # Advertise server into the thread
        Thread.current.puma_server = @server
Repository: puma/puma
File: lib/puma/server_plugin_control.rb (Ruby)

Code before:
  #
  # This class acts as a facade between plugins and the Puma server,
  # allowing plugins to safely modify server configuration and thread pool
  # settings without direct access to the server's internal state.
  #
  class ServerPluginControl
    def initialize(server)
      @server = server
      @thread_pool = server&.thread_pool
    end

    # Returns the maximum number of threads in the thread pool.
    def max_threads
      @server.max_threads
    end

    # Returns the minimum number of threads in the thread pool.
    def min_threads
      @server.min_threads
    end

    # Updates the minimum and maximum number of threads in the thread pool.
    #
    # @see Puma::Server#update_thread_pool_min_max
    #
    def update_thread_pool_min_max(min: @thread_pool.min, max: @thread_pool.max)
      @server.update_thread_pool_min_max(min: min, max: max)
    end
  end
end


Review comment (performance):
ServerPluginControl is instantiated from ThreadPool#spawn_thread before Server#thread_pool is assigned (ThreadPool.new returns before @thread_pool is set). With @thread_pool cached as nil, default args here call @thread_pool.min/@thread_pool.max and raise NoMethodError in before_thread_start. Consider resolving the pool lazily (e.g., @server.thread_pool) when computing defaults.

Code after:
  #
  # This class acts as a facade between plugins and the Puma server,
  # allowing plugins to safely modify server configuration and thread pool
  # settings without direct access to the server's internal state.
  #
  class ServerPluginControl
    def initialize(server)
      @server = server
    end

    # Returns the maximum number of threads in the thread pool.
    def max_threads
      @server.max_threads
    end

    # Returns the minimum number of threads in the thread pool.
    def min_threads
      @server.min_threads
    end

    # Updates the minimum and maximum number of threads in the thread pool.
    #
    # @see Puma::Server#update_thread_pool_min_max
    #
    def update_thread_pool_min_max(min: max_threads, max: min_threads)
      @server.update_thread_pool_min_max(min: min, max: max)
    end
  end
end
Repository: puma/puma
File: lib/puma/server.rb (Ruby)

Code before:
    #   Defaults to the current minimum if not specified. Must be greater than 0
    #   and less than or equal to max.
    # @param max [Integer] The maximum number of threads allowed in the pool.
    #   Defaults to the current maximum if not specified. Must be greater than or
    #   equal to min.
    #
    # @return [void]
    #
    # @note If validation fails, a warning message is logged and no changes are made.
    #
    # @example Update both min and max threads
    #   server.update_thread_pool_min_max(min: 2, max: 8)
    #
    # @example Update only the minimum threads
    #   server.update_thread_pool_min_max(min: 4)
    #
    # @example Update only the maximum threads
    #   server.update_thread_pool_min_max(max: 16)
    #
    def update_thread_pool_min_max(min: @thread_pool.min, max: @thread_pool.max)
      if min > max
        @log_writer.log "`min' value cannot be greater than `max' value."
        return
      end

      if min <= 0
        @log_writer.log "`min' value cannot be less than 0"
        return
      end

      @thread_pool&.with_mutex do
        @thread_pool.min, @thread_pool.max = min, max
        @min_threads, @max_threads = min, max
      end
    end

    # @!attribute [r] connected_ports
    def connected_ports
      @binder.connected_ports
    end
  end
end


Review comment (suggestion):
Puma's default min_threads is 0. This guard rejects min <= 0, so update_thread_pool_min_max(max: X) on a default-config server logs a warning and ignores the update. Allowing min == 0 (check min < 0) keeps it consistent with existing config semantics.

Code after:
    #   and less than or equal to max.
    # @param max [Integer] The maximum number of threads allowed in the pool.
    #   Defaults to the current maximum if not specified. Must be greater than or
    #   equal to min.
    #
    # @return [void]
    #
    # @note If validation fails, a warning message is logged and no changes are made.
    #
    # @example Update both min and max threads
    #   server.update_thread_pool_min_max(min: 2, max: 8)
    #
    # @example Update only the minimum threads
    #   server.update_thread_pool_min_max(min: 4)
    #
    # @example Update only the maximum threads
    #   server.update_thread_pool_min_max(max: 16)
    #
    def update_thread_pool_min_max(min: @min_threads, max: @max_threads)
      if min > max
        @log_writer.log "`min' value cannot be greater than `max' value."
        return
      end

      if min < 0
        @log_writer.log "`min' value cannot be less than 0"
        return
      end

      @thread_pool&.with_mutex do
        @thread_pool.min, @thread_pool.max = min, max
        @min_threads, @max_threads = min, max
      end
    end

    # @!attribute [r] connected_ports
    def connected_ports
      @binder.connected_ports
    end
  end
end