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

Code before:
# frozen_string_literal: true

require 'uri'
require 'socket'

require 'puma/const'
require 'puma/util'
require 'puma/configuration'

module Puma

  if HAS_SSL
    require 'puma/minissl'
    require 'puma/minissl/context_builder'

    # odd bug in 'pure Ruby' nio4r only with 2.5.2.  Fixed in newer version,
    # didn't exist in older versions.
    # Doesn't really need the OpenSSL code, but it checks for an OpenSSL error.
    # @todo remove when 2.3 support is dropped
    #
    if windows? && RbConfig::CONFIG['ruby_version'] == '2.3.0'
      require 'openssl'
    end
  end

  class Binder
    include Puma::Const

    RACK_VERSION = [1,6].freeze

    def initialize(events, conf = Configuration.new)
      @events = events
      @listeners = []
      @inherited_fds = {}
      @activated_sockets = {}
      @unix_paths = []

      @proto_env = {
        "rack.version".freeze => RACK_VERSION,
        "rack.errors".freeze => events.stderr,
        "rack.multithread".freeze => conf.options[:max_threads] > 1,

Review comment (bug):
```suggestion
    # odd bug in 'pure Ruby' nio4r v2.5.2 (fixed in newer versions),
```

To make it a bit more clear what software 2.5.2 refers to.

I assume https://github.com/socketry/nio4r/issues/238 is the issue you are referring to, should we include that URL in the comment?

Code after:
# frozen_string_literal: true

require 'uri'
require 'socket'

require 'puma/const'
require 'puma/util'
require 'puma/configuration'

module Puma

  if HAS_SSL
    require 'puma/minissl'
    require 'puma/minissl/context_builder'

    # Odd bug in 'pure Ruby' nio4r verion 2.5.2, which installs with Ruby 2.3.
    # NIO doesn't create any OpenSSL objects, but it rescues an OpenSSL error.
    # The bug was that it did not require openssl.
    # @todo remove when Ruby 2.3 support is dropped
    #
    if windows? && RbConfig::CONFIG['ruby_version'] == '2.3.0'
      require 'openssl'
    end
  end

  class Binder
    include Puma::Const

    RACK_VERSION = [1,6].freeze

    def initialize(events, conf = Configuration.new)
      @events = events
      @listeners = []
      @inherited_fds = {}
      @activated_sockets = {}
      @unix_paths = []

      @proto_env = {
        "rack.version".freeze => RACK_VERSION,
        "rack.errors".freeze => events.stderr,
        "rack.multithread".freeze => conf.options[:max_threads] > 1,
Repository: puma/puma
File: lib/puma/launcher.rb (Ruby)

Code before:
        # Not going to log this one, as SIGINFO is *BSD only and would be pretty annoying
        # to see this constantly on Linux.
      end
    end

    def require_rubygems_min_version!(min_version, feature)
      return if min_version <= Gem::Version.new(Gem::VERSION)

      raise "#{feature} is not supported on your version of RubyGems. " \
              "You must have RubyGems #{min_version}+ to use this feature."
    end

    # @version 5.0.0
    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 log_config
      log "Configuration:"

      @config.options.default_options
        .merge(@config.options.file_options)
        .merge(@config.options.user_options)
        .each { |config_key, value| log "-#{config_key}: #{value}" }

      if @config.plugins.instances.size > 0
        log "\nLoaded plugins:"
        log @config.plugins.instances
      end

      log "\n"
    end
  end
end


Review comment (refactor):
Hmmm. In practice, I'd rather that this method just called something like `@config.final_options` and then iterate through those. Then, we also use _that_ method when calculating the final config. That method may not exist yet, you may have to refactor it out and use it in both places.

Normally I wouldn't be so eager to DRY up 2 places, but in this case it's critical for correctness.

Code after:
        # Not going to log this one, as SIGINFO is *BSD only and would be pretty annoying
        # to see this constantly on Linux.
      end
    end

    def require_rubygems_min_version!(min_version, feature)
      return if min_version <= Gem::Version.new(Gem::VERSION)

      raise "#{feature} is not supported on your version of RubyGems. " \
              "You must have RubyGems #{min_version}+ to use this feature."
    end

    # @version 5.0.0
    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 log_config
      log "Configuration:"

      @config.final_options
        .each { |config_key, value| log "- #{config_key}: #{value}" }

      log "\n"
    end
  end
end
Repository: puma/puma
File: test/test_json.rb (Ruby)

Code before:
    value = 1
    ex = assert_raises Puma::JSON::SerializationError do
      Puma::JSON.generate value
    end
    assert_equal "Could not serialize object of type #{value.class}", ex.message
  end

  def test_generate_raises_error_for_unexpected_key_type
    value = { [1] => 'b' }
    ex = assert_raises Puma::JSON::SerializationError do
      Puma::JSON.generate value
    end
    assert_equal 'Could not serialize object of type Array as object key', ex.message
  end

  def test_json_generates_string_for_hash_with_symbol_keys
    value = { key: 'value' }
    assert_equal '{"key":"value"}', Puma::JSON.generate(value)
  end

  def test_json_generates_string_for_true
    value = [true]
    assert_equal '[true]', Puma::JSON.generate(value)
  end

  def test_json_generates_string_for_true
    value = [false]
    assert_equal '[false]', Puma::JSON.generate(value)
  end
end


Review comment (suggestion):
Looks like a minor copy/paste error:
```suggestion
  def test_json_generates_string_for_false
```

Code after:
    assert_puma_json_generates_string '[1,[2,[3]]]', value
  end

  def test_json_generates_string_for_integer
    value = 42
    assert_puma_json_generates_string '42', value
  end

  def test_json_generates_string_for_float
    value = 1.23
    assert_puma_json_generates_string '1.23', value
  end

  def test_json_escapes_strings_with_quotes
    value = 'a"'
    assert_puma_json_generates_string '"a\""', value
  end

  def test_json_escapes_strings_with_backslashes
    value = 'a\\'
    assert_puma_json_generates_string '"a\\\\"', value
  end

  def test_json_escapes_strings_with_null_byte
    value = "\x00"
    assert_puma_json_generates_string '"\u0000"', value
  end

  def test_json_escapes_strings_with_unicode_information_separator_one
    value = "\x1f"
    assert_puma_json_generates_string '"\u001F"', value
  end

  def test_json_generates_string_for_true
    value = true
    assert_puma_json_generates_string 'true', value
  end

  def test_json_generates_string_for_false
    value = false
    assert_puma_json_generates_string 'false', value
  end

  def test_json_generates_string_for_nil
    value = nil
    assert_puma_json_generates_string 'null', value
  end

  def test_generate_raises_error_for_unexpected_value_type
    value = /abc/
    ex = assert_raises Puma::JSON::SerializationError do
Repository: puma/puma
File: lib/puma/json.rb (Ruby)

Code before:
            serialize_object_key output, k
            output << ':'
            serialize_value output, v
          end
          output << '}'
        when String
          serialize_string output, value
        when Integer, Float
          output << value.to_s
        else
          raise SerializationError, "Unexpected value of type #{value.class}"
        end
      end

      def serialize_string(output, value)
        output << '"'
        output << value.gsub(/[\\"]/, '\\' => '\\\\', '"' => '\\"')
        output << '"'
      end

      def serialize_object_key(output, value)
        case value
        when Symbol
          serialize_string output, value.to_s
        when String
          serialize_string output, value
        else
          raise SerializationError, "Could not serialize object of type #{value.class} as object key"
        end
      end
    end
  end
end


Review comment (suggestion):
In the past I used inspect for strings and symbols.  Maybe replace `serialize_string` with `inspect`:
```ruby
when Symbol, String
  output << value.to_s.inspect
```
or 
```ruby
when Symbol
  output << value.to_s.inspect
when String
  output << value.inspect
```
I think it does all the needed processing?

**EDIT:** Just noticed the call ro `serialize_string` in `serialize_value`, that needs to be replaced also.  Just tried the test locally, and `inspect` seems fine.

Code after:
            serialize_value output, member
          end
          output << ']'
        when Integer, Float
          output << value.to_s
        when String
          serialize_string output, value
        when true
          output << 'true'
        when false
          output << 'false'
        when nil
          output << 'null'
        else
          raise SerializationError, "Unexpected value of type #{value.class}"
        end
      end

      def serialize_string(output, value)
        output << '"'
        output << value.gsub(CHAR_TO_ESCAPE) do |character|
          case character
          when BACKSLASH
            '\\\\'
          when QUOTE
            '\\"'
          when CONTROL_CHAR_TO_ESCAPE
            '\u%.4X' % character.ord
          end
        end
        output << '"'
      end

      def serialize_object_key(output, value)
        case value
        when Symbol, String
          serialize_string output, value.to_s
        else
          raise SerializationError, "Could not serialize object of type #{value.class} as object key"
        end
      end
    end
  end
end
Repository: puma/puma
File: lib/puma/minissl.rb (Ruby)

Code before:
      "SUITEB_128_LOS_ONLY"  => 0x10000,
      "SUITEB_192_LOS"       => 0x20000,
      "SUITEB_128_LOS"       => 0x30000,
      "PARTIAL_CHAIN"        => 0x80000,
      "NO_ALT_CHAINS"        => 0x100000,
      "NO_CHECK_TIME"        => 0x200000
    }.freeze

    class Server
      def initialize(socket, ctx)
        @socket = socket
        @ctx = ctx
        unless IS_JRUBY
          @sslctx = SSLContext.new ctx
        end
      end

      # @!attribute [r] to_io
      def to_io
        @socket
      end

      def accept
        @ctx.check
        io = @socket.accept
        engine = Engine.server (IS_JRUBY ? @ctx : @sslctx)

        Socket.new io, engine
      end

      def accept_nonblock
        @ctx.check
        io = @socket.accept_nonblock
        engine = Engine.server (IS_JRUBY ? @ctx : @sslctx)

        Socket.new io, engine
      end

      # @!attribute [r] addr
      # @version 5.0.0
      def addr
        @socket.addr
      end

      def close
        @socket.close unless @socket.closed?       # closed? call is for Windows
      end
    end
  end
end


Review comment (suggestion):
this engine/sslctx is pretty hot-path, so I think we should instead just have an `engine` method which only has to run this check/ternary once. memoize or just use metaprogramming, your choice.

Code after:
      "TRUSTED_FIRST"        => 0x8000,
      "SUITEB_128_LOS_ONLY"  => 0x10000,
      "SUITEB_192_LOS"       => 0x20000,
      "SUITEB_128_LOS"       => 0x30000,
      "PARTIAL_CHAIN"        => 0x80000,
      "NO_ALT_CHAINS"        => 0x100000,
      "NO_CHECK_TIME"        => 0x200000
    }.freeze

    class Server
      def initialize(socket, ctx)
        @socket = socket
        @ctx = ctx
        @eng_ctx = IS_JRUBY ? @ctx : SSLContext.new(ctx)
      end

      def accept
        @ctx.check
        io = @socket.accept
        engine = Engine.server @eng_ctx
        Socket.new io, engine
      end

      def accept_nonblock
        @ctx.check
        io = @socket.accept_nonblock
        engine = Engine.server @eng_ctx
        Socket.new io, engine
      end

      # @!attribute [r] to_io
      def to_io
        @socket
      end

      # @!attribute [r] addr
      # @version 5.0.0
      def addr
        @socket.addr
      end

      def close
        @socket.close unless @socket.closed?       # closed? call is for Windows
      end

      def closed?
        @socket.closed?
      end
    end
  end
end
Repository: puma/puma
File: lib/puma/control_cli.rb (Ruby)

Code before:
        @control_auth_token = sf.control_auth_token
        @pid = sf.pid
      elsif @pidfile
        # get pid from pid_file
        @pid = File.read(@pidfile, mode: 'rb:UTF-8').to_i
      end
    end

    def send_request
      uri = URI.parse @control_url

      # create server object by scheme
      server =
        case uri.scheme
        when 'ssl'
          require 'openssl'
          OpenSSL::SSL::SSLSocket.new(
            TCPSocket.new(uri.host, uri.port),
            OpenSSL::SSL::SSLContext.new)
            .tap { |ssl| ssl.sync_close = true }  # default is false
            .tap(&:connect)
        when 'tcp'
          TCPSocket.new uri.host, uri.port
        when 'unix'
          UNIXSocket.new(@control_url.start_with?('unix://@') ?
            "\0#{uri.host}#{uri.path}" : "#{uri.host}#{uri.path}")
        else
          raise "Invalid scheme: #{uri.scheme}"
        end

      if @command == 'status'
        message 'Puma is started'
      elsif NO_REQ_COMMANDS.include? @command
        raise "Invalid request command: #{@command}"
      else
        url = "/#{@command}"

        if @control_auth_token
          url = url + "?token=#{@control_auth_token}"
        end

        server.syswrite "GET #{url} HTTP/1.0\r\n\r\n"

        unless data = server.read
          raise 'Server closed connection before responding'
        end

        response = data.split("\r\n")

        if response.empty?
          raise "Server sent empty response"

Review comment (refactor):
Everywhere that we check for a socket starting with `@`, I think we should have a comment explaining this a check for an abstract socket.

Code after:
        @control_auth_token = sf.control_auth_token
        @pid = sf.pid
      elsif @pidfile
        # get pid from pid_file
        @pid = File.read(@pidfile, mode: 'rb:UTF-8').to_i
      end
    end

    def send_request
      uri = URI.parse @control_url

      # create server object by scheme
      server =
        case uri.scheme
        when 'ssl'
          require 'openssl'
          OpenSSL::SSL::SSLSocket.new(
            TCPSocket.new(uri.host, uri.port),
            OpenSSL::SSL::SSLContext.new)
            .tap { |ssl| ssl.sync_close = true }  # default is false
            .tap(&:connect)
        when 'tcp'
          TCPSocket.new uri.host, uri.port
        when 'unix'
          # check for abstract UNIXSocket
          UNIXSocket.new(@control_url.start_with?('unix://@') ?
            "\0#{uri.host}#{uri.path}" : "#{uri.host}#{uri.path}")
        else
          raise "Invalid scheme: #{uri.scheme}"
        end

      if @command == 'status'
        message 'Puma is started'
      elsif NO_REQ_COMMANDS.include? @command
        raise "Invalid request command: #{@command}"
      else
        url = "/#{@command}"

        if @control_auth_token
          url = url + "?token=#{@control_auth_token}"
        end

        server.syswrite "GET #{url} HTTP/1.0\r\n\r\n"

        unless data = server.read
          raise 'Server closed connection before responding'
        end

        response = data.split("\r\n")

        if response.empty?
Repository: puma/puma
File: lib/puma/binder.rb (Ruby)

Code before:
      @ios << ssl

      s
    end

    # Tell the server to listen on +path+ as a UNIX domain socket.
    #
    def add_unix_listener(path, umask=nil, mode=nil, backlog=1024)
      # Let anyone connect by default
      umask ||= 0

      begin
        old_mask = File.umask(umask)

        if File.exist? path
          begin
            old = UNIXSocket.new path
          rescue SystemCallError, IOError
            File.unlink path
          else
            old.close
            raise "There is already a server bound to: #{path}"
          end
        end

        s = UNIXServer.new path.sub(/\A@/, "\0")
        s.listen backlog
        @ios << s
      ensure
        File.umask old_mask
      end

      if mode
        File.chmod mode, path
      end

      env = @proto_env.dup
      env[REMOTE_ADDR] = "127.0.0.1"
      @envs[s] = env

      s
    end

    def inherit_unix_listener(path, fd)
      s = fd.kind_of?(::TCPServer) ? fd : ::UNIXServer.for_fd(fd)

      @ios << s

      env = @proto_env.dup
      env[REMOTE_ADDR] = "127.0.0.1"
      @envs[s] = env

Review comment (refactor):
Everywhere that we check for a socket starting with `@`, I think we should have a comment explaining this a check for an abstract socket.

Code after:
      @ios << ssl

      s
    end

    # Tell the server to listen on +path+ as a UNIX domain socket.
    #
    def add_unix_listener(path, umask=nil, mode=nil, backlog=1024)
      # Let anyone connect by default
      umask ||= 0

      begin
        old_mask = File.umask(umask)

        if File.exist? path
          begin
            old = UNIXSocket.new path
          rescue SystemCallError, IOError
            File.unlink path
          else
            old.close
            raise "There is already a server bound to: #{path}"
          end
        end
        s = UNIXServer.new path.sub(/\A@/, "\0") # check for abstract UNIXSocket
        s.listen backlog
        @ios << s
      ensure
        File.umask old_mask
      end

      if mode
        File.chmod mode, path
      end

      env = @proto_env.dup
      env[REMOTE_ADDR] = "127.0.0.1"
      @envs[s] = env

      s
    end

    def inherit_unix_listener(path, fd)
      s = fd.kind_of?(::TCPServer) ? fd : ::UNIXServer.for_fd(fd)

      @ios << s

      env = @proto_env.dup
      env[REMOTE_ADDR] = "127.0.0.1"
      @envs[s] = env
Repository: puma/puma
File: test/test_integration_systemd.rb (Ruby)

Code before:
  end

  def test_systemd_notify_usr2_hot_restart_cluster
    skip_unless :fork
    restart :USR2
  end

  def test_systemd_notify_usr2_hot_restart_single
      restart :USR2, workers: 0
  end

  def test_systemd_watchdog
    ENV["WATCHDOG_USEC"] = "1_000_000"

    cli_server "test/rackup/hello.ru"
    assert_equal(socket_message, "READY=1")

    assert_equal(socket_message, "WATCHDOG=1")

    stop_server
    assert_match(socket_message, "STOPPING=1")
  end

  private

  def restart(signal, workers: 2)
    cli_server "-w#{workers} test/rackup/hello.ru"
    assert_equal socket_message, 'READY=1'

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

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

    stop_server
    assert_equal socket_message, 'STOPPING=1'
  end

  def socket_message
    @socket.recvfrom(15)[0]
  end
end


Review comment (suggestion):
Stylistically, I think every test should end with `assert` or, rarely, a cleanup step or two. This method is basically a set of assertions, so I think it would be appropriate to call it `assert_restarts_with_systemd` or similar.

Code after:
  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
    ENV["WATCHDOG_USEC"] = "1_000_000"

    cli_server "test/rackup/hello.ru"
    assert_equal(socket_message, "READY=1")

    assert_equal(socket_message, "WATCHDOG=1")

    stop_server
    assert_match(socket_message, "STOPPING=1")
  end

  private

  def assert_restarts_with_systemd(signal, workers: 2)
    cli_server "-w#{workers} test/rackup/hello.ru"
    assert_equal socket_message, 'READY=1'

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

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

    stop_server
    assert_equal socket_message, 'STOPPING=1'
  end

  def socket_message
    @socket.recvfrom(15)[0]
  end
end
Repository: puma/puma
File: test/test_puma_server.rb (Ruby)

Code before:
  def test_GET_with_empty_body_has_sane_chunking
    server_run { [200, {}, [""]] }

    data = send_http_and_read "HEAD / HTTP/1.0\r\n\r\n"

    assert_equal "HTTP/1.0 200 OK\r\nContent-Length: 0\r\n\r\n", data
  end

  def test_early_hints_works
    server_run(early_hints: true) do |env|
     env['rack.early_hints'].call("Link" => "</style.css>; rel=preload; as=style\n</script.js>; rel=preload")
     [200, { "X-Hello" => "World" }, ["Hello world!"]]
    end

    data = send_http_and_read "HEAD / HTTP/1.0\r\n\r\n"

    expected_data = (<<EOF
HTTP/1.1 103 Early Hints
Link: </style.css>; rel=preload; as=style
Link: </script.js>; rel=preload

HTTP/1.0 200 OK
X-Hello: World
Content-Length: 12
EOF
).split("\n").join("\r\n") + "\r\n\r\n"

    assert_equal true, @server.early_hints
    assert_equal expected_data, data
  end

  def test_early_hints_are_ignored_if_connection_lost

    server_run(early_hints: true) do |env|
      env['rack.early_hints'].call("Link" => "</script.js>; rel=preload")
      [200, { "X-Hello" => "World" }, ["Hello world!"]]
    end

    def @server.fast_write(*args)
      raise Puma::ConnectionError
    end

    # This request will cause the server to try and send early hints
    _ = send_http "HEAD / HTTP/1.0\r\n\r\n"

    # Give the server some time to try to write (and fail)
    sleep 0.1

    # Expect no errors in stderr
    assert @events.stderr.pos.zero?, "Server didn't swallow the connection error"
  end

Review comment (suggestion):
Missed one (I've done similar)? Maybe:
```ruby
server_run { [200, {}, [env['SERVER_PORT']]] }
```

Code after:

  def test_respect_x_forwarded_scheme
    env = {}
    env['HOST'] = 'example.com'
    env['HTTP_X_FORWARDED_SCHEME'] = 'https'

    assert_equal '443', @server.default_server_port(env)
  end

  def test_default_server_port
    server_run do |env|
      [200, {}, [env['SERVER_PORT']]]
    end

    req = Net::HTTP::Get.new '/'
    req['HOST'] = 'example.com'

    res = Net::HTTP.start @host, @port do |http|
      http.request(req)
    end

    assert_equal "80", res.body
  end

  def test_default_server_port_respects_x_forwarded_proto
    server_run do |env|
      [200, {}, [env['SERVER_PORT']]]
    end

    req = Net::HTTP::Get.new("/")
    req['HOST'] = "example.com"
    req['X-FORWARDED-PROTO'] = "https,http"

    res = Net::HTTP.start @host, @port do |http|
      http.request(req)
    end

    assert_equal "443", res.body
  end

  def test_HEAD_has_no_body
    server_run { [200, {"Foo" => "Bar"}, ["hello"]] }

    data = send_http_and_read "HEAD / HTTP/1.0\r\n\r\n"

    assert_equal "HTTP/1.0 200 OK\r\nFoo: Bar\r\nContent-Length: 5\r\n\r\n", data
  end

  def test_GET_with_empty_body_has_sane_chunking
    server_run { [200, {}, [""]] }