Remove per-request "server implementation details" and provide a consistent way to expose this to middleware during build time.

CHANGELOG.md

@@ -34,6 +34,10 @@ All notable changes to this project will be documented in this file. For info on
 
 - [[CVE-2020-8184](https://nvd.nist.gov/vuln/detail/CVE-2020-8184)] Do not allow percent-encoded cookie name to override existing cookie names. BREAKING CHANGE: Accessing cookie names that require URL encoding with decoded name no longer works. ([@fletchto99](https://github.com/fletchto99))
 
+### Removed
+
+- Remove `rack.multithread`/`rack.multiprocess`/`rack.run_once`. These variables generally come too late to be useful. Removed `Rack::Lock` which depends on these variables. ([#1618](https://github.com/rack/rack/pull/1591), [@ioquatix](https://github.com/ioquatix))
+
 ## [2.2.2] - 2020-02-11
 
 ### Fixed

README.rdoc

@@ -77,7 +77,6 @@ middleware:
 * Rack::Files, for serving static files.
 * Rack::Head, for returning an empty body for HEAD requests.
 * Rack::Lint, for checking conformance to the \Rack API.
-* Rack::Lock, for serializing requests using a mutex.
 * Rack::Logger, for setting a logger to handle logging errors.
 * Rack::MethodOverride, for modifying the request method based on a submitted
   parameter.

SPEC.rdoc

@@ -74,19 +74,6 @@ Rack-specific variables:
                            request URL.
 <tt>rack.input</tt>:: See below, the input stream.
 <tt>rack.errors</tt>:: See below, the error stream.
-<tt>rack.multithread</tt>:: true if the application object may be
-                            simultaneously invoked by another thread
-                            in the same process, false otherwise.
-<tt>rack.multiprocess</tt>:: true if an equivalent application object
-                             may be simultaneously invoked by another
-                             process, false otherwise.
-<tt>rack.run_once</tt>:: true if the server expects
-                         (but does not guarantee!) that the
-                         application will only be invoked this one
-                         time during the life of its containing
-                         process. Normally, this will only be true
-                         for a server based on CGI
-                         (or something similar).
 <tt>rack.hijack?</tt>:: present and true if the server supports
                         connection hijacking. See below, hijacking.
 <tt>rack.hijack</tt>:: an object responding to #call that must be

lib/rack.rb

@@ -55,9 +55,6 @@ module Rack
   RACK_SESSION                        = 'rack.session'
   RACK_SESSION_OPTIONS                = 'rack.session.options'
   RACK_SHOWSTATUS_DETAIL              = 'rack.showstatus.detail'
-  RACK_MULTITHREAD                    = 'rack.multithread'
-  RACK_MULTIPROCESS                   = 'rack.multiprocess'
-  RACK_RUNONCE                        = 'rack.run_once'
   RACK_URL_SCHEME                     = 'rack.url_scheme'
   RACK_HIJACK                         = 'rack.hijack'
   RACK_IS_HIJACK                      = 'rack.hijack?'

lib/rack/builder.rb

@@ -59,9 +59,9 @@ class Builder
     #   # requires ./my_app.rb, which should be in the
     #   # process's current directory.  After requiring,
     #   # assumes MyApp constant contains Rack application
-    def self.parse_file(path)
+    def self.parse_file(path, **options)
       if path.end_with?('.ru')
-        return self.load_file(path)
+        return self.load_file(path, **options)
       else
         require path
         return Object.const_get(::File.basename(path, '.rb').split('_').map(&:capitalize).join(''))
@@ -83,7 +83,7 @@ def self.parse_file(path)
     #   use Rack::ContentLength
     #   require './app.rb'
     #   run App
-    def self.load_file(path)
+    def self.load_file(path, **options)
       config = ::File.read(path)
       config.slice!(/\A#{UTF_8_BOM}/) if config.encoding == Encoding::UTF_8
 
@@ -93,28 +93,52 @@ def self.load_file(path)
 
       config.sub!(/^__END__\n.*\Z/m, '')
 
-      return new_from_string(config, path)
+      return new_from_string(config, path, **options)
     end
 
     # Evaluate the given +builder_script+ string in the context of
     # a Rack::Builder block, returning a Rack application.
-    def self.new_from_string(builder_script, file = "(rackup)")
-      # We want to build a variant of TOPLEVEL_BINDING with self as a Rack::Builder instance.
-      # We cannot use instance_eval(String) as that would resolve constants differently.
-      binding, builder = TOPLEVEL_BINDING.eval('Rack::Builder.new.instance_eval { [binding, self] }')
-      eval builder_script, binding, file
+    def self.new_from_string(builder_script, file = "(rackup)", **options)
+      builder = self.new(**options)
+
+      # Create a top level scope with self as the builder instance:
+      binding = TOPLEVEL_BINDING.eval('->(builder){builder.instance_eval{binding}}').call(builder)
+
+      eval(builder_script, binding, file)
 
       return builder.to_app
     end
 
+    DEFAULT_APP = proc{|env| [404, [], []]}
+
     # Initialize a new Rack::Builder instance.  +default_app+ specifies the
     # default application if +run+ is not called later.  If a block
     # is given, it is evaluted in the context of the instance.
-    def initialize(default_app = nil, &block)
-      @use, @map, @run, @warmup, @freeze_app = [], nil, default_app, nil, false
+    def initialize(default_app = DEFAULT_APP, **options, &block)
+      @use = []
+      @map = nil
+      @run = default_app
+      @warmup = nil
+      @freeze_app = false
+
+      @options = options
+
       instance_eval(&block) if block_given?
     end
 
+    attr :options
+
+    # Whether the application server will invoke the application from multiple threads. Implies {reentrant?}.
+    def multithread?
+      @options[:multithread]
+    end
+
+    # Re-entrancy is a feature of event-driven servers which may perform non-blocking operations. When an operation blocks, that particular request may yield and another request may enter the application stack.
+    # @return [Boolean] Whether the application can be invoked multiple times from the same thread.
+    def reentrant?
+      multithread? || @options[:reentrant]
+    end
+
     # Create a new Rack::Builder instance and return the Rack application
     # generated from it.
     def self.app(default_app = nil, &block)
@@ -145,7 +169,12 @@ def use(middleware, *args, &block)
         mapping, @map = @map, nil
         @use << proc { |app| generate_map(app, mapping) }
       end
-      @use << proc { |app| middleware.new(app, *args, &block) }
+
+      if middleware.respond_to?(:rackup)
+        @use << proc { |app| rackup(app, middleware, *args) }
+      else
+        @use << proc { |app| middleware.new(app, *args, &block) }
+      end
     end
     ruby2_keywords(:use) if respond_to?(:ruby2_keywords, true)
 
@@ -220,8 +249,8 @@ def freeze_app
     # Return the Rack application generated by this instance.
     def to_app
       app = @map ? generate_map(@run, @map) : @run
-      fail "missing run or map statement" unless app
       app.freeze if @freeze_app
+
       app = @use.reverse.inject(app) { |a, e| e[a].tap { |x| x.freeze if @freeze_app } }
       @warmup.call(app) if @warmup
       app
@@ -236,6 +265,15 @@ def call(env)
 
     private
 
+    def rackup(app, middleware, *args)
+      builder = self.class.new(app, **@options) do
+        # In this context, self is the builder instance:
+        middleware.rackup(self, *args)
+      end
+
+      return builder.to_app
+    end
+
     # Generate a URLMap instance by generating new Rack applications for each
     # map block in this instance.
     def generate_map(default_app, mapping)

lib/rack/handler/cgi.rb

@@ -18,9 +18,6 @@ def self.serve(app)
           RACK_VERSION      => Rack::VERSION,
           RACK_INPUT        => Rack::RewindableInput.new($stdin),
           RACK_ERRORS       => $stderr,
-          RACK_MULTITHREAD  => false,
-          RACK_MULTIPROCESS => true,
-          RACK_RUNONCE      => true,
           RACK_URL_SCHEME   => ["yes", "on", "1"].include?(ENV[HTTPS]) ? "https" : "http"
         )
 

lib/rack/handler/webrick.rb

@@ -75,9 +75,6 @@ def service(req, res)
           RACK_VERSION      => Rack::VERSION,
           RACK_INPUT        => rack_input,
           RACK_ERRORS       => $stderr,
-          RACK_MULTITHREAD  => true,
-          RACK_MULTIPROCESS => false,
-          RACK_RUNONCE      => false,
           RACK_URL_SCHEME   => ["yes", "on", "1"].include?(env[HTTPS]) ? "https" : "http",
           RACK_IS_HIJACK    => true,
           RACK_HIJACK       => lambda { raise NotImplementedError, "only partial hijack is supported."},

lib/rack/lint.rb

@@ -157,22 +157,6 @@ def check_env(env)
 
       ## <tt>rack.errors</tt>:: See below, the error stream.
 
-      ## <tt>rack.multithread</tt>:: true if the application object may be
-      ##                             simultaneously invoked by another thread
-      ##                             in the same process, false otherwise.
-
-      ## <tt>rack.multiprocess</tt>:: true if an equivalent application object
-      ##                              may be simultaneously invoked by another
-      ##                              process, false otherwise.
-
-      ## <tt>rack.run_once</tt>:: true if the server expects
-      ##                          (but does not guarantee!) that the
-      ##                          application will only be invoked this one
-      ##                          time during the life of its containing
-      ##                          process. Normally, this will only be true
-      ##                          for a server based on CGI
-      ##                          (or something similar).
-
       ## <tt>rack.hijack?</tt>:: present and true if the server supports
       ##                         connection hijacking. See below, hijacking.
 
@@ -274,8 +258,7 @@ def check_env(env)
       ##
 
       %w[REQUEST_METHOD SERVER_NAME QUERY_STRING
-         rack.version rack.input rack.errors
-         rack.multithread rack.multiprocess rack.run_once].each { |header|
+         rack.version rack.input rack.errors].each { |header|
         assert("env missing required key #{header}") { env.include? header }
       }
 

lib/rack/lock.rb

@@ -1,32 +1,27 @@
 # frozen_string_literal: true
 
-require 'thread'
+require 'logger'
 
 module Rack
-  # Rack::Lock locks every request inside a mutex, so that every request
-  # will effectively be executed synchronously.
+  # Sets up rack.logger to write to rack.errors stream
   class Lock
-    def initialize(app, mutex = Mutex.new)
-      @app, @mutex = app, mutex
-    end
+    class Wrapper
+      def initialize(app)
+        @app = app
+        @mutex = ::Thread::Mutex.new
+      end
 
-    def call(env)
-      @mutex.lock
-      @env = env
-      @old_rack_multithread = env[RACK_MULTITHREAD]
-      begin
-        response = @app.call(env.merge!(RACK_MULTITHREAD => false))
-        returned = response << BodyProxy.new(response.pop) { unlock }
-      ensure
-        unlock unless returned
+      def call(env)
+        @mutex.synchronize do
+          @app.call(env)
+        end
       end
     end
 
-    private
-
-    def unlock
-      @mutex.unlock
-      @env[RACK_MULTITHREAD] = @old_rack_multithread
+    def self.rackup(builder)
+      if builder.multithread?
+        builder.use(Wrapper)
+      end
     end
   end
 end

lib/rack/mock.rb

@@ -44,9 +44,6 @@ def string
       RACK_VERSION      => Rack::VERSION,
       RACK_INPUT        => StringIO.new,
       RACK_ERRORS       => StringIO.new,
-      RACK_MULTITHREAD  => true,
-      RACK_MULTIPROCESS => true,
-      RACK_RUNONCE      => false,
     }.freeze
 
     def initialize(app)

lib/rack/request.rb

@@ -159,7 +159,6 @@ def query_string;    get_header(QUERY_STRING).to_s                  end
       def content_length;  get_header('CONTENT_LENGTH')                   end
       def logger;          get_header(RACK_LOGGER)                        end
       def user_agent;      get_header('HTTP_USER_AGENT')                  end
-      def multithread?;    get_header(RACK_MULTITHREAD)                   end
 
       # the referer of the client
       def referer;         get_header('HTTP_REFERER')                     end

lib/rack/session/pool.rb

@@ -36,6 +36,7 @@ def initialize(app, options = {})
         @mutex = Mutex.new
       end
 
+      # This method is not thread safe.
       def generate_sid
         loop do
           sid = super
@@ -44,7 +45,7 @@ def generate_sid
       end
 
       def find_session(req, sid)
-        with_lock(req) do
+        @mutex.synchronize do
           unless sid and session = get_session_with_fallback(sid)
             sid, session = generate_sid, {}
             @pool.store sid.private_id, session
@@ -54,27 +55,20 @@ def find_session(req, sid)
       end
 
       def write_session(req, session_id, new_session, options)
-        with_lock(req) do
+        @mutex.synchronize do
           @pool.store session_id.private_id, new_session
           session_id
         end
       end
 
       def delete_session(req, session_id, options)
-        with_lock(req) do
+        @mutex.synchronize do
           @pool.delete(session_id.public_id)
           @pool.delete(session_id.private_id)
           generate_sid unless options[:drop]
         end
       end
 
-      def with_lock(req)
-        @mutex.lock if req.multithread?
-        yield
-      ensure
-        @mutex.unlock if @mutex.locked?
-      end
-
       private
 
       def get_session_with_fallback(sid)

test/spec_builder.rb

@@ -218,10 +218,9 @@ def o.call(env)
     Rack::MockRequest.new(app).get("/c").status.must_equal 200
   end
 
-  it 'complains about a missing run' do
-    proc do
-      Rack::Lint.new Rack::Builder.app { use Rack::ShowExceptions }
-    end.must_raise(RuntimeError)
+  it 'should result in the default app' do
+    app = Rack::Builder.new.to_app
+    app.must_equal Rack::Builder::DEFAULT_APP
   end
 
   describe "parse_file" do