Restore Rack::Lock implementation

This correctly doesn't release the lock until after the body is
closed. Restore the related tests as well.  Just make changes to
avoid use of rack.multithread.

lib/rack/lock.rb

@@ -1,24 +1,27 @@
 # frozen_string_literal: true
 
 module Rack
+  # Rack::Lock locks every request inside a mutex, so that every request
+  # will effectively be executed synchronously.
   class Lock
-    def initialize(app)
-      @app = app
-      @mutex = ::Thread::Mutex.new
+    def initialize(app, mutex = Mutex.new)
+      @app, @mutex = app, mutex
     end
 
     def call(env)
-      @mutex.synchronize do
-        @app.call(env)
+      @mutex.lock
+      begin
+        response = @app.call(env)
+        returned = response << BodyProxy.new(response.pop) { unlock }
+      ensure
+        unlock unless returned
       end
     end
 
-    def self.rackup(config, app)
-      if config.multithread? || (config.concurrent? && RUBY_VERSION >= '3')
-        new(app)
-      else
-        app
-      end
+    private
+
+    def unlock
+      @mutex.unlock
     end
   end
 end

test/spec_lock.rb

@@ -2,24 +2,179 @@
 
 require_relative 'helper'
 
+class Lock
+  attr_reader :synchronized
+
+  def initialize
+    @synchronized = false
+  end
+
+  def lock
+    @synchronized = true
+  end
+
+  def unlock
+    @synchronized = false
+  end
+end
+
+module LockHelpers
+  def lock_app(app, lock = Lock.new)
+    app = if lock
+      Rack::Lock.new app, lock
+    else
+      Rack::Lock.new app
+    end
+    Rack::Lint.new app
+  end
+end
+
 describe Rack::Lock do
-  it "constructs lock when builder is multithreaded" do
-    x = Object.new
-    builder = Rack::Builder.new(config: Rack::Builder::Config.new(multithread: true)) do
-      use Rack::Lock
-      run x
+  include LockHelpers
+
+  describe 'Proxy' do
+    include LockHelpers
+
+    it 'delegate each' do
+      env      = Rack::MockRequest.env_for("/")
+      response = Class.new {
+        attr_accessor :close_called
+        def initialize; @close_called = false; end
+        def each; %w{ hi mom }.each { |x| yield x }; end
+      }.new
+
+      app = lock_app(lambda { |inner_env| [200, { "Content-Type" => "text/plain" }, response] })
+      response = app.call(env)[2]
+      list = []
+      response.each { |x| list << x }
+      list.must_equal %w{ hi mom }
     end
 
-    builder.to_app.must_be_kind_of Rack::Lock
+    it 'delegate to_path' do
+      lock = Lock.new
+      env  = Rack::MockRequest.env_for("/")
+
+      res = ['Hello World']
+      def res.to_path ; "/tmp/hello.txt" ; end
+
+      app = Rack::Lock.new(lambda { |inner_env| [200, { "Content-Type" => "text/plain" }, res] }, lock)
+      body = app.call(env)[2]
+
+      body.must_respond_to :to_path
+      body.to_path.must_equal "/tmp/hello.txt"
+    end
+
+    it 'not delegate to_path if body does not implement it' do
+      env = Rack::MockRequest.env_for("/")
+
+      res = ['Hello World']
+
+      app = lock_app(lambda { |inner_env| [200, { "Content-Type" => "text/plain" }, res] })
+      body = app.call(env)[2]
+
+      body.wont_respond_to :to_path
+    end
+  end
+
+  it 'call super on close' do
+    env      = Rack::MockRequest.env_for("/")
+    response = Class.new {
+      attr_accessor :close_called
+      def initialize; @close_called = false; end
+      def close; @close_called = true; end
+    }.new
+
+    app = lock_app(lambda { |inner_env| [200, { "Content-Type" => "text/plain" }, response] })
+    app.call(env)
+    response.close_called.must_equal false
+    response.close
+    response.close_called.must_equal true
+  end
+
+  it "not unlock until body is closed" do
+    lock     = Lock.new
+    env      = Rack::MockRequest.env_for("/")
+    response = Object.new
+    app      = lock_app(lambda { |inner_env| [200, { "Content-Type" => "text/plain" }, response] }, lock)
+    lock.synchronized.must_equal false
+    response = app.call(env)[2]
+    lock.synchronized.must_equal true
+    response.close
+    lock.synchronized.must_equal false
+  end
+
+  it "return value from app" do
+    env  = Rack::MockRequest.env_for("/")
+    body = [200, { "Content-Type" => "text/plain" }, %w{ hi mom }]
+    app  = lock_app(lambda { |inner_env| body })
+
+    res = app.call(env)
+    res[0].must_equal body[0]
+    res[1].must_equal body[1]
+    res[2].to_enum.to_a.must_equal ["hi", "mom"]
+  end
+
+  it "call synchronize on lock" do
+    lock = Lock.new
+    env = Rack::MockRequest.env_for("/")
+    app = lock_app(lambda { |inner_env| [200, { "Content-Type" => "text/plain" }, %w{ a b c }] }, lock)
+    lock.synchronized.must_equal false
+    app.call(env)
+    lock.synchronized.must_equal true
   end
 
-  it "ignores lock when builder is not multithreaded" do
-    x = Object.new
-    builder = Rack::Builder.new(config: Rack::Builder::Config.new(multithread: false)) do
-      use Rack::Lock
-      run x
+  it "unlock if the app raises" do
+    lock = Lock.new
+    env = Rack::MockRequest.env_for("/")
+    app = lock_app(lambda { raise Exception }, lock)
+    lambda { app.call(env) }.must_raise Exception
+    lock.synchronized.must_equal false
+  end
+
+  it "unlock if the app throws" do
+    lock = Lock.new
+    env = Rack::MockRequest.env_for("/")
+    app = lock_app(lambda {|_| throw :bacon }, lock)
+    lambda { app.call(env) }.must_throw :bacon
+    lock.synchronized.must_equal false
+  end
+
+  it 'not unlock if an error is raised before the mutex is locked' do
+    lock = Class.new do
+      def initialize() @unlocked = false end
+      def unlocked?() @unlocked end
+      def lock() raise Exception end
+      def unlock() @unlocked = true end
+    end.new
+    env = Rack::MockRequest.env_for("/")
+    app = lock_app(proc { [200, { "Content-Type" => "text/plain" }, []] }, lock)
+    lambda { app.call(env) }.must_raise Exception
+    lock.unlocked?.must_equal false
+  end
+
+  it "not reset the environment while the body is proxied" do
+    proxy = Class.new do
+      attr_reader :env
+      def initialize(env) @env = env end
     end
+    app = Rack::Lock.new lambda { |env| [200, { "Content-Type" => "text/plain" }, proxy.new(env)] }
+    response = app.call(Rack::MockRequest.env_for("/"))[2]
+  end
+
+  it "unlock if an exception occurs before returning" do
+    lock = Lock.new
+    env  = Rack::MockRequest.env_for("/")
+    app  = lock_app(proc { [].freeze }, lock)
+    lambda { app.call(env) }.must_raise Exception
+    lock.synchronized.must_equal false
+  end
+
+  it "not replace the environment" do
+    env  = Rack::MockRequest.env_for("/")
+    app  = lock_app(lambda { |inner_env| [200, { "Content-Type" => "text/plain" }, [inner_env.object_id.to_s]] })
+
+    _, _, body = app.call(env)
 
-    builder.to_app.must_be_same_as x
+    body.to_enum.to_a.must_equal [env.object_id.to_s]
   end
 end