Merge pull request #1857 from Jesus/master

Avoid blocking on SSL's `#read_nonblock`

lib/puma/minissl.rb

@@ -54,22 +54,21 @@ module Puma
           output = engine_read_all
           return output if output
 
-          begin
           data = @socket.read_nonblock(size, exception: false)
           if data == :wait_readable || data == :wait_writable
-              if @socket.to_io.respond_to?(data)
+            # It would make more sense to let @socket.read_nonblock raise
-                @socket.to_io.__send__(data)
+            # EAGAIN if necessary but it seems like it'll misbehave on Windows.
-              elsif data == :wait_readable
+            # I don't have a Windows machine to debug this so I can't explain
-                IO.select([@socket.to_io])
+            # exactly whats happening in that OS. Please let me know if you
-              else
+            # find out!
-                IO.select(nil, [@socket.to_io])
+            #
-              end
+            # In the meantime, we can emulate the correct behavior by
-            elsif !data
+            # capturing :wait_readable & :wait_writable and raising EAGAIN
+            # ourselves.
+            raise IO::EAGAINWaitReadable
+          elsif data.nil?
             return nil
-            else
-              break
           end
-          end while true
 
           @engine.inject(data)
           output = engine_read_all

test/test_puma_server_ssl.rb

@@ -34,8 +34,8 @@ class TestPumaServerSSL < Minitest::Test
 
   def setup
     return if DISABLE_SSL
-    port = UniquePort.call
+    @port = UniquePort.call
-    host = "127.0.0.1"
+    @host = "127.0.0.1"
 
     app = lambda { |env| [200, {}, [env['rack.url_scheme']]] }
 
@@ -53,10 +53,10 @@ class TestPumaServerSSL < Minitest::Test
 
     @events = SSLEventsHelper.new STDOUT, STDERR
     @server = Puma::Server.new app, @events
-    @ssl_listener = @server.add_ssl_listener host, port, ctx
+    @ssl_listener = @server.add_ssl_listener @host, @port, ctx
     @server.run
 
-    @http = Net::HTTP.new host, port
+    @http = Net::HTTP.new @host, @port
     @http.use_ssl = true
     @http.verify_mode = OpenSSL::SSL::VERIFY_NONE
   end
@@ -80,6 +80,25 @@ class TestPumaServerSSL < Minitest::Test
     assert_equal "https", body
   end
 
+  def test_request_wont_block_thread
+    # Open a connection and give enough data to trigger a read, then wait
+    ctx = OpenSSL::SSL::SSLContext.new
+    ctx.verify_mode = OpenSSL::SSL::VERIFY_NONE
+    socket = OpenSSL::SSL::SSLSocket.new TCPSocket.new(@host, @port), ctx
+    socket.write "x"
+    sleep 0.1
+
+    # Capture the amount of threads being used after connecting and being idle
+    thread_pool = @server.instance_variable_get(:@thread_pool)
+    busy_threads = thread_pool.spawned - thread_pool.waiting
+
+    socket.close
+
+    # The thread pool should be empty since the request would block on read
+    # and our request should have been moved to the reactor.
+    assert busy_threads.zero?, "Our connection is monopolizing a thread"
+  end
+
   def test_very_large_return
     giant = "x" * 2056610