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

Code before:
    # this tests all values but the last, which must be chunked
    ALLOWED_TRANSFER_ENCODING = %w[compress deflate gzip].freeze

    # chunked body validation
    CHUNK_SIZE_INVALID = /[^\h]/.freeze
    CHUNK_VALID_ENDING = Const::LINE_END
    CHUNK_VALID_ENDING_SIZE = CHUNK_VALID_ENDING.bytesize

    # The maximum number of bytes we'll buffer looking for a valid
    # chunk header.
    MAX_CHUNK_HEADER_SIZE = 4096

    # The maximum amount of excess data the client sends
    # using chunk size extensions before we abort the connection.
    MAX_CHUNK_EXCESS = 16 * 1024

    # Content-Length header value validation
    CONTENT_LENGTH_VALUE_INVALID = /[^\d]/.freeze

    TE_ERR_MSG = 'Invalid Transfer-Encoding'

    # See:
    # https://httpwg.org/specs/rfc9110.html#rfc.section.5.6.1.1
    # https://httpwg.org/specs/rfc9112.html#rfc.section.6.1
    OWS_PREFIX = /\A[ \t]+/
    OWS_SUFFIX = /[ \t]+\z/

    # The object used for a request with no body. All requests with
    # no body share this one object since it has no state.
    EmptyBody = NullIO.new

    include Puma::Const

    def initialize(io, env=nil)
      @io = io
      @to_io = io.to_io
      @io_buffer = IOBuffer.new
      @proto_env = env
      @env = env&.dup

      @parser = HttpParser.new
      @parsed_bytes = 0
      @read_header = true
      @read_proxy = false
      @ready = false

      @body = nil
      @body_read_start = nil
      @buffer = nil
      @tempfile = nil


Review comment (suggestion):
```suggestion
    STRIP_OWS = /\A[ \t]+|[ \t]+\z/
```

Code after:
    # this tests all values but the last, which must be chunked
    ALLOWED_TRANSFER_ENCODING = %w[compress deflate gzip].freeze

    # chunked body validation
    CHUNK_SIZE_INVALID = /[^\h]/.freeze
    CHUNK_VALID_ENDING = Const::LINE_END
    CHUNK_VALID_ENDING_SIZE = CHUNK_VALID_ENDING.bytesize

    # The maximum number of bytes we'll buffer looking for a valid
    # chunk header.
    MAX_CHUNK_HEADER_SIZE = 4096

    # The maximum amount of excess data the client sends
    # using chunk size extensions before we abort the connection.
    MAX_CHUNK_EXCESS = 16 * 1024

    # Content-Length header value validation
    CONTENT_LENGTH_VALUE_INVALID = /[^\d]/.freeze

    TE_ERR_MSG = 'Invalid Transfer-Encoding'

    # See:
    # https://httpwg.org/specs/rfc9110.html#rfc.section.5.6.1.1
    # https://httpwg.org/specs/rfc9112.html#rfc.section.6.1
    STRIP_OWS = /\A[ \t]+|[ \t]+\z/

    # The object used for a request with no body. All requests with
    # no body share this one object since it has no state.
    EmptyBody = NullIO.new

    include Puma::Const

    def initialize(io, env=nil)
      @io = io
      @to_io = io.to_io
      @io_buffer = IOBuffer.new
      @proto_env = env
      @env = env&.dup

      @parser = HttpParser.new
      @parsed_bytes = 0
      @read_header = true
      @read_proxy = false
      @ready = false

      @body = nil
      @body_read_start = nil
      @buffer = nil
      @tempfile = nil

      @timeout_at = nil
Repository: puma/puma
File: lib/puma/configuration.rb (Ruby)

Code before:
    def load_rackup
      raise "Missing rackup file '#{rackup}'" unless File.exist?(rackup)

      rack_app, rack_options = rack_builder.parse_file(rackup)
      rack_options = rack_options || {}

      options.file_options.merge!(rack_options)

      config_ru_binds = []
      rack_options.each do |k, v|
        config_ru_binds << v if k.to_s.start_with?("bind")
      end

      options.file_options[:binds] = config_ru_binds unless config_ru_binds.empty?

      rack_app
    end

    def set_conditional_default_options
      @_options.default_options[:preload_app] = !@_options[:prune_bundler] &&
        (@_options[:workers] > 1) && Puma.forkable?
    end

    def warn_hooks
      return if options[:workers] > 0
      return if options[:silence_fork_callback_warning]

      @hooks.each do |key, method|
        options.all_of(key).each do |hook_options|
          next unless hook_options[:cluster_only]

          LogWriter.stdio.log(<<~MSG.tr("\n", " "))
            Warning: The code in the `#{method}` block will not execute
            in the current Puma configuration. The `#{method}` block only
            executes in Puma's cluster mode. To fix this, either remove the
            `#{method}` call or increase Puma's worker count above zero.
          MSG
        end
      end
    end
  end
end


Review comment (suggestion):
We were initialising a logger per-log, which is unnecessary. Ref: https://github.com/puma/puma/blob/1acac470543a8892fa921c98f61118edfa24cfa0/lib/puma/error_logger.rb#L24-L26

Code after:
      end
    end

    def load_rackup
      raise "Missing rackup file '#{rackup}'" unless File.exist?(rackup)

      rack_app, rack_options = rack_builder.parse_file(rackup)
      rack_options = rack_options || {}

      options.file_options.merge!(rack_options)

      config_ru_binds = []
      rack_options.each do |k, v|
        config_ru_binds << v if k.to_s.start_with?("bind")
      end

      options.file_options[:binds] = config_ru_binds unless config_ru_binds.empty?

      rack_app
    end

    def warn_hooks
      return if options[:workers] > 0
      return if options[:silence_fork_callback_warning]

      log_writer = LogWriter.stdio
      @hooks.each_key do |hook|
        options.all_of(hook).each do |hook_options|
          next unless hook_options[:cluster_only]

          log_writer.log(<<~MSG.tr("\n", " "))
            Warning: The code in the `#{hook}` block will not execute
            in the current Puma configuration. The `#{hook}` block only
            executes in Puma's cluster mode. To fix this, either remove the
            `#{hook}` call or increase Puma's worker count above zero.
          MSG
        end
      end
    end
  end
end
Repository: puma/puma
File: lib/puma/dsl.rb (Ruby)

Code before:

    private

    # To avoid adding cert_pem and key_pem as URI params, we store them on the
    # options[:store] from where Puma binder knows how to find and extract them.
    #
    def add_pem_values_to_options_store(opts)
      return if defined?(JRUBY_VERSION)

      @options[:store] ||= []

      # Store cert_pem and key_pem to options[:store] if present
      [:cert, :key].each do |v|
        opt_key = :"#{v}_pem"
        if opts[opt_key]
          index = @options[:store].length
          @options[:store] << opts[opt_key]
          opts[v] = "store:#{index}"
        end
      end
    end

    def process_hook(options_key, key, block, method, cluster_only: false)
      raise ArgumentError, "expected #{method} to be given a block" unless block

      @config.hooks[options_key] = method

      @options[options_key] ||= []
      hook_options = { block: block, cluster_only: cluster_only }
      hook_options[:id] = if ON_WORKER_KEY.include? key.class
        key.to_sym
      elsif key.nil?
        nil
      else
        raise "'#{method}' key must be String or Symbol"
      end
      @options[options_key] << hook_options
    end
  end
end


Review comment (suggestion):
We still need `#hooks` to keep track of which hook types were registered.

Code after:

    private

    # To avoid adding cert_pem and key_pem as URI params, we store them on the
    # options[:store] from where Puma binder knows how to find and extract them.
    #
    def add_pem_values_to_options_store(opts)
      return if defined?(JRUBY_VERSION)

      @options[:store] ||= []

      # Store cert_pem and key_pem to options[:store] if present
      [:cert, :key].each do |v|
        opt_key = :"#{v}_pem"
        if opts[opt_key]
          index = @options[:store].length
          @options[:store] << opts[opt_key]
          opts[v] = "store:#{index}"
        end
      end
    end

    def process_hook(options_key, key, block, cluster_only: false)
      raise ArgumentError, "expected #{options_key} to be given a block" unless block

      @config.hooks[options_key] = true

      @options[options_key] ||= []
      hook_options = { block: block, cluster_only: cluster_only }
      hook_options[:id] = if ON_WORKER_KEY.include?(key.class)
        key.to_sym
      elsif key.nil?
        nil
      else
        raise "'#{options_key}' key must be String or Symbol"
      end
      @options[options_key] << hook_options
    end
  end
end
Repository: puma/puma
File: ext/puma_http11/puma_http11.c (C)

Code before:
                                 const char *value, size_t vlen)
{
  VALUE f = Qnil;
  VALUE v;

  VALIDATE_MAX_LENGTH(flen, FIELD_NAME);
  VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);

  f = find_common_field_value(field, flen);

  if (f == Qnil) {
    /*
     * We got a strange header that we don't have a memoized value for.
     * Fallback to creating a new string to use as a hash key.
     */

    size_t new_size = HTTP_PREFIX_LEN + flen;
    assert(new_size < BUFFER_LEN);

    memcpy(hp->buf, HTTP_PREFIX, HTTP_PREFIX_LEN);
    memcpy(hp->buf + HTTP_PREFIX_LEN, field, flen);

    f = rb_str_new(hp->buf, new_size);
  }

  while (vlen > 0 && (value[vlen - 1] == ' ' || value[vlen - 1] == '\t')) vlen--;
  while (vlen > 0 && (value[0] == ' ' || value[0] == '\t')) {
      vlen--;
      value++;
  }

  /* check for duplicate header */
  v = rb_hash_aref(hp->request, f);

  if (v == Qnil) {
      v = rb_str_new(value, vlen);
      rb_hash_aset(hp->request, f, v);
  } else {
      /* if duplicate header, normalize to comma-separated values */
      rb_str_cat2(v, ", ");
      rb_str_cat(v, value, vlen);
  }
}

void request_method(puma_parser* hp, const char *at, size_t length)
{
  VALUE val = Qnil;

  val = rb_str_new(at, length);
  rb_hash_aset(hp->request, global_request_method, val);
}

Review comment (bug):
(I think, C is not my daily driver) We should be using `isspace` instead of rolling our own check. The `isspace` call already handles tab:

- https://man.openbsd.org/isspace.3
- https://manpages.debian.org/testing/manpages-dev/isspace.3.en.html

The code on main already strips leading whitespace, the reason your test fails on main is because of the trailing whitespace. 

Code after:
    return c == ' ' || c == '\t';
}

void http_field(puma_parser* hp, const char *field, size_t flen,
                                 const char *value, size_t vlen)
{
  VALUE f = Qnil;
  VALUE v;

  VALIDATE_MAX_LENGTH(flen, FIELD_NAME);
  VALIDATE_MAX_LENGTH(vlen, FIELD_VALUE);

  f = find_common_field_value(field, flen);

  if (f == Qnil) {
    /*
     * We got a strange header that we don't have a memoized value for.
     * Fallback to creating a new string to use as a hash key.
     */

    size_t new_size = HTTP_PREFIX_LEN + flen;
    assert(new_size < BUFFER_LEN);

    memcpy(hp->buf, HTTP_PREFIX, HTTP_PREFIX_LEN);
    memcpy(hp->buf + HTTP_PREFIX_LEN, field, flen);

    f = rb_str_new(hp->buf, new_size);
  }

  while (vlen > 0 && is_ows(value[vlen - 1])) vlen--;
  while (vlen > 0 && is_ows(value[0])) {
      vlen--;
      value++;
  }

  /* check for duplicate header */
  v = rb_hash_aref(hp->request, f);

  if (v == Qnil) {
      v = rb_str_new(value, vlen);
      rb_hash_aset(hp->request, f, v);
  } else {
      /* if duplicate header, normalize to comma-separated values */
      rb_str_cat2(v, ", ");
      rb_str_cat(v, value, vlen);
  }
}

void request_method(puma_parser* hp, const char *at, size_t length)
{
  VALUE val = Qnil;
Repository: puma/puma
File: lib/puma/thread_pool.rb (Ruby)

Code before:
# frozen_string_literal: true

require 'thread'

require_relative 'io_buffer'

module Puma
  # Internal Docs for A simple thread pool management object.
  #
  # Each Puma "worker" has a thread pool to process requests.
  #
  # First a connection to a client is made in `Puma::Server`. It is wrapped in a
  # `Puma::Client` instance and then passed to the `Puma::Reactor` to ensure
  # the whole request is buffered into memory. Once the request is ready, it is passed into
  # a thread pool via the `Puma::ThreadPool#<<` operator where it is stored in a `@todo` array.
  #
  # Each thread in the pool has an internal loop where it pulls a request from the `@todo` array
  # and processes it.
  class ThreadPool
    class ForceShutdown < RuntimeError
    end

    # How long, after raising the ForceShutdown of a thread during
    # forced shutdown mode, to wait for the thread to try and finish
    # up its work before leaving the thread to die on the vine.
    SHUTDOWN_GRACE_TIME = 5 # seconds

    attr_reader :out_of_band_running

    # Maintain a minimum of +min+ and maximum of +max+ threads
    # in the pool.
    #
    # The block passed is the work that will be performed in each

Review comment (suggestion):
Nit - feel free to ignore if this is your personal preference.


```suggestion
```

Code after:
# frozen_string_literal: true

require 'thread'

require_relative 'io_buffer'

module Puma

  # Add `Thread#puma_server` and `Thread#puma_server=`
  Thread.attr_accessor(:puma_server)

  # Internal Docs for A simple thread pool management object.
  #
  # Each Puma "worker" has a thread pool to process requests.
  #
  # First a connection to a client is made in `Puma::Server`. It is wrapped in a
  # `Puma::Client` instance and then passed to the `Puma::Reactor` to ensure
  # the whole request is buffered into memory. Once the request is ready, it is passed into
  # a thread pool via the `Puma::ThreadPool#<<` operator where it is stored in a `@todo` array.
  #
  # Each thread in the pool has an internal loop where it pulls a request from the `@todo` array
  # and processes it.
  class ThreadPool
    class ForceShutdown < RuntimeError
    end

    # How long, after raising the ForceShutdown of a thread during
    # forced shutdown mode, to wait for the thread to try and finish
    # up its work before leaving the thread to die on the vine.
    SHUTDOWN_GRACE_TIME = 5 # seconds

    attr_reader :out_of_band_running
Repository: puma/puma
File: lib/puma/client.rb (Ruby)

Code before:
      else
        # The body[0,0] trick is to get an empty string in the same
        # encoding as body.
        @body = StringIO.new body[0,0]
      end

      @body.write body

      @body_remain = remain

      false
    end

    def read_body
      if @chunked_body
        return read_chunked_body
      end

      # Read an odd sized chunk so we can read even sized ones
      # after this
      remain = @body_remain

      # don't bother with reading zero bytes
      unless remain.zero?

        want = remain > CHUNK_SIZE ? CHUNK_SIZE : remain

        begin
          chunk = @io.read_nonblock(want, @read_buffer)
        rescue IO::WaitReadable
          return false
        rescue SystemCallError, IOError
          raise ConnectionError, "Connection error detected during read"
        end

        # No chunk means a closed socket
        unless chunk
          @body.close
          @buffer = nil
          set_ready
          raise EOFError
        end

        remain -= @body.write(chunk)
      end

      if remain <= 0
        @body.rewind
        @buffer = nil
        set_ready
        return true

Review comment (refactor):
Optional Suggestion: We can also remove this variable assignment and move this logic inline later:

```ruby
        begin
          chunk = @io.read_nonblock(remain.clamp(0, CHUNK_SIZE), @read_buffer)
```

I feel like that's clearer and cleaner. I'm fine with the code as is, but would prefer `clamp` if you are willing to make that change.

Code after:
      else
        # The body[0,0] trick is to get an empty string in the same
        # encoding as body.
        @body = StringIO.new body[0,0]
      end

      @body.write body

      @body_remain = remain

      false
    end

    def read_body
      if @chunked_body
        return read_chunked_body
      end

      # Read an odd sized chunk so we can read even sized ones
      # after this
      remain = @body_remain

      # don't bother with reading zero bytes
      unless remain.zero?
        begin
          chunk = @io.read_nonblock(remain.clamp(0, CHUNK_SIZE), @read_buffer)
        rescue IO::WaitReadable
          return false
        rescue SystemCallError, IOError
          raise ConnectionError, "Connection error detected during read"
        end

        # No chunk means a closed socket
        unless chunk
          @body.close
          @buffer = nil
          set_ready
          raise EOFError
        end

        remain -= @body.write(chunk)
      end

      if remain <= 0
        @body.rewind
        @buffer = nil
        set_ready
        return true
      end

      @body_remain = remain
Repository: puma/puma
File: lib/puma/configuration.rb (Ruby)

Code before:

    def self.random_token
      require 'securerandom' unless defined?(SecureRandom)

      SecureRandom.hex(16)
    end

    private

    def require_processor_counter
      require 'concurrent/utility/processor_counter'
    rescue LoadError
      warn <<~MESSAGE
        WEB_CONCURRENCY=auto or workers(:auto) requires the "concurrent-ruby" gem to be installed.
        Please add "concurrent-ruby" to your Gemfile.
      MESSAGE
      raise
    end

    def parse_workers(value)
      if value == :auto || value == 'auto'
        require_processor_counter
        return Integer(::Concurrent.available_processor_count)
      end

      Integer(value)
    rescue ArgumentError, TypeError
      raise ArgumentError, "workers must be an Integer or :auto"
    end

    # Load and use the normal Rack builder if we can, otherwise
    # fallback to our minimal version.
    def rack_builder
      # Load bundler now if we can so that we can pickup rack from
      # a Gemfile
      if @puma_bundler_pruned
        begin
          require 'bundler/setup'
        rescue LoadError
        end
      end

      begin
        require 'rack'
        require 'rack/builder'
        ::Rack::Builder
      rescue LoadError
        require_relative 'rack/builder'
        Puma::Rack::Builder
      end
    end

Review comment (suggestion):
Maybe update with the following:

```ruby
      if value == :auto || value == 'auto'
        require_processor_counter
        Integer(::Concurrent.available_processor_count)
      else
        Integer(value)
      end
```

Code after:

    def self.random_token
      require 'securerandom' unless defined?(SecureRandom)

      SecureRandom.hex(16)
    end

    private

    def require_processor_counter
      require 'concurrent/utility/processor_counter'
    rescue LoadError
      warn <<~MESSAGE
        WEB_CONCURRENCY=auto or workers(:auto) requires the "concurrent-ruby" gem to be installed.
        Please add "concurrent-ruby" to your Gemfile.
      MESSAGE
      raise
    end

    def parse_workers(value)
      if value == :auto || value == 'auto'
        require_processor_counter
        Integer(::Concurrent.available_processor_count)
      else
        Integer(value)
      end
    rescue ArgumentError, TypeError
      raise ArgumentError, "workers must be an Integer or :auto"
    end

    # Load and use the normal Rack builder if we can, otherwise
    # fallback to our minimal version.
    def rack_builder
      # Load bundler now if we can so that we can pickup rack from
      # a Gemfile
      if @puma_bundler_pruned
        begin
          require 'bundler/setup'
        rescue LoadError
        end
      end

      begin
        require 'rack'
        require 'rack/builder'
        ::Rack::Builder
      rescue LoadError
        require_relative 'rack/builder'
        Puma::Rack::Builder
      end
    end
Repository: puma/puma
File: test/test_config.rb (Ruby)

Code before:
    assert_equal default_max_threads, conf.options.default_options[:max_threads]
  end

  def test_config_loads_workers_from_env
    env = { "WEB_CONCURRENCY" => "9" }
    conf = Puma::Configuration.new({}, {}, env)
    conf.clamp
    assert_equal 9, conf.options.default_options[:workers]
  end

  def test_config_ignores_blank_workers_from_env
    env = { "WEB_CONCURRENCY" => "" }
    conf = Puma::Configuration.new({}, {}, env)
    conf.clamp
    assert_equal 0, conf.options.default_options[:workers]
  end

  def test_config_does_not_preload_app_if_not_using_workers
    env = { "WEB_CONCURRENCY" => "0" }
    conf = Puma::Configuration.new({}, {}, env)
    conf.clamp
    assert_equal false, conf.options.default_options[:preload_app]
  end

  def test_weird_web_concurrency_does_not_break
    [nil, '', ' ', 'hello', ' hello ', '!?', ' !? '].each do |wc|
      env = { "WEB_CONCURRENCY" => wc }
      conf = Puma::Configuration.new({}, {}, env)
      conf.clamp
      assert_equal false, conf.options.default_options[:preload_app]
    end
  end

  def test_config_preloads_app_if_using_workers
    env = { "WEB_CONCURRENCY" => "2" }
    preload = Puma.forkable?
    conf = Puma::Configuration.new({}, {}, env)
    conf.clamp
    assert_equal preload, conf.options.default_options[:preload_app]
  end
end

class TestConfigFileWithFakeEnv < PumaTest
  def setup
    FileUtils.mkpath("config/puma")
    File.write("config/puma/fake-env.rb", "")
  end

  def teardown
    FileUtils.rm_r("config/puma")
  end

Review comment (suggestion):
```suggestion
  def test_blank_web_concurrency_does_not_break
    [nil, '', ' ', " \n ", " \t "].each do |wc|
```

Code after:
    assert_equal default_max_threads, conf.options.default_options[:max_threads]
  end

  def test_config_loads_workers_from_env
    env = { "WEB_CONCURRENCY" => "9" }
    conf = Puma::Configuration.new({}, {}, env)
    conf.clamp
    assert_equal 9, conf.options.default_options[:workers]
  end

  def test_config_ignores_blank_workers_from_env
    [nil, '', ' ', " \n ", " \t "].each do |wc|
      env = { "WEB_CONCURRENCY" => wc }
      conf = Puma::Configuration.new({}, {}, env)
      conf.clamp
      assert_equal 0, conf.options.default_options[:workers]
    end
  end

  def test_config_does_not_preload_app_if_not_using_workers
    env = { "WEB_CONCURRENCY" => "0" }
    conf = Puma::Configuration.new({}, {}, env)
    conf.clamp
    assert_equal false, conf.options.default_options[:preload_app]
  end

  def test_config_preloads_app_if_using_workers
    env = { "WEB_CONCURRENCY" => "2" }
    preload = Puma.forkable?
    conf = Puma::Configuration.new({}, {}, env)
    conf.clamp
    assert_equal preload, conf.options.default_options[:preload_app]
  end
end

class TestConfigFileWithFakeEnv < PumaTest
  def setup
    FileUtils.mkpath("config/puma")
    File.write("config/puma/fake-env.rb", "")
  end

  def teardown
    FileUtils.rm_r("config/puma")
  end

  def test_config_files_with_app_env
    env = { 'APP_ENV' => 'fake-env' }

    conf = Puma::Configuration.new({}, {}, env)
    conf.clamp