From 64db4f01df2f8dcde9f53a73cf660255e7866a1f Mon Sep 17 00:00:00 2001
From: Will Jordan <will@code.org>
Date: Tue, 14 Apr 2020 22:29:48 -0700
Subject: [PATCH] Use mutex for out of band hook Prevent overlapping requests
 from being processed during out of band hook.

---
 lib/puma/const.rb        |  1 -
 lib/puma/server.rb       | 15 ++++++--------
 test/test_puma_server.rb | 44 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/lib/puma/const.rb b/lib/puma/const.rb
index 9d4aa793fb..c000d32659 100644
--- a/lib/puma/const.rb
+++ b/lib/puma/const.rb
@@ -186,7 +186,6 @@ module Const
     STOP_COMMAND = "?".freeze
     HALT_COMMAND = "!".freeze
     RESTART_COMMAND = "R".freeze
-    OUT_OF_BAND_COMMAND = "O".freeze
 
     RACK_INPUT = "rack.input".freeze
     RACK_URL_SCHEME = "rack.url_scheme".freeze
diff --git a/lib/puma/server.rb b/lib/puma/server.rb
index 145674debd..b5269f0ddd 100644
--- a/lib/puma/server.rb
+++ b/lib/puma/server.rb
@@ -224,12 +224,6 @@ def run(background=true)
             @reactor.add client
           end
         end
-
-        if @options[:out_of_band]
-          @thread_pool.with_mutex do
-            notify_safely(OUT_OF_BAND_COMMAND) if @thread_pool.busy_threads == 1
-          end
-        end
       end
 
       @thread_pool.clean_thread_locals = @options[:clean_thread_locals]
@@ -346,9 +340,6 @@ def handle_check
       when RESTART_COMMAND
         @status = :restart
         return true
-      when OUT_OF_BAND_COMMAND
-        @options[:out_of_band].each(&:call) if @options[:out_of_band]
-        return false
       end
 
       return false
@@ -445,6 +436,12 @@ def process_client(client, buffer)
         rescue StandardError => e
           @events.unknown_error self, e, "Client"
         end
+
+        if @options[:out_of_band]
+          @thread_pool.with_mutex do
+            @options[:out_of_band].each(&:call) if @thread_pool.busy_threads == 1
+          end
+        end
       end
     end
 
diff --git a/test/test_puma_server.rb b/test/test_puma_server.rb
index 6656363729..944a95af83 100644
--- a/test/test_puma_server.rb
+++ b/test/test_puma_server.rb
@@ -989,13 +989,28 @@ def oob_server(**options)
     @request_count = 0
     @oob_count = 0
     @start = Time.now
+    in_oob = Mutex.new
+    @mutex = Mutex.new
     @oob_finished = ConditionVariable.new
-    oob = -> {@oob_count += 1; @oob_finished.signal}
+    oob_wait = options.delete(:oob_wait)
+    oob = -> do
+puts 'oob'
+      in_oob.synchronize do
+        @mutex.synchronize do
+puts 'oob enter'
+          @oob_count += 1
+          @oob_finished.signal
+          @oob_finished.wait(@mutex, 1) if oob_wait
+puts 'oob 2'
+        end
+      end
+puts 'oob exit'
+    end
     @server = Puma::Server.new @app, @events, out_of_band: [oob], **options
     @server.min_threads = 5
     @server.max_threads = 5
-    @mutex = Mutex.new
     server_run app: ->(_) do
+      raise 'OOB conflict' if in_oob.locked?
       @mutex.synchronize {@request_count += 1}
       [200, {}, [""]]
     end
@@ -1007,7 +1022,7 @@ def test_out_of_band
     oob_server queue_requests: false
     n.times do
       @mutex.synchronize do
-        send_http "HEAD / HTTP/1.0\r\n\r\n"
+        send_http "GET / HTTP/1.0\r\n\r\n"
         @oob_finished.wait(@mutex, 1)
       end
     end
@@ -1021,7 +1036,7 @@ def test_out_of_band_stream
     n = 100
     threads = 10
     oob_server
-    req = "HEAD / HTTP/1.1\r\n"
+    req = "GET / HTTP/1.1\r\n"
     @mutex.synchronize do
       Array.new(threads) do
         Thread.new do
@@ -1033,4 +1048,25 @@ def test_out_of_band_stream
     assert_equal n, @request_count
     assert_equal 1, @oob_count
   end
+
+  def test_out_of_band_overlapping_requests
+    oob_server oob_wait: true
+    sock = nil
+    sock2 = nil
+    @mutex.synchronize do
+      sock2 = send_http "GET / HTTP/1.0\r\n"
+      sleep 0.01
+      sock = send_http "GET / HTTP/1.0\r\n\r\n"
+      # Request 1 processed
+      @oob_finished.wait(@mutex) # enter oob
+      sock2 << "\r\n"
+      sleep 0.01
+      @oob_finished.signal # exit oob
+      # Request 2 processed
+      @oob_finished.wait(@mutex) # enter oob
+      @oob_finished.signal # exit oob
+    end
+    assert_match(/200/, sock.read)
+    assert_match(/200/, sock2.read)
+  end
 end