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

Code before:

    def halt
      @server.halt
    end

    def stop_blocked
      log "- Gracefully stopping, waiting for requests to finish"
      @control.stop(true) if @control
      @server.stop(true) if @server
    end

    def run
      output_header "single"

      load_and_bind

      Plugins.fire_background

      @launcher.write_state

      start_control

      @server = server = start_server
      server_thread = server.run

      @launcher.config.run_hooks :on_booted, nil, @launcher.events

      log "Use Ctrl-C to stop"
      redirect_io

      @launcher.events.fire_on_booted!

      begin
        server_thread.join
      rescue Interrupt
        # Swallow it
      end
    end
  end
end


Review comment (suggestion):
Rather than add this line, should we somehow make  use the already-existing `on_booted` event, I wonder? It's maybe not great that we have this `run_hooks` system which has the exact same name of a trigger as an `Event`... feels like they could be combined.

Code after:

    def halt
      @server&.halt
    end

    def stop_blocked
      log "- Gracefully stopping, waiting for requests to finish"
      @control&.stop true
      @server&.stop true
    end

    def run
      output_header "single"

      load_and_bind

      Plugins.fire_background

      @launcher.write_state

      start_control

      @server = server = start_server
      server_thread = server.run

      log "Use Ctrl-C to stop"
      redirect_io

      @events.fire_on_booted!

      debug_loaded_extensions("Loaded Extensions:") if @log_writer.debug?

      begin
        server_thread.join
      rescue Interrupt
        # Swallow it
      end
    end
  end
end
Repository: puma/puma
File: test/test_config.rb (Ruby)

Code before:
  def test_run_hooks_on_restart_hook
    assert_run_hooks :on_restart
  end

  def test_run_hooks_before_worker_fork
    assert_run_hooks :before_worker_fork, configured_with: :on_worker_fork
  end

  def test_run_hooks_after_worker_fork
    assert_run_hooks :after_worker_fork
  end

  def test_run_hooks_before_worker_boot
    assert_run_hooks :before_worker_boot, configured_with: :on_worker_boot
  end

  def test_run_hooks_before_worker_shutdown
    assert_run_hooks :before_worker_shutdown, configured_with: :on_worker_shutdown
  end

  def test_run_hooks_before_fork
    assert_run_hooks :before_fork
  end

  def test_run_hooks_on_booted
    assert_run_hooks :on_booted
  end

  def test_run_hooks_and_exception
    conf = Puma::Configuration.new do |c|
      c.on_restart do |a|
        raise RuntimeError, 'Error from hook'
      end
    end
    conf.load
    events = Puma::Events.strings

    conf.run_hooks :on_restart, 'ARG', events
    expected = /WARNING hook on_restart failed with exception \(RuntimeError\) Error from hook/
    assert_match expected, events.stdout.string
  end

  def test_config_does_not_load_workers_by_default
    assert_equal 0, Puma::Configuration.new.options.default_options[:workers]
  end

  def test_final_options_returns_merged_options
    conf = Puma::Configuration.new({ min_threads: 1, max_threads: 2 }, { min_threads: 2 })

    assert_equal 1, conf.final_options[:min_threads]
    assert_equal 2, conf.final_options[:max_threads]

Review comment (bug):
This seems to be failing inside of `assert_run_hooks`. I think it is specifically this line:
https://github.com/puma/puma/blob/a6cdca85a15b2c277f64a5fa7572effb786fa9a1/test/test_config.rb#L379

Code after:
    ssl_binding = conf.options[:binds].first
    assert ssl_binding.include?("&ssl_cipher_filter=#{cipher_filter}")
  end

  def test_ssl_bind_with_verification_flags
    skip_if :jruby
    skip_unless :ssl

    conf = Puma::Configuration.new do |c|
      c.ssl_bind "0.0.0.0", "9292", {
        cert: "cert",
        key: "key",
        verification_flags: ["TRUSTED_FIRST", "NO_CHECK_TIME"]
      }
    end

    conf.load

    ssl_binding = conf.options[:binds].first
    assert ssl_binding.include?("&verification_flags=TRUSTED_FIRST,NO_CHECK_TIME")
  end

  def test_ssl_bind_with_ca
    skip_unless :ssl
    conf = Puma::Configuration.new do |c|
      c.ssl_bind "0.0.0.0", "9292", {
        cert: "/path/to/cert",
        ca: "/path/to/ca",
        key: "/path/to/key",
        verify_mode: :peer,
      }
    end

    conf.load

    ssl_binding = conf.options[:binds].first
    assert_includes ssl_binding, Puma::Util.escape("/path/to/ca")
    assert_includes ssl_binding, "verify_mode=peer"
  end

  def test_lowlevel_error_handler_DSL
    conf = Puma::Configuration.new do |c|
      c.load "test/config/app.rb"
    end
    conf.load

    app = conf.options[:lowlevel_error_handler]

    assert_equal [200, {}, ["error page"]], app.call({})
  end
Repository: puma/puma
File: lib/puma/runner.rb (Ruby)

Code before:
      end

      begin
        @app = @launcher.config.app
      rescue Exception => e
        log "! Unable to load application: #{e.class}: #{e.message}"
        raise e
      end

      @launcher.binder.parse @options[:binds], self
    end

    # @!attribute [r] app
    def app
      @app ||= @launcher.config.app
    end

    def start_server
      server = Puma::Server.new app, @launcher.events, @options
      server.inherit_binder @launcher.binder
      server
    end

    private

    def ensure_output_directory_exists(path)
      unless Dir.exist?(File.dirname(path))
        raise "Cannot redirect STDERR to #{path}"
      end
    end
  end
end


Review comment (suggestion):
Maybe something like:
```ruby
def ensure_output_directory_exists(path, io_name)
```
and pass `'STDOUT'` or `'STDERR'` to it, along with changing:
```ruby
raise "Cannot redirect #{io_name} to #{path}"

Code after:

      begin
        @app = @launcher.config.app
      rescue Exception => e
        log "! Unable to load application: #{e.class}: #{e.message}"
        raise e
      end

      @launcher.binder.parse @options[:binds], self
    end

    # @!attribute [r] app
    def app
      @app ||= @launcher.config.app
    end

    def start_server
      server = Puma::Server.new app, @launcher.events, @options
      server.inherit_binder @launcher.binder
      server
    end

    private
    def ensure_output_directory_exists(path, io_name)
      unless Dir.exist?(File.dirname(path))
        raise "Cannot redirect #{io_name} to #{path}"
      end
    end
  end
end
Repository: puma/puma
File: lib/puma/dsl.rb (Ruby)

Code before:
    # Store the pid of the server in the file at "path".
    #
    # @example
    #   pidfile '/u/apps/lolcat/tmp/pids/puma.pid'
    def pidfile(path)
      @options[:pidfile] = path.to_s
    end

    # Disable request logging, if this isn't used it'll be enabled by default.
    #
    # @example
    #   quiet
    def quiet(which=true)
      @options[:log_requests] = !which
    end

    # Enable request logging
    #
    def log_requests(which=true)
      @options[:log_requests] = which
    end

    # Pass in a custom logging class instance
    def logger(custom_logger)
      @options[:logger] = custom_logger
    end

    # Show debugging info
    #
    def debug
      @options[:debug] = true
    end

    # Load +path+ as a rackup file.
    #
    # The default is "config.ru".
    #
    # @example
    #   rackup '/u/apps/lolcat/config.ru'
    def rackup(path)
      @options[:rackup] ||= path.to_s
    end

    # Allows setting `env['rack.url_scheme']`.
    # Only necessary if X-Forwarded-Proto is not being set by your proxy
    # Normal values are 'http' or 'https'.
    def rack_url_scheme(scheme=nil)
      @options[:rack_url_scheme] = scheme
    end

    def early_hints(answer=true)

Review comment (suggestion):
```suggestion
    def custom_logger(custom_logger)
      @options[:custom_logger] = custom_logger
    end
```

Code after:
    # Store the pid of the server in the file at "path".
    #
    # @example
    #   pidfile '/u/apps/lolcat/tmp/pids/puma.pid'
    def pidfile(path)
      @options[:pidfile] = path.to_s
    end

    # Disable request logging, if this isn't used it'll be enabled by default.
    #
    # @example
    #   quiet
    def quiet(which=true)
      @options[:log_requests] = !which
    end

    # Enable request logging
    #
    def log_requests(which=true)
      @options[:log_requests] = which
    end

    # Pass in a custom logging class instance
    def custom_logger(custom_logger)
      @options[:custom_logger] = custom_logger
    end

    # Show debugging info
    #
    def debug
      @options[:debug] = true
    end

    # Load +path+ as a rackup file.
    #
    # The default is "config.ru".
    #
    # @example
    #   rackup '/u/apps/lolcat/config.ru'
    def rackup(path)
      @options[:rackup] ||= path.to_s
    end

    # Allows setting `env['rack.url_scheme']`.
    # Only necessary if X-Forwarded-Proto is not being set by your proxy
    # Normal values are 'http' or 'https'.
    def rack_url_scheme(scheme=nil)
      @options[:rack_url_scheme] = scheme
    end

    def early_hints(answer=true)
Repository: puma/puma
File: test/config/custom_logger.rb (Ruby)

Code before:
class CustomLogger
  def initialize(output=STDOUT)
    @output = output
  end

  def write(msg)
    @output.puts 'Custom logging: ' + msg
    @output.flush
  end
end

log_requests
logger CustomLogger.new(STDOUT)


Review comment (suggestion):
```suggestion
custom_logger CustomLogger.new(STDOUT)
```

Code after:
class CustomLogger
  def initialize(output=STDOUT)
    @output = output
  end

  def write(msg)
    @output.puts 'Custom logging: ' + msg
    @output.flush
  end
end

log_requests
custom_logger CustomLogger.new(STDOUT)
Repository: puma/puma
File: lib/puma/cluster.rb (Ruby)

Code before:
      return if diff < 1

      debug "Culling #{diff.inspect} workers"

      workers_to_cull =
        case @options[:worker_culling_strategy]
        when :default
          @workers[-diff,diff]
        when :oldest
          @workers.sort_by(&:started_at)[0,diff]
        end

      debug "Workers to cull: #{workers_to_cull.inspect}"

      workers_to_cull.each do |worker|
        log "- Worker #{worker.index} (PID: #{worker.pid}) terminating"
        worker.term
      end
    end

    # @!attribute [r] next_worker_index
    def next_worker_index
      occupied_positions = @workers.map(&:index)
      idx = 0
      idx += 1 until !occupied_positions.include?(idx)
      idx
    end

    def all_workers_booted?
      @workers.count { |w| !w.booted? } == 0
    end

    def check_workers
      return if @next_check >= Time.now

      @next_check = Time.now + @options[:worker_check_interval]

      timeout_workers
      wait_workers
      cull_workers
      spawn_workers

      if all_workers_booted?
        # If we're running at proper capacity, check to see if
        # we need to phase any workers out (which will restart
        # in the right phase).
        #
        w = @workers.find { |x| x.phase != @phase }

        if w
          log "- Stopping #{w.pid} for phased upgrade..."

Review comment (bug):
The reason I changed this method's implementation is because I ran into an edge case while writing tests for this PR: if `@options[:workers]` is decreased (e.g. because the process received a TTOU signal), the previous implementation could potentially return `nil`.

Code after:
      return if diff < 1

      debug "Culling #{diff.inspect} workers"

      workers_to_cull =
        case @options[:worker_culling_strategy]
        when :youngest
          @workers.sort_by(&:started_at)[-diff,diff]
        when :oldest
          @workers.sort_by(&:started_at)[0,diff]
        end

      debug "Workers to cull: #{workers_to_cull.inspect}"

      workers_to_cull.each do |worker|
        log "- Worker #{worker.index} (PID: #{worker.pid}) terminating"
        worker.term
      end
    end

    # @!attribute [r] next_worker_index
    def next_worker_index
      occupied_positions = @workers.map(&:index)
      idx = 0
      idx += 1 until !occupied_positions.include?(idx)
      idx
    end

    def all_workers_booted?
      @workers.count { |w| !w.booted? } == 0
    end

    def check_workers
      return if @next_check >= Time.now

      @next_check = Time.now + @options[:worker_check_interval]

      timeout_workers
      wait_workers
      cull_workers
      spawn_workers

      if all_workers_booted?
        # If we're running at proper capacity, check to see if
        # we need to phase any workers out (which will restart
        # in the right phase).
        #
        w = @workers.find { |x| x.phase != @phase }

        if w
          log "- Stopping #{w.pid} for phased upgrade..."
Repository: puma/puma
File: test/test_rack_server.rb (Ruby)

Code before:
    socket.puts "\r\n"

    headers = socket.readline("\r\n\r\n")
      .split("\r\n")
      .drop(1)
      .map { |line| line.split(/:\s?/) }
      .to_h

    content_length = headers["Content-Length"].to_i
    real_response_body = socket.read(content_length)

    assert_equal "Hello", real_response_body

    # When after_reply breaks the connection it will write the expected HTTP
    # response followed by a second HTTP response: HTTP/1.1 500
    #
    # This sleeps to give the server time to write the invalid/extra HTTP
    # response.
    #
    # * If we can read from the socket, we know that extra content has been
    #   written to the connection and assert that it's our erroneous 500
    #   response.
    # * If we would block trying to read from the socket, we can assume that
    #   the erroneous 500 response wasn't/won't be written.
    sleep 0.1
    assert_raises IO::EAGAINWaitReadable do
      content = socket.read_nonblock(12)
      refute_includes content, "500"
    end

    socket.close

    stop
  end

  def test_common_logger
    log = StringIO.new

    logger = Rack::CommonLogger.new(@simple, log)

    @server.app = logger

    @server.run

    hit(["#{@tcp}/test"])

    stop

    assert_match %r!GET /test HTTP/1\.1!, log.string
  end
end

Review comment (suggestion):
I'm not the biggest fan of this test, but using `Net::HTTP` was flaky and the problem is pretty low-level so this is what I ended up with. It is technically testable using `Net::HTTP` but you have to `instance_variable_get(:@socket)` on the connection and I wasn't a huge fan of diving into stdlib internals.

Happy to get feedback/suggestions on alternatives here if anyone has any.

Code after:
    socket.puts "\r\n"

    headers = socket.readline("\r\n\r\n")
      .split("\r\n")
      .drop(1)
      .map { |line| line.split(/:\s?/) }
      .to_h

    content_length = headers["Content-Length"].to_i
    real_response_body = socket.read(content_length)

    assert_equal "Hello", real_response_body

    # When after_reply breaks the connection it will write the expected HTTP
    # response followed by a second HTTP response: HTTP/1.1 500
    #
    # This sleeps to give the server time to write the invalid/extra HTTP
    # response.
    #
    # * If we can read from the socket, we know that extra content has been
    #   written to the connection and assert that it's our erroneous 500
    #   response.
    # * If we would block trying to read from the socket, we can assume that
    #   the erroneous 500 response wasn't/won't be written.
    sleep 0.1
    assert_raises IO::WaitReadable do
      content = socket.read_nonblock(12)
      refute_includes content, "500"
    end

    socket.close

    stop
  end

  def test_common_logger
    log = StringIO.new

    logger = Rack::CommonLogger.new(@simple, log)

    @server.app = logger

    @server.run

    hit(["#{@tcp}/test"])

    stop

    assert_match %r!GET /test HTTP/1\.1!, log.string
  end
end
Repository: puma/puma
File: test/test_cli.rb (Ruby)

Code before:

require "puma/cli"
require "json"
require "psych"

class TestCLI < Minitest::Test
  include SSLHelper if ::Puma::HAS_SSL
  include TmpPath

  def setup
    @environment = 'production'

    @tmp_path = tmp_path('puma-test')
    @tmp_path2 = "#{@tmp_path}2"

    File.unlink @tmp_path  if File.exist? @tmp_path
    File.unlink @tmp_path2 if File.exist? @tmp_path2

    @wait, @ready = IO.pipe

    @log_writer = Puma::LogWriter.strings

    @events = Puma::Events.new
    @events.on_booted { @ready << "!" }

    @puma_vers_re = "\\d+.\\d+.\\d+(\\.[a-z\\d]+)?"
  end

  def wait_booted
    @wait.sysread 1
  rescue Errno::EAGAIN
    sleep 0.001
    retry
  end

  def teardown
    File.unlink @tmp_path if File.exist? @tmp_path
    File.unlink @tmp_path2 if File.exist? @tmp_path2

    @wait.close
    @ready.close
  end

  def test_control_for_tcp
    cntl = UniquePort.call
    url = "tcp://127.0.0.1:#{cntl}/"

    cli = Puma::CLI.new ["-b", "tcp://127.0.0.1:0",
                         "--control-url", url,
                         "--control-token", "",
                         "test/rackup/hello.ru"], @log_writer, @events

Review comment (suggestion):
```suggestion
    @puma_version_pattern = "\\d+.\\d+.\\d+(\\.[a-z\\d]+)?"
```

Code after:

require "puma/cli"
require "json"
require "psych"

class TestCLI < Minitest::Test
  include SSLHelper if ::Puma::HAS_SSL
  include TmpPath

  def setup
    @environment = 'production'

    @tmp_path = tmp_path('puma-test')
    @tmp_path2 = "#{@tmp_path}2"

    File.unlink @tmp_path  if File.exist? @tmp_path
    File.unlink @tmp_path2 if File.exist? @tmp_path2

    @wait, @ready = IO.pipe

    @log_writer = Puma::LogWriter.strings

    @events = Puma::Events.new
    @events.on_booted { @ready << "!" }

    @puma_version_pattern = "\\d+.\\d+.\\d+(\\.[a-z\\d]+)?"
  end

  def wait_booted
    @wait.sysread 1
  rescue Errno::EAGAIN
    sleep 0.001
    retry
  end

  def teardown
    File.unlink @tmp_path if File.exist? @tmp_path
    File.unlink @tmp_path2 if File.exist? @tmp_path2

    @wait.close
    @ready.close
  end

  def test_control_for_tcp
    cntl = UniquePort.call
    url = "tcp://127.0.0.1:#{cntl}/"

    cli = Puma::CLI.new ["-b", "tcp://127.0.0.1:0",
                         "--control-url", url,
                         "--control-token", "",
                         "test/rackup/hello.ru"], @log_writer, @events
Repository: puma/puma
File: lib/puma/plugin/systemd.rb (Ruby)

Code before:
        launcher.log_writer.log "Pinging systemd watchdog every #{ping_f.round(1)} sec"
        loop do
          sleep ping_f
          SdNotify.watchdog
        end
      end
    end

    # start status loop
    instance = self
    sleep_time = 1.0
    in_background do
      launcher.log_writer.log "Sending status to systemd every #{sleep_time.round(1)} sec"

      loop do
        sleep sleep_time
        # TODO: error handling?
        SdNotify.status(instance.status)
      end
    end
  end

  def status
    if clustered?
      messages = Array.new(workers) do |i|
        common_message(stats[:worker_status][i][:last_status])
      end.join(',')

      "Puma #{Puma::Const::VERSION}: cluster: #{booted_workers}/#{workers}, worker_status: [#{messages}]"
    else
      "Puma #{Puma::Const::VERSION}: worker: #{common_message(stats)}"
    end
  end

  private

  def watchdog_sleep_time
    usec = Integer(ENV["WATCHDOG_USEC"])

    sec_f = usec / 1_000_000.0
    # "It is recommended that a daemon sends a keep-alive notification message
    # to the service manager every half of the time returned here."
    sec_f / 2
  end

  def stats
    Puma.stats_hash
  end

  def clustered?
    stats.has_key?(:workers)

Review comment (suggestion):
I didn't look at the actual source, but would it be possible to iterate over `stats[:worker_status]` instead? Untested, but something like:
```suggestion
      messages = stats[:worker_status].values do |worker|
        common_message(worker[:last_status])
```

Code after:
      end
    end

    # start status loop
    instance = self
    sleep_time = 1.0
    in_background do
      launcher.log_writer.log "Sending status to systemd every #{sleep_time.round(1)} sec"

      loop do
        sleep sleep_time
        # TODO: error handling?
        Puma::SdNotify.status(instance.status)
      end
    end
  end

  def status
    if clustered?
      messages = stats[:worker_status].map do |worker|
        common_message(worker[:last_status])
      end.join(',')

      "Puma #{Puma::Const::VERSION}: cluster: #{booted_workers}/#{workers}, worker_status: [#{messages}]"
    else
      "Puma #{Puma::Const::VERSION}: worker: #{common_message(stats)}"
    end
  end

  private

  def watchdog_sleep_time
    usec = Integer(ENV["WATCHDOG_USEC"])

    sec_f = usec / 1_000_000.0
    # "It is recommended that a daemon sends a keep-alive notification message
    # to the service manager every half of the time returned here."
    sec_f / 2
  end

  def stats
    Puma.stats_hash
  end

  def clustered?
    stats.has_key?(:workers)
  end

  def workers
    stats.fetch(:workers, 1)
  end
Repository: puma/puma
File: lib/puma/plugin/systemd.rb (Ruby)

Code before:
# frozen_string_literal: true

require_relative '../plugin'

# Puma's systemd integration allows Puma to inform systemd:
#  1. when it has successfully started
#  2. when it is starting shutdown
#  3. periodically for a liveness check with a watchdog thread
#  4. periodically set the status
Puma::Plugin.create do
  def start(launcher)
    begin
      require_relative '../sd_notify'
    rescue LoadError
      launcher.log_writer.log "Systemd integration failed. It looks like you're trying to use systemd notify but don't have sd_notify gem installed"
      return
    end

    launcher.log_writer.log "* Enabling systemd notification integration"

    # hook_events
    launcher.events.on_booted { Puma::SdNotify.ready }
    launcher.events.on_stopped { Puma::SdNotify.stopping }
    launcher.events.on_restart { Puma::SdNotify.reloading }

    # start watchdog
    if Puma::SdNotify.watchdog?
      ping_f = watchdog_sleep_time

      in_background do
        launcher.log_writer.log "Pinging systemd watchdog every #{ping_f.round(1)} sec"
        loop do
          sleep ping_f
          Puma::SdNotify.watchdog
        end
      end
    end

    # start status loop
    instance = self

Review comment (bug):
Now that it's vendored (https://github.com/puma/puma/commit/6d8b728b42a61bcf3c1e4c698c9165a45e6071e8), I don't think there's a chance that it will fail to load. Certainly the message is wrong now since the `sd_notify` gem is no longer needed.

Code after:
# frozen_string_literal: true

require_relative '../plugin'

# Puma's systemd integration allows Puma to inform systemd:
#  1. when it has successfully started
#  2. when it is starting shutdown
#  3. periodically for a liveness check with a watchdog thread
#  4. periodically set the status
Puma::Plugin.create do
  def start(launcher)
    require_relative '../sd_notify'

    launcher.log_writer.log "* Enabling systemd notification integration"

    # hook_events
    launcher.events.on_booted { Puma::SdNotify.ready }
    launcher.events.on_stopped { Puma::SdNotify.stopping }
    launcher.events.on_restart { Puma::SdNotify.reloading }

    # start watchdog
    if Puma::SdNotify.watchdog?
      ping_f = watchdog_sleep_time

      in_background do
        launcher.log_writer.log "Pinging systemd watchdog every #{ping_f.round(1)} sec"
        loop do
          sleep ping_f
          Puma::SdNotify.watchdog
        end
      end
    end

    # start status loop
    instance = self
    sleep_time = 1.0
    in_background do
      launcher.log_writer.log "Sending status to systemd every #{sleep_time.round(1)} sec"

      loop do