THRIFT-5949: Add default timeout to Ruby server sockets

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

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

lib/rb/lib/thrift/transport/server_socket.rb

@@ -23,19 +23,20 @@
 
 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
       else
         @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

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
 

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

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)

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

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)