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

Code before:
    private
    def ensure_output_directory_exists(path, io_name)
      unless Dir.exist?(File.dirname(path))
        raise "Cannot redirect #{io_name} to #{path}"
      end
    end

    def stats
      {
        versions: {
          puma: Puma::Const::PUMA_VERSION,
          ruby: {
            engine: RUBY_ENGINE,
            version: RUBY_VERSION,
            patchlevel: RUBY_PATCHLEVEL
          }
        }
      }
    end

    # this method call should always be guarded by `@log_writer.debug?`
    def debug_loaded_extensions(str)
      @log_writer.debug str
      re_ext = /\.#{RbConfig::CONFIG['DLEXT']}\z/i
      $LOADED_FEATURES.grep(re_ext).each { |f| @log_writer.debug("    #{f}") }
    end
  end
end


Review comment (bug):
Why not have the guard in this method?

Also, I think "Puma running in debug mode" (`PUMA_DEBUG` set) is not something only `LogWriter` should know about. In the future, more things could be happening in "debug more" (e.g. collecting more details stats) in my mind.

What do you think of adding a constant `PUMA_DEBUG` to `lib/puma.rb`? And a method `Puma.debug?` (or `Puma.debug_mode?`) and use that here. That method can also be used in both `LogWriter` and `ErrorLogger` (both do `ENV.key?('PUMA_DEBUG')` today)

Code after:
    private
    def ensure_output_directory_exists(path, io_name)
      unless Dir.exist?(File.dirname(path))
        raise "Cannot redirect #{io_name} to #{path}"
      end
    end

    def utc_iso8601(val)
      "#{val.utc.strftime '%FT%T'}Z"
    end

    def stats
      {
        versions: {
          puma: Puma::Const::PUMA_VERSION,
          ruby: {
            engine: RUBY_ENGINE,
            version: RUBY_VERSION,
            patchlevel: RUBY_PATCHLEVEL
          }
        }
      }
    end

    # this method call should always be guarded by `@log_writer.debug?`
    def debug_loaded_extensions(str)
      @log_writer.debug "────────────────────────────────── #{str}"
      re_ext = /\.#{RbConfig::CONFIG['DLEXT']}\z/i
      $LOADED_FEATURES.grep(re_ext).each { |f| @log_writer.debug("    #{f}") }
    end
  end
end
Repository: puma/puma
File: lib/puma/client.rb (Ruby)

Code before:
          end
        else
          @prev_chunk = line
          return false
        end
      end

      if @in_last_chunk
        set_ready
        true
      else
        false
      end
    end

    def set_ready
      if @body_read_start
        @env['puma.request_body_wait'] = Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_millisecond) - @body_read_start
      end
      @requests_served += 1
      @ready = true
    end

    def above_http_content_limit(value)
      return false if @http_content_length_limit.nil?

      value > @http_content_length_limit
    end
  end
end


Review comment (suggestion):
Maybe the following?   Looks a bit odd, but...
```ruby
  def above_http_content_limit(value)
    @http_content_length_limit&.< value
  end
```

Code after:
          end
        else
          @prev_chunk = line
          return false
        end
      end

      if @in_last_chunk
        set_ready
        true
      else
        false
      end
    end

    def set_ready
      if @body_read_start
        @env['puma.request_body_wait'] = Process.clock_gettime(Process::CLOCK_MONOTONIC, :float_millisecond) - @body_read_start
      end
      @requests_served += 1
      @ready = true
    end

    def above_http_content_limit(value)
      @http_content_length_limit&.< value
    end
  end
end
Repository: puma/puma
File: lib/rack/handler/puma.rb (Ruby)

Code before:
# frozen_string_literal: true

# Code below is 'odd', but don't want to define a top level constant

if Object.const_defined? :Rackup
  require 'rackup/handler'
  ::Rackup::Handler
else
  require 'rack/handler'
  ::Rack::Handler
end.module_eval <<'HEREDOC'
  module Puma
    DEFAULT_OPTIONS = {
      :Verbose => false,
      :Silent  => false
    }

    def self.config(app, options = {})
      # `require_relative` won't work with `module_eval`
      require 'puma'
      require 'puma/configuration'
      require 'puma/log_writer'
      require 'puma/launcher'

      default_options = DEFAULT_OPTIONS.dup

      # Libraries pass in values such as :Port and there is no way to determine
      # if it is a default provided by the library or a special value provided
      # by the user. A special key `user_supplied_options` can be passed. This
      # contains an array of all explicitly defined user options. We then
      # know that all other values are defaults
      if user_supplied_options = options.delete(:user_supplied_options)
        (options.keys - user_supplied_options).each do |k|
          default_options[k] = options.delete(k)
        end
      end

Review comment (refactor):
To get rid of this `module_eval` we could create a new lib/rackup/handler.rb file that is required here. The file could either duplicate the code, or we are putting the code inside a module and are including this new module inside the new rackup/handler and rack/handler :thinking: 

Code after:
# frozen_string_literal: true

# This module is used as an 'include' file in code at bottom of file
module Puma
  module RackHandler
    DEFAULT_OPTIONS = {
      :Verbose => false,
      :Silent  => false
    }

    def config(app, options = {})
      require_relative '../../puma'
      require_relative '../../puma/configuration'
      require_relative '../../puma/log_writer'
      require_relative '../../puma/launcher'

      default_options = DEFAULT_OPTIONS.dup

      # Libraries pass in values such as :Port and there is no way to determine
      # if it is a default provided by the library or a special value provided
      # by the user. A special key `user_supplied_options` can be passed. This
      # contains an array of all explicitly defined user options. We then
      # know that all other values are defaults
      if user_supplied_options = options.delete(:user_supplied_options)
        (options.keys - user_supplied_options).each do |k|
          default_options[k] = options.delete(k)
        end
      end

      conf = ::Puma::Configuration.new(options, default_options) do |user_config, file_config, default_config|
        if options.delete(:Verbose)
          require 'rack/common_logger'
          app = Rack::CommonLogger.new(app, STDOUT)
        end

        if options[:environment]
Repository: puma/puma
File: test/test_plugin_systemd_jruby.rb (Ruby)

Code before:
require_relative "helper"
require_relative "helpers/integration"

class TestPluginSystemdJruby < TestIntegration

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

  def setup
    skip "Skipped because Systemd support is linux-only" if windows? || osx?
    skip_unless :unix
    skip_unless_signal_exist? :TERM
    skip_unless :jruby

    super

    ENV["NOTIFY_SOCKET"] = "/tmp/doesntmatter"
  end

  def test_systemd_plugin_not_loaded
    cli_server "test/rackup/hello.ru"

    assert_nil Puma::Plugins.instance_variable_get(:@plugins)["systemd"]

    stop_server
  end
end


Review comment (refactor):
@mohamedhafez Let's add a new `:linux` symbol to our `skip` logic in `test/helper.rb` and use it here.

Code after:
require_relative "helper"
require_relative "helpers/integration"

class TestPluginSystemdJruby < TestIntegration

  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_unless :jruby

    super

    ENV["NOTIFY_SOCKET"] = "/tmp/doesntmatter"
  end

  def test_systemd_plugin_not_loaded
    cli_server "test/rackup/hello.ru"

    assert_nil Puma::Plugins.instance_variable_get(:@plugins)["systemd"]

    stop_server
  end
end
Repository: puma/puma
File: test/test_integration_single.rb (Ruby)

Code before:
  end

  def test_puma_debug_loaded_exts
    cli_server "#{set_pumactl_args} test/rackup/hello.ru", puma_debug: true

    assert wait_for_server_to_include('Loaded Extensions:')

    cli_pumactl 'stop'
  end

  def test_pre_existing_unix_after_idle_timeout
    skip_unless :unix

    File.open(@bind_path, mode: 'wb') { |f| f.puts 'pre existing' }

    cli_server "-q test/rackup/hello.ru", unix: :unix, config: "idle_timeout 1"

    sleep 1.15 # ensure the timeout isn't set until the first connection

    sock = connection = connect(nil, unix: true)
    read_body(connection)

    sleep 1.15

    assert sock.wait_readable(1), 'Unexpected timeout'
    assert_raises Errno::ECONNREFUSED do
      connection = connect(nil, unix: true)
    end

    assert File.exist?(@bind_path)
  ensure
    if UNIX_SKT_EXIST
      File.unlink @bind_path if File.exist? @bind_path
    end
  end
end


Review comment (suggestion):
Looks like JRuby raises `IOError` with message `Connection refused` (e.g. https://github.com/puma/puma/actions/runs/5847300372/job/15853513628#step:8:809)

Code after:
  end

  def test_puma_debug_loaded_exts
    cli_server "#{set_pumactl_args} test/rackup/hello.ru", puma_debug: true

    assert wait_for_server_to_include('Loaded Extensions:')

    cli_pumactl 'stop'
  end

  def test_pre_existing_unix_after_idle_timeout
    skip_unless :unix

    File.open(@bind_path, mode: 'wb') { |f| f.puts 'pre existing' }

    cli_server "-q test/rackup/hello.ru", unix: :unix, config: "idle_timeout 1"

    sock = connection = connect(nil, unix: true)
    read_body(connection)

    sleep 1.15

    assert sock.wait_readable(1), 'Unexpected timeout'
    assert_raises Puma.jruby? ? IOError : Errno::ECONNREFUSED, "Connection refused" do
      connection = connect(nil, unix: true)
    end

    assert File.exist?(@bind_path)
  ensure
    if UNIX_SKT_EXIST
      File.unlink @bind_path if File.exist? @bind_path
    end
  end
end
Repository: puma/puma
File: test/test_plugin_systemd_jruby.rb (Ruby)

Code before:
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/integration"

require 'puma/plugin'

class TestPluginSystemdJruby < TestIntegration

  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_unless :jruby

    super

    ENV["NOTIFY_SOCKET"] = "/tmp/doesntmatter"
  end

  def teardown
    super unless skipped?
  end

  def test_systemd_plugin_not_loaded
    cli_server "test/rackup/hello.ru"

    assert_nil Puma::Plugins.instance_variable_get(:@plugins)["systemd"]

Review comment (suggestion):
```suggestion
require "puma/plugin"
```

I dislike mixing use of `"` and `'` and seeing `"` being used just two lines above I thought I should point out this :) 

Not sure if we have any RuboCop rule for this and if it would be worth to get us being consistent about it, but my thinking is that we could at least try to not make it more inconsistent with new code 😛 

Code after:
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/integration"

require "puma/plugin"

class TestPluginSystemdJruby < TestIntegration

  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_unless :jruby

    super

    ENV["NOTIFY_SOCKET"] = "/tmp/doesntmatter"
  end

  def teardown
    super unless skipped?
  end

  def test_systemd_plugin_not_loaded
    cli_server "test/rackup/hello.ru"

    assert_nil Puma::Plugins.instance_variable_get(:@plugins)["systemd"]
Repository: puma/puma
File: test/test_puma_server.rb (Ruby)

Code before:
    @server = Puma::Server.new @app, @events, {log_writer: @log_writer}
  end

  def teardown
    @server.stop(true)
    # Errno::EBADF raised on macOS
    @ios.each do |io|
      begin
        io.close if io.respond_to?(:close) && !io.closed?
        File.unlink io.path if io.is_a? File
      rescue Errno::EBADF
      ensure
        io = nil
      end
    end
  end

  def server_run(**options, &block)
    options[:log_writer]  ||= @log_writer
    options[:min_threads] ||= 1
    @server = Puma::Server.new block || @app, @events, options
    @port = (@server.add_tcp_listener @host, 0).addr[1]
    @server.run
  end

  def header(skt)
    header = []
    while true
      skt.wait_readable 5
      line = skt.gets
      break if line == "\r\n"
      header << line.strip
    end

    header
  end

  # only for shorter bodies!
  def send_http_and_sysread(req)
    skt = send_http(req)
    skt.wait_readable 5
    skt.sysread 2_048
  end

  def send_http_and_read(req)
    skt = send_http req
    skt.wait_readable 5
    skt.read
  end

  def send_http(req)

Review comment (suggestion):
Oh no, not "skt" again... IMHO "sock" (or why not "socket"?) is more clear (less cryptic) than "skt" 

Though I suspect I'm too late, there's already a lot of "skt" usage in main: https://github.com/search?q=repo%3Apuma%2Fpuma+skt+language%3ARuby&type=code&l=Ruby

Code after:
    @server = Puma::Server.new @app, @events, {log_writer: @log_writer}
  end

  def teardown
    @server.stop(true)
    # Errno::EBADF raised on macOS
    @ios.each do |io|
      begin
        io.close if io.respond_to?(:close) && !io.closed?
        File.unlink io.path if io.is_a? File
      rescue Errno::EBADF
      ensure
        io = nil
      end
    end
  end

  def server_run(**options, &block)
    options[:log_writer]  ||= @log_writer
    options[:min_threads] ||= 1
    @server = Puma::Server.new block || @app, @events, options
    @port = (@server.add_tcp_listener @host, 0).addr[1]
    @server.run
  end

  def header(socket)
    header = []
    while true
      socket.wait_readable 5
      line = socket.gets
      break if line == "\r\n"
      header << line.strip
    end

    header
  end

  # only for shorter bodies!
  def send_http_and_sysread(req)
    socket = send_http(req)
    socket.wait_readable 5
    socket.sysread 2_048
  end

  def send_http_and_read(req)
    socket = send_http req
    socket.wait_readable 5
    socket.read
  end

  def send_http(req)
Repository: puma/puma
File: test/test_cli.rb (Ruby)

Code before:
    cli.send(:setup_options)

    assert_equal @environment, cli.instance_variable_get(:@conf).environment
  ensure
    ENV.delete 'RAILS_ENV'
  end

  def test_silent
    cli = Puma::CLI.new ['--silent']
    cli.send(:setup_options)

    log_writer = cli.instance_variable_get(:@log_writer)

    assert_equal log_writer.class, Puma::LogWriter.null.class
    assert_equal log_writer.stdout.class, Puma::NullIO
    assert_equal log_writer.stderr, $stderr
  end

  def test_plugins
    assert_empty Puma::Plugins.instance_variable_get(:@plugins)

    cli = Puma::CLI.new ['--plugin', 'tmp_restart', '--plugin', 'systemd']
    cli.send(:setup_options)

    assert Puma::Plugins.instance_variable_get(:@plugins)["tmp_restart"]
    assert Puma::Plugins.instance_variable_get(:@plugins)["systemd"]
  end
end


Review comment (suggestion):
I think we can use the `#find` method over this?

https://github.com/puma/puma/blob/119330475ea62126d76b47fe202ac89dad1417dc/lib/puma/plugin.rb#L40

Code after:
    cli.send(:setup_options)

    assert_equal @environment, cli.instance_variable_get(:@conf).environment
  ensure
    ENV.delete 'RAILS_ENV'
  end

  def test_silent
    cli = Puma::CLI.new ['--silent']
    cli.send(:setup_options)

    log_writer = cli.instance_variable_get(:@log_writer)

    assert_equal log_writer.class, Puma::LogWriter.null.class
    assert_equal log_writer.stdout.class, Puma::NullIO
    assert_equal log_writer.stderr, $stderr
  end

  def test_plugins
    assert_empty Puma::Plugins.instance_variable_get(:@plugins)

    cli = Puma::CLI.new ['--plugin', 'tmp_restart', '--plugin', 'systemd']
    cli.send(:setup_options)

    assert Puma::Plugins.find("tmp_restart")
    assert Puma::Plugins.find("systemd")
  end
end
Repository: puma/puma
File: test/test_worker_gem_independence.rb (Ruby)

Code before:

  def set_release_symlink(target_dir)
    FileUtils.rm current_release_symlink, force: true
    FileUtils.symlink target_dir, current_release_symlink, force: true
  end

  def start_phased_restart
    Process.kill :USR1, @pid

    true while @server.gets !~ /booted in [.0-9]+s, phase: 1/
  end

  def with_unbundled_env
    bundler_ver = Gem::Version.new(Bundler::VERSION)
    if bundler_ver < Gem::Version.new('2.1.0')
      Bundler.with_clean_env { yield }
    else
      Bundler.with_unbundled_env { yield }
    end
  end

  def verify_process_tag(pid, tag)
    cmd = "ps aux | grep #{pid}"
    io = IO.popen cmd, 'r'
    assert io.read.include? tag
  end
end


Review comment (suggestion):
@dentarg 

Added testcases in `TestWorkerGemIndependence`. not sure if it should be in separate or new class.

also used grep here as I did not find any alternative to get the title of Process running. we are using `setproctitle` to set the title but there is no equivalent to this for get.

Code after:
  end

  def current_release_symlink
    File.expand_path "worker_gem_independence_test/current", __dir__
  end

  def set_release_symlink(target_dir)
    FileUtils.rm current_release_symlink, force: true
    FileUtils.symlink target_dir, current_release_symlink, force: true
  end

  def start_phased_restart
    Process.kill :USR1, @pid

    true while @server.gets !~ /booted in [.0-9]+s, phase: 1/
  end

  def with_unbundled_env
    bundler_ver = Gem::Version.new(Bundler::VERSION)
    if bundler_ver < Gem::Version.new('2.1.0')
      Bundler.with_clean_env { yield }
    else
      Bundler.with_unbundled_env { yield }
    end
  end

  def verify_process_tag(pid, tag)
    cmd = "ps aux | grep #{pid}"
    io = IO.popen cmd, 'r'
    assert io.read.include? tag
  end
end
Repository: puma/puma
File: test/test_skip_systemd.rb (Ruby)

Code before:
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/integration"

require "puma/plugin"

class TestSkipSystemd < TestIntegration

  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

    ENV["PUMA_SKIP_SYSTEMD"] = "true"
    ENV["NOTIFY_SOCKET"] = "/tmp/doesntmatter"
  end

  def teardown
    super unless skipped?
  end

  def test_systemd_plugin_not_loaded
    cli_server "test/rackup/hello.ru",
               env: {'NOTIFY_SOCKET' => '/tmp/doesntmatter' }, config: <<~CONFIG
      app do |_|
        [200, {}, [Puma::Plugins.instance_variable_get(:@plugins)['systemd'].to_s]]
      end
    CONFIG


Review comment (suggestion):
Hmm, do we need this here? :)

I see we added it in https://github.com/puma/puma/pull/3079 that this is probably based on, but maybe it wasn't needed there too (feel free to remove it from there now as well if it isn't needed)

Code after:
# frozen_string_literal: true

require_relative "helper"
require_relative "helpers/integration"

require "puma/plugin"

class TestSkipSystemd < TestIntegration

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

    super
  end

  def teardown
    super unless skipped?
  end

  def test_systemd_plugin_not_loaded
    cli_server "test/rackup/hello.ru",
               env: { 'PUMA_SKIP_SYSTEMD' => 'true', 'NOTIFY_SOCKET' => '/tmp/doesntmatter' }, config: <<~CONFIG
      app do |_|
        [200, {}, [Puma::Plugins.instance_variable_get(:@plugins)['systemd'].to_s]]
      end
    CONFIG

    assert_empty read_body(connect)

    stop_server
  end
end
Repository: puma/puma
File: test/test_integration_cluster.rb (Ruby)

Code before:
    cli_server "-w 2 test/rackup/hello.ru", config: <<~RUBY
      worker_culling_strategy :oldest
      fork_worker
    RUBY

    get_worker_pids # to consume server logs

    Process.kill :TTIN, @pid

    line = @server.gets
    assert_match(/Worker 2 \(PID: \d+\) booted in/, line)

    Process.kill :TTOU, @pid

    line = @server.gets
    assert_match(/Worker 1 \(PID: \d+\) terminating/, line)
  end

  def test_hook_data
    skip_unless_signal_exist? :TERM

    cli_server "-C test/config/hook_data.rb test/rackup/hello.ru"
    get_worker_pids 0, 2 # make sure workers are booted
    stop_server

    ary = Array.new(2) do |_|
      wait_for_server_to_match(/(index \d data \d)/, 1)
    end.sort

    assert 'index 0 data 0', ary[0]
    assert 'index 1 data 1', ary[1]
  end

  def test_worker_hook_warning_cli
    cli_server "-w2 test/rackup/hello.ru", config: <<~CONFIG
      on_worker_boot(:test) do |index, data|
        data[:test] = index
      end
    CONFIG

    get_worker_pids
    line = @server_log[/.+on_worker_boot.+/]
    refute line, "Warning below should not be shown!\n#{line}"
  end

  def test_worker_hook_warning_web_concurrency
    cli_server "test/rackup/hello.ru",
      env: { 'WEB_CONCURRENCY' => '2'},
      config: <<~CONFIG
      on_worker_boot(:test) do |index, data|
        data[:test] = index

Review comment (suggestion):
```suggestion
    ary = Array.new(2) do |_index|
```

Code after:
    cli_server "-w 2 test/rackup/hello.ru", config: <<~RUBY
      worker_culling_strategy :oldest
      fork_worker
    RUBY

    get_worker_pids # to consume server logs

    Process.kill :TTIN, @pid

    line = @server.gets
    assert_match(/Worker 2 \(PID: \d+\) booted in/, line)

    Process.kill :TTOU, @pid

    line = @server.gets
    assert_match(/Worker 1 \(PID: \d+\) terminating/, line)
  end

  def test_hook_data
    skip_unless_signal_exist? :TERM

    cli_server "-C test/config/hook_data.rb test/rackup/hello.ru"
    get_worker_pids 0, 2 # make sure workers are booted
    stop_server

    ary = Array.new(2) do |_index|
      wait_for_server_to_match(/(index \d data \d)/, 1)
    end.sort

    assert 'index 0 data 0', ary[0]
    assert 'index 1 data 1', ary[1]
  end

  def test_worker_hook_warning_cli
    cli_server "-w2 test/rackup/hello.ru", config: <<~CONFIG
      on_worker_boot(:test) do |index, data|
        data[:test] = index
      end
    CONFIG

    get_worker_pids
    line = @server_log[/.+on_worker_boot.+/]
    refute line, "Warning below should not be shown!\n#{line}"
  end

  def test_worker_hook_warning_web_concurrency
    cli_server "test/rackup/hello.ru",
      env: { 'WEB_CONCURRENCY' => '2'},
      config: <<~CONFIG
      on_worker_boot(:test) do |index, data|
        data[:test] = index