From fceabeb148a5d82768241f258d42bc8fdde09757 Mon Sep 17 00:00:00 2001 From: Dmytro Shteflyuk Date: Mon, 18 May 2026 18:03:06 -0400 Subject: [PATCH] THRIFT-5949: Add default timeout to Ruby server sockets --- lib/rb/README.md | 7 ++++ .../thrift/transport/base_server_transport.rb | 2 + lib/rb/lib/thrift/transport/server_socket.rb | 8 ++-- .../lib/thrift/transport/ssl_server_socket.rb | 4 +- .../thrift/transport/unix_server_socket.rb | 5 ++- lib/rb/spec/server_socket_spec.rb | 36 +++++++++++++++++ lib/rb/spec/ssl_server_socket_spec.rb | 39 +++++++++++++++++++ lib/rb/spec/unix_socket_spec.rb | 15 +++++++ 8 files changed, 110 insertions(+), 6 deletions(-) diff --git a/lib/rb/README.md b/lib/rb/README.md index 9d07f454e6f..f241a4e7e94 100644 --- a/lib/rb/README.md +++ b/lib/rb/README.md @@ -141,6 +141,13 @@ to handle `TIMED_OUT`. If you relied on `timeout = 0` meaning immediate failure or on repeated retries extending the effective timeout during TCP fallback or TLS handshake, update those call paths before upgrading. +Ruby server socket transports now apply a 5-second timeout to accepted client +sockets by default. This prevents stalled clients from blocking server threads +indefinitely during response writes. Applications that intentionally require +blocking accepted sockets can pass `client_timeout: nil` or `client_timeout: 0` +when constructing `Thrift::ServerSocket`, `Thrift::SSLServerSocket`, or +`Thrift::UNIXServerSocket`. + Generated Ruby structs and unions now suffix field ID constants as `*_FIELD_ID` instead of exposing bare uppercased field names. For example, `MyStruct::FOO` becomes `MyStruct::FOO_FIELD_ID`. This avoids collisions with diff --git a/lib/rb/lib/thrift/transport/base_server_transport.rb b/lib/rb/lib/thrift/transport/base_server_transport.rb index 196ec9da9e8..45ed75867ca 100644 --- a/lib/rb/lib/thrift/transport/base_server_transport.rb +++ b/lib/rb/lib/thrift/transport/base_server_transport.rb @@ -21,6 +21,8 @@ module Thrift class BaseServerTransport + DEFAULT_CLIENT_TIMEOUT = 5 + def listen raise NotImplementedError end diff --git a/lib/rb/lib/thrift/transport/server_socket.rb b/lib/rb/lib/thrift/transport/server_socket.rb index ed5fe59c402..81b04439d59 100644 --- a/lib/rb/lib/thrift/transport/server_socket.rb +++ b/lib/rb/lib/thrift/transport/server_socket.rb @@ -23,8 +23,8 @@ module Thrift class ServerSocket < BaseServerTransport - # call-seq: initialize(host = nil, port) - def initialize(host_or_port, port = nil) + # call-seq: initialize(host = nil, port, client_timeout: DEFAULT_CLIENT_TIMEOUT) + def initialize(host_or_port, port = nil, client_timeout: DEFAULT_CLIENT_TIMEOUT) if port @host = host_or_port @port = port @@ -32,10 +32,11 @@ def initialize(host_or_port, port = nil) @host = nil @port = host_or_port end + @client_timeout = client_timeout @handle = nil end - attr_reader :handle + attr_reader :handle, :client_timeout def listen @handle = TCPServer.new(@host, @port) @@ -46,6 +47,7 @@ def accept sock = @handle.accept sock.setsockopt(::Socket::IPPROTO_TCP, ::Socket::TCP_NODELAY, 1) trans = Socket.new + trans.timeout = @client_timeout trans.handle = sock trans end diff --git a/lib/rb/lib/thrift/transport/ssl_server_socket.rb b/lib/rb/lib/thrift/transport/ssl_server_socket.rb index ef12c73ae78..2ccb9bf40ff 100644 --- a/lib/rb/lib/thrift/transport/ssl_server_socket.rb +++ b/lib/rb/lib/thrift/transport/ssl_server_socket.rb @@ -23,8 +23,8 @@ module Thrift class SSLServerSocket < ServerSocket - def initialize(host_or_port, port = nil, ssl_context = nil) - super(host_or_port, port) + def initialize(host_or_port, port = nil, ssl_context = nil, client_timeout: DEFAULT_CLIENT_TIMEOUT) + super(host_or_port, port, client_timeout: client_timeout) @ssl_context = ssl_context end diff --git a/lib/rb/lib/thrift/transport/unix_server_socket.rb b/lib/rb/lib/thrift/transport/unix_server_socket.rb index 1e0953d117f..adeb50c450f 100644 --- a/lib/rb/lib/thrift/transport/unix_server_socket.rb +++ b/lib/rb/lib/thrift/transport/unix_server_socket.rb @@ -23,12 +23,14 @@ module Thrift class UNIXServerSocket < BaseServerTransport - def initialize(path) + def initialize(path, client_timeout: DEFAULT_CLIENT_TIMEOUT) @path = path + @client_timeout = client_timeout @handle = nil end attr_accessor :handle + attr_reader :client_timeout def listen @handle = ::UNIXServer.new(@path) @@ -38,6 +40,7 @@ def accept unless @handle.nil? sock = @handle.accept trans = UNIXSocket.new(nil) + trans.timeout = @client_timeout trans.handle = sock trans end diff --git a/lib/rb/spec/server_socket_spec.rb b/lib/rb/spec/server_socket_spec.rb index 5a793e1075e..ee31b5706ae 100644 --- a/lib/rb/spec/server_socket_spec.rb +++ b/lib/rb/spec/server_socket_spec.rb @@ -48,10 +48,46 @@ expect(handle).to receive(:accept).and_return(sock) trans = double("Socket") expect(Thrift::Socket).to receive(:new).and_return(trans) + expect(trans).to receive(:timeout=).with(Thrift::BaseServerTransport::DEFAULT_CLIENT_TIMEOUT) expect(trans).to receive(:handle=).with(sock) expect(@socket.accept).to eq(trans) end + it "should default accepted sockets to a finite client timeout" do + expect(@socket.client_timeout).to eq(5) + end + + it "should accept a custom client timeout" do + @socket = Thrift::ServerSocket.new(1234, client_timeout: 2.5) + expect(@socket.client_timeout).to eq(2.5) + end + + it "should allow blocking accepted sockets with nil or zero client timeout" do + expect(Thrift::ServerSocket.new(1234, client_timeout: nil).client_timeout).to be_nil + expect(Thrift::ServerSocket.new(1234, client_timeout: 0).client_timeout).to eq(0) + end + + it "should use the accepted socket timeout for writes" do + handle = double("TCPServer") + allow(TCPServer).to receive(:new).with(nil, 1234).and_return(handle) + @socket.listen + + sock = double("sock", :closed? => false) + allow(sock).to receive(:setsockopt) + allow(handle).to receive(:accept).and_return(sock) + + transport = @socket.accept + expect(transport.timeout).to eq(Thrift::BaseServerTransport::DEFAULT_CLIENT_TIMEOUT) + + expect(sock).to receive(:write_nonblock).with("test data").and_raise(IO::EAGAINWaitWritable) + expect(Process).to receive(:clock_gettime).with(Process::CLOCK_MONOTONIC).and_return(100.0, 100.0, 105.1) + expect(IO).to receive(:select).with(nil, [sock], nil, 5.0).and_return(nil) + + expect { transport.write("test data") }.to raise_error(Thrift::TransportException) do |e| + expect(e.type).to eq(Thrift::TransportException::TIMED_OUT) + end + end + it "should close the handle when closed" do handle = double("TCPServer", :closed? => false) expect(TCPServer).to receive(:new).with(nil, 1234).and_return(handle) diff --git a/lib/rb/spec/ssl_server_socket_spec.rb b/lib/rb/spec/ssl_server_socket_spec.rb index 18bce537809..b3d0b9a5d13 100644 --- a/lib/rb/spec/ssl_server_socket_spec.rb +++ b/lib/rb/spec/ssl_server_socket_spec.rb @@ -39,6 +39,45 @@ expect(@socket.to_io).to eq(tcp_server) end + it "should default accepted sockets to a finite client timeout" do + expect(@socket.client_timeout).to eq(5) + end + + it "should preserve the positional ssl context argument" do + context = double("SSLContext") + socket = Thrift::SSLServerSocket.new(nil, 1234, context) + + expect(socket.ssl_context).to eq(context) + expect(socket.client_timeout).to eq(5) + end + + it "should accept a custom client timeout" do + context = double("SSLContext") + socket = Thrift::SSLServerSocket.new(nil, 1234, context, client_timeout: 2.5) + + expect(socket.ssl_context).to eq(context) + expect(socket.client_timeout).to eq(2.5) + end + + it "should apply the client timeout to accepted sockets" do + tcp_server = double("TCPServer") + ssl_server = double("SSLServer") + sock = double("SSLSocket") + + allow(TCPServer).to receive(:new).with(nil, 1234).and_return(tcp_server) + allow(OpenSSL::SSL::SSLServer).to receive(:new).with(tcp_server, nil).and_return(ssl_server) + expect(ssl_server).to receive(:accept).and_return(sock) + expect(sock).to receive(:setsockopt).with(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) + + trans = double("Socket") + expect(Thrift::Socket).to receive(:new).and_return(trans) + expect(trans).to receive(:timeout=).with(Thrift::BaseServerTransport::DEFAULT_CLIENT_TIMEOUT) + expect(trans).to receive(:handle=).with(sock) + + @socket.listen + expect(@socket.accept).to eq(trans) + end + it "should provide a reasonable to_s" do expect(@socket.to_s).to eq("ssl(socket(:1234))") end diff --git a/lib/rb/spec/unix_socket_spec.rb b/lib/rb/spec/unix_socket_spec.rb index 9e395f70382..29cb2d4e479 100644 --- a/lib/rb/spec/unix_socket_spec.rb +++ b/lib/rb/spec/unix_socket_spec.rb @@ -68,10 +68,25 @@ expect(handle).to receive(:accept).and_return(sock) trans = double("UNIXSocket") expect(Thrift::UNIXSocket).to receive(:new).and_return(trans) + expect(trans).to receive(:timeout=).with(Thrift::BaseServerTransport::DEFAULT_CLIENT_TIMEOUT) expect(trans).to receive(:handle=).with(sock) expect(@socket.accept).to eq(trans) end + it "should default accepted sockets to a finite client timeout" do + expect(@socket.client_timeout).to eq(5) + end + + it "should accept a custom client timeout" do + @socket = Thrift::UNIXServerSocket.new(@path, client_timeout: 2.5) + expect(@socket.client_timeout).to eq(2.5) + end + + it "should allow blocking accepted sockets with nil or zero client timeout" do + expect(Thrift::UNIXServerSocket.new(@path, client_timeout: nil).client_timeout).to be_nil + expect(Thrift::UNIXServerSocket.new(@path, client_timeout: 0).client_timeout).to eq(0) + end + it "should close the handle when closed" do handle = double("UNIXServer", :closed? => false) expect(UNIXServer).to receive(:new).with(@path).and_return(handle)