Empty reactor on server shutdown

puma · Feb 20, 2020 · b379b8a · b379b8a
1 parent 3304499
commit b379b8a
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 6 deletions.
diff --git a/lib/puma/client.rb b/lib/puma/client.rb
@@ -273,6 +273,17 @@ def peerip
       @peerip ||= @io.peeraddr.last
     end
 
+    # Returns true if the connection is idle and may be closed.
+    #
+    # From RFC 2616 section 8.1.4:
+    # Servers SHOULD always respond to at least one request per connection,
+    # if at all possible. Servers SHOULD NOT close a connection in the
+    # middle of transmitting a response, unless a network or client failure
+    # is suspected.
+    def idle?
+      @requests_served > 0 && !in_data_phase
+    end
+
     private
 
     def setup_body

diff --git a/lib/puma/reactor.rb b/lib/puma/reactor.rb
@@ -189,7 +189,12 @@ def run_internal
                     if submon.value == @ready
                       false
                     else
-                      submon.value.close
+                      if submon.value.idle?
+                        submon.value.close
+                      else
+                        # Pass non-idle client connections to the thread pool.
+                        @app_pool << submon.value
+                      end
                       begin
                         selector.deregister submon.value
                       rescue IOError

diff --git a/lib/puma/server.rb b/lib/puma/server.rb
@@ -293,8 +293,6 @@ def run(background=true)
         return run_lopez_mode(background)
       end
 
-      queue_requests = @queue_requests
-
       @thread_pool = ThreadPool.new(@min_threads,
                                     @max_threads,
                                     IOBuffer) do |client, buffer|
@@ -305,7 +303,7 @@ def run(background=true)
         process_now = false
 
         begin
-          if queue_requests
+          if @queue_requests
             process_now = client.eagerly_finish
           else
             client.finish(@first_data_timeout)
@@ -338,7 +336,7 @@ def run(background=true)
 
       @thread_pool.clean_thread_locals = @options[:clean_thread_locals]
 
-      if queue_requests
+      if @queue_requests
         @reactor = Reactor.new self, @thread_pool
         @reactor.run_in_thread
       end
@@ -422,11 +420,12 @@ def handle_servers
 
         @events.fire :state, @status
 
-        graceful_shutdown if @status == :stop || @status == :restart
         if queue_requests
+          @queue_requests = false
           @reactor.clear!
           @reactor.shutdown
         end
+        graceful_shutdown if @status == :stop || @status == :restart
       rescue Exception => e
         STDERR.puts "Exception handling servers: #{e.message} (#{e.class})"
         STDERR.puts e.backtrace
@@ -497,6 +496,7 @@ def process_client(client, buffer)
             end
 
             unless client.reset(check_for_more_data)
+              return unless @queue_requests
               close_socket = false
               client.set_timeout @persistent_timeout
               @reactor.add client

diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb
@@ -767,4 +767,23 @@ def test_open_connection_wait_no_queue
     @server = Puma::Server.new @app, @events, queue_requests: false
     test_open_connection_wait
   end
+
+  def test_empty_reactor_on_shutdown
+    server_run app: ->(env) {
+      sleep 3
+      [204, {}, []]
+    }
+
+    s1 = send_http "GET / HTTP/1.1\r\n\r\n"
+    s2 = send_http "GET / HTTP/1.1\r\n"
+    sleep 1
+
+    @server.stop
+    sleep 1
+
+    s2 << "\r\n"
+
+    assert_match /204/, s1.gets
+    assert_match /204/, s2.gets
+  end
 end