diff --git a/CHANGELOG.md b/CHANGELOG.md index dbdb5cad7..70bb9aff4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.rdoc b/README.rdoc index caebc845f..37d9fa6a5 100644 --- a/README.rdoc +++ b/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. diff --git a/SPEC.rdoc b/SPEC.rdoc index 4cb02d744..89981fd7c 100644 --- a/SPEC.rdoc +++ b/SPEC.rdoc @@ -74,19 +74,6 @@ Rack-specific variables: request URL. rack.input:: See below, the input stream. rack.errors:: See below, the error stream. -rack.multithread:: true if the application object may be - simultaneously invoked by another thread - in the same process, false otherwise. -rack.multiprocess:: true if an equivalent application object - may be simultaneously invoked by another - process, false otherwise. -rack.run_once:: 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). rack.hijack?:: present and true if the server supports connection hijacking. See below, hijacking. rack.hijack:: an object responding to #call that must be diff --git a/lib/rack.rb b/lib/rack.rb index b931292e8..ad3efec68 100644 --- a/lib/rack.rb +++ b/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?' diff --git a/lib/rack/builder.rb b/lib/rack/builder.rb index 7f298e8dd..fa64ae3ed 100644 --- a/lib/rack/builder.rb +++ b/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) diff --git a/lib/rack/handler/cgi.rb b/lib/rack/handler/cgi.rb index b514fe747..df0864a14 100644 --- a/lib/rack/handler/cgi.rb +++ b/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" ) diff --git a/lib/rack/handler/webrick.rb b/lib/rack/handler/webrick.rb index 07c7f9e0e..9ce73b400 100644 --- a/lib/rack/handler/webrick.rb +++ b/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."}, diff --git a/lib/rack/lint.rb b/lib/rack/lint.rb index 67264771d..c7c621ef2 100755 --- a/lib/rack/lint.rb +++ b/lib/rack/lint.rb @@ -157,22 +157,6 @@ def check_env(env) ## rack.errors:: See below, the error stream. - ## rack.multithread:: true if the application object may be - ## simultaneously invoked by another thread - ## in the same process, false otherwise. - - ## rack.multiprocess:: true if an equivalent application object - ## may be simultaneously invoked by another - ## process, false otherwise. - - ## rack.run_once:: 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). - ## rack.hijack?:: 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 } } diff --git a/lib/rack/lock.rb b/lib/rack/lock.rb index 4bae3a903..ce8fdf91e 100644 --- a/lib/rack/lock.rb +++ b/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 diff --git a/lib/rack/mock.rb b/lib/rack/mock.rb index f330ae18f..c704ef82b 100644 --- a/lib/rack/mock.rb +++ b/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) diff --git a/lib/rack/request.rb b/lib/rack/request.rb index 74377ede3..3225066f4 100644 --- a/lib/rack/request.rb +++ b/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 diff --git a/lib/rack/session/pool.rb b/lib/rack/session/pool.rb index 4885605f5..87a5e8f5e 100644 --- a/lib/rack/session/pool.rb +++ b/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) diff --git a/test/spec_builder.rb b/test/spec_builder.rb index eb13a9766..3fcc9933d 100644 --- a/test/spec_builder.rb +++ b/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 diff --git a/test/spec_lock.rb b/test/spec_lock.rb index 895704986..1a745eec9 100644 --- a/test/spec_lock.rb +++ b/test/spec_lock.rb @@ -2,202 +2,22 @@ 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 - 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 - - 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" + it "constructs lock when builder is multithreaded" do + builder = Rack::Builder.new(nil, multithread: true) do + use Rack::Lock 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 + app = builder.to_app + app.must_be_kind_of Rack::Lock::Wrapper 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 "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 "set multithread flag to false" do - app = lock_app(lambda { |env| - env['rack.multithread'].must_equal false - [200, { "Content-Type" => "text/plain" }, %w{ a b c }] - }, false) - env = Rack::MockRequest.env_for("/") - env['rack.multithread'].must_equal true - _, _, body = app.call(env) - body.close - env['rack.multithread'].must_equal true - end - - it "reset original multithread flag when exiting lock" do - app = Class.new(Rack::Lock) { - def call(env) - env['rack.multithread'].must_equal true - super - end - }.new(lambda { |env| [200, { "Content-Type" => "text/plain" }, %w{ a b c }] }) - Rack::Lint.new(app).call(Rack::MockRequest.env_for("/")) - 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 + it "ignores lock when builder is not multithreaded" do + builder = Rack::Builder.new(nil, multithread: false) do + use Rack::Lock end - app = Rack::Lock.new lambda { |env| [200, { "Content-Type" => "text/plain" }, proxy.new(env)] } - response = app.call(Rack::MockRequest.env_for("/"))[2] - response.env['rack.multithread'].must_equal false - 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) - body.to_enum.to_a.must_equal [env.object_id.to_s] + app = builder.to_app + app.must_be_nil end end diff --git a/test/spec_session_pool.rb b/test/spec_session_pool.rb index aba93fb16..306786f65 100644 --- a/test/spec_session_pool.rb +++ b/test/spec_session_pool.rb @@ -225,7 +225,7 @@ tnum = rand(7).to_i + 5 r = Array.new(tnum) do Thread.new(treq) do |run| - run.get('/', "HTTP_COOKIE" => cookie, 'rack.multithread' => true) + run.get('/', "HTTP_COOKIE" => cookie) end end.reverse.map{|t| t.run.join.value } r.each do |resp| diff --git a/test/spec_webrick.rb b/test/spec_webrick.rb index b0bfa2b08..a6ea3cb5b 100644 --- a/test/spec_webrick.rb +++ b/test/spec_webrick.rb @@ -50,9 +50,6 @@ def is_running? it "have rack headers" do GET("/test") response["rack.version"].must_equal [1, 3] - response["rack.multithread"].must_equal true - assert_equal false, response["rack.multiprocess"] - assert_equal false, response["rack.run_once"] end it "have CGI headers on GET" do