From b652fa51c1342496bdcdecca8e567f1fb46c41c9 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Tue, 5 Sep 2023 06:43:20 +0000 Subject: kill off remaining kgio uses kgio is an extra download and shared object which costs users bandwidth, disk space, startup time and memory. Ruby 2.3+ provides `Socket#accept_nonblock(exception: false)' support in addition to `exception: false' support in IO#*_nonblock methods from Ruby 2.1. We no longer distinguish between TCPServer and UNIXServer as separate classes internally; instead favoring the `Socket' class of Ruby for both. This allows us to use `Socket#accept_nonblock' and get a populated `Addrinfo' object off accept4(2)/accept(2) without resorting to a getpeername(2) syscall (kgio avoided getpeername(2) in the same way). The downside is there's more Ruby-level argument passing and stack usage on our end with HttpRequest#read_headers (formerly HttpRequest#read). I chose this tradeoff since advancements in Ruby itself can theoretically mitigate the cost of argument passing, while syscalls are a high fixed cost given modern CPU vulnerability mitigations. Note: no benchmarks have been run since I don't have a suitable system. --- test/unit/test_request.rb | 47 ++++++++++----------------- test/unit/test_socket_helper.rb | 72 +++++++++-------------------------------- test/unit/test_stream_input.rb | 2 +- test/unit/test_tee_input.rb | 2 +- 4 files changed, 34 insertions(+), 89 deletions(-) (limited to 'test') diff --git a/test/unit/test_request.rb b/test/unit/test_request.rb index 7f22b24..53ae944 100644 --- a/test/unit/test_request.rb +++ b/test/unit/test_request.rb @@ -10,14 +10,9 @@ include Unicorn class RequestTest < Test::Unit::TestCase - class MockRequest < StringIO - alias_method :readpartial, :sysread - alias_method :kgio_read!, :sysread - alias_method :read_nonblock, :sysread - def kgio_addr - '127.0.0.1' - end - end + MockRequest = Class.new(StringIO) + + AI = Addrinfo.new(Socket.sockaddr_un('/unicorn/sucks')) def setup @request = HttpRequest.new @@ -30,7 +25,7 @@ class RequestTest < Test::Unit::TestCase def test_options client = MockRequest.new("OPTIONS * HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal '', env['REQUEST_PATH'] assert_equal '', env['PATH_INFO'] assert_equal '*', env['REQUEST_URI'] @@ -40,7 +35,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_query client = MockRequest.new("GET http://e:3/x?y=z HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal 'y=z', env['QUERY_STRING'] @@ -50,7 +45,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_fragment client = MockRequest.new("GET http://e:3/x#frag HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal '', env['QUERY_STRING'] @@ -61,7 +56,7 @@ class RequestTest < Test::Unit::TestCase def test_absolute_uri_with_query_and_fragment client = MockRequest.new("GET http://e:3/x?a=b#frag HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal '/x', env['REQUEST_PATH'] assert_equal '/x', env['PATH_INFO'] assert_equal 'a=b', env['QUERY_STRING'] @@ -73,7 +68,7 @@ class RequestTest < Test::Unit::TestCase %w(ssh+http://e/ ftp://e/x http+ssh://e/x).each do |abs_uri| client = MockRequest.new("GET #{abs_uri} HTTP/1.1\r\n" \ "Host: foo\r\n\r\n") - assert_raises(HttpParserError) { @request.read(client) } + assert_raises(HttpParserError) { @request.read_headers(client, AI) } end end @@ -81,7 +76,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: https\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal "https", env['rack.url_scheme'] assert_kind_of Array, @lint.call(env) end @@ -90,7 +85,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: http\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal "http", env['rack.url_scheme'] assert_kind_of Array, @lint.call(env) end @@ -99,14 +94,14 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("GET / HTTP/1.1\r\n" \ "X-Forwarded-Proto: ftp\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal "http", env['rack.url_scheme'] assert_kind_of Array, @lint.call(env) end def test_rack_lint_get client = MockRequest.new("GET / HTTP/1.1\r\nHost: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal "http", env['rack.url_scheme'] assert_equal '127.0.0.1', env['REMOTE_ADDR'] assert_kind_of Array, @lint.call(env) @@ -114,7 +109,7 @@ class RequestTest < Test::Unit::TestCase def test_no_content_stringio client = MockRequest.new("GET / HTTP/1.1\r\nHost: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal StringIO, env['rack.input'].class end @@ -122,7 +117,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("PUT / HTTP/1.1\r\n" \ "Content-Length: 0\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal StringIO, env['rack.input'].class end @@ -130,7 +125,7 @@ class RequestTest < Test::Unit::TestCase client = MockRequest.new("PUT / HTTP/1.1\r\n" \ "Content-Length: 1\r\n" \ "Host: foo\r\n\r\n") - env = @request.read(client) + env = @request.read_headers(client, AI) assert_equal Unicorn::TeeInput, env['rack.input'].class end @@ -141,7 +136,7 @@ class RequestTest < Test::Unit::TestCase "Content-Length: 5\r\n" \ "\r\n" \ "abcde") - env = @request.read(client) + env = @request.read_headers(client, AI) assert ! env.include?(:http_body) assert_kind_of Array, @lint.call(env) end @@ -152,14 +147,6 @@ class RequestTest < Test::Unit::TestCase buf = (' ' * bs).freeze length = bs * count client = Tempfile.new('big_put') - def client.kgio_addr; '127.0.0.1'; end - def client.kgio_read(*args) - readpartial(*args) - rescue EOFError - end - def client.kgio_read!(*args) - readpartial(*args) - end client.syswrite( "PUT / HTTP/1.1\r\n" \ "Host: foo\r\n" \ @@ -167,7 +154,7 @@ class RequestTest < Test::Unit::TestCase "\r\n") count.times { assert_equal bs, client.syswrite(buf) } assert_equal 0, client.sysseek(0) - env = @request.read(client) + env = @request.read_headers(client, AI) assert ! env.include?(:http_body) assert_equal length, env['rack.input'].size count.times { diff --git a/test/unit/test_socket_helper.rb b/test/unit/test_socket_helper.rb index 62d5a3a..a446f06 100644 --- a/test/unit/test_socket_helper.rb +++ b/test/unit/test_socket_helper.rb @@ -24,7 +24,8 @@ class TestSocketHelper < Test::Unit::TestCase port = unused_port @test_addr @tcp_listener_name = "#@test_addr:#{port}" @tcp_listener = bind_listen(@tcp_listener_name) - assert TCPServer === @tcp_listener + assert Socket === @tcp_listener + assert @tcp_listener.local_address.ip? assert_equal @tcp_listener_name, sock_name(@tcp_listener) end @@ -38,10 +39,10 @@ class TestSocketHelper < Test::Unit::TestCase { :backlog => 16, :rcvbuf => 4096, :sndbuf => 4096 } ].each do |opts| tcp_listener = bind_listen(tcp_listener_name, opts) - assert TCPServer === tcp_listener + assert tcp_listener.local_address.ip? tcp_listener.close unix_listener = bind_listen(unix_listener_name, opts) - assert UNIXServer === unix_listener + assert unix_listener.local_address.unix? unix_listener.close end end @@ -52,11 +53,13 @@ class TestSocketHelper < Test::Unit::TestCase @unix_listener_path = tmp.path File.unlink(@unix_listener_path) @unix_listener = bind_listen(@unix_listener_path) - assert UNIXServer === @unix_listener + assert Socket === @unix_listener + assert @unix_listener.local_address.unix? assert_equal @unix_listener_path, sock_name(@unix_listener) assert File.readable?(@unix_listener_path), "not readable" assert File.writable?(@unix_listener_path), "not writable" assert_equal 0777, File.umask + assert_equal @unix_listener, bind_listen(@unix_listener) ensure File.umask(old_umask) end @@ -67,7 +70,6 @@ class TestSocketHelper < Test::Unit::TestCase @unix_listener_path = tmp.path File.unlink(@unix_listener_path) @unix_listener = bind_listen(@unix_listener_path, :umask => 077) - assert UNIXServer === @unix_listener assert_equal @unix_listener_path, sock_name(@unix_listener) assert_equal 0140700, File.stat(@unix_listener_path).mode assert_equal 0777, File.umask @@ -75,28 +77,6 @@ class TestSocketHelper < Test::Unit::TestCase File.umask(old_umask) end - def test_bind_listen_unix_idempotent - test_bind_listen_unix - a = bind_listen(@unix_listener) - assert_equal a.fileno, @unix_listener.fileno - unix_server = server_cast(@unix_listener) - assert UNIXServer === unix_server - a = bind_listen(unix_server) - assert_equal a.fileno, unix_server.fileno - assert_equal a.fileno, @unix_listener.fileno - end - - def test_bind_listen_tcp_idempotent - test_bind_listen_tcp - a = bind_listen(@tcp_listener) - assert_equal a.fileno, @tcp_listener.fileno - tcp_server = server_cast(@tcp_listener) - assert TCPServer === tcp_server - a = bind_listen(tcp_server) - assert_equal a.fileno, tcp_server.fileno - assert_equal a.fileno, @tcp_listener.fileno - end - def test_bind_listen_unix_rebind test_bind_listen_unix new_listener = nil @@ -107,14 +87,18 @@ class TestSocketHelper < Test::Unit::TestCase File.unlink(@unix_listener_path) new_listener = bind_listen(@unix_listener_path) - assert UNIXServer === new_listener assert new_listener.fileno != @unix_listener.fileno assert_equal sock_name(new_listener), sock_name(@unix_listener) assert_equal @unix_listener_path, sock_name(new_listener) pid = fork do - client = server_cast(new_listener).accept - client.syswrite('abcde') - exit 0 + begin + client, _ = new_listener.accept + client.syswrite('abcde') + exit 0 + rescue => e + warn "#{e.message} (#{e.class})" + exit 1 + end end s = unix_socket(@unix_listener_path) IO.select([s]) @@ -123,32 +107,6 @@ class TestSocketHelper < Test::Unit::TestCase assert status.success? end - def test_server_cast - test_bind_listen_unix - test_bind_listen_tcp - unix_listener_socket = Socket.for_fd(@unix_listener.fileno) - assert Socket === unix_listener_socket - @unix_server = server_cast(unix_listener_socket) - assert_equal @unix_listener.fileno, @unix_server.fileno - assert UNIXServer === @unix_server - assert_equal(@unix_server.path, @unix_listener.path, - "##{@unix_server.path} != #{@unix_listener.path}") - assert File.socket?(@unix_server.path) - assert_equal @unix_listener_path, sock_name(@unix_server) - - tcp_listener_socket = Socket.for_fd(@tcp_listener.fileno) - assert Socket === tcp_listener_socket - @tcp_server = server_cast(tcp_listener_socket) - assert_equal @tcp_listener.fileno, @tcp_server.fileno - assert TCPServer === @tcp_server - assert_equal @tcp_listener_name, sock_name(@tcp_server) - end - - def test_sock_name - test_server_cast - sock_name(@unix_server) - end - def test_tcp_defer_accept_default return unless defined?(TCP_DEFER_ACCEPT) port = unused_port @test_addr diff --git a/test/unit/test_stream_input.rb b/test/unit/test_stream_input.rb index 2a14135..7986ca7 100644 --- a/test/unit/test_stream_input.rb +++ b/test/unit/test_stream_input.rb @@ -9,7 +9,7 @@ class TestStreamInput < Test::Unit::TestCase @rs = "\n" $/ == "\n" or abort %q{test broken if \$/ != "\\n"} @env = {} - @rd, @wr = Kgio::UNIXSocket.pair + @rd, @wr = UNIXSocket.pair @rd.sync = @wr.sync = true @start_pid = $$ end diff --git a/test/unit/test_tee_input.rb b/test/unit/test_tee_input.rb index 6f5bc8a..607ce87 100644 --- a/test/unit/test_tee_input.rb +++ b/test/unit/test_tee_input.rb @@ -10,7 +10,7 @@ end class TestTeeInput < Test::Unit::TestCase def setup - @rd, @wr = Kgio::UNIXSocket.pair + @rd, @wr = UNIXSocket.pair @rd.sync = @wr.sync = true @start_pid = $$ @rs = "\n" -- cgit v1.2.3-24-ge0c7