diff --git a/CHANGELOG.md b/CHANGELOG.md index 44c056462..b65673e88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ * [#2831](https://github.com/ruby-grape/grape/pull/2831): Document and test open (beginless/endless) `Range`s as a `values:`/`except_values:` alternative to a dedicated numericality validator - [@dblock](https://github.com/dblock). * [#2830](https://github.com/ruby-grape/grape/pull/2830): Remove `Grape::Util::InheritableValues`, resolving inheritable settings by walking `Grape::Util::InheritableSetting#parent` instead of layering a second chain of stores, and keep namespace settings in a plain Hash - [@ericproulx](https://github.com/ericproulx). * [#2833](https://github.com/ruby-grape/grape/pull/2833): Fix `Grape::API::Instance.to_s` returning an empty string instead of the class name when the instance has no base (regression from #2581) - [@ericproulx](https://github.com/ericproulx). +* [#2835](https://github.com/ruby-grape/grape/pull/2835): Return the compiled instance from `Grape::API::Instance.compile!` so `call` and `recognize_path` no longer re-read `@instance`, which a concurrent `change!` could nil between the two reads - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/lib/grape/api/instance.rb b/lib/grape/api/instance.rb index 0e423490e..f3cce286d 100644 --- a/lib/grape/api/instance.rb +++ b/lib/grape/api/instance.rb @@ -54,20 +54,19 @@ def reset! # the headers, and the body. See [the rack specification] # (http://www.rubydoc.info/github/rack/rack/master/file/SPEC) for more. def call(env) - compile! - @instance.call(env) + compile!.call(env) end + # Returns the compiled instance, so callers serve the one they + # compiled rather than re-reading @instance — +change!+ can nil it + # between the two reads (see #call / #recognize_path). def compile! - return if @instance - - LOCK.synchronize { @instance ||= new } + @instance || LOCK.synchronize { @instance ||= new } end # see Grape::Router#recognize_path def recognize_path(path) - compile! - @instance.router.recognize_path(path) + compile!.router.recognize_path(path) end # Wipe the compiled API so we can recompile after changes were made. diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 5e6ffe449..ff3454f15 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -25,6 +25,42 @@ expect(shared.version_options).to be_present end + + # Regression: #call and #recognize_path read @instance again after + # compiling it, so a change! landing between the two reads (what `helpers` + # and `mount` do on a live API) left them calling nil. + it 'compiles to the instance so callers need not re-read @instance' do + subject.get('/x') { 'x' } + base = subject.base_instance + + compiled = base.compile! + expect(compiled).to be_a(base) + # Already compiled: must still hand back the instance rather than nil, + # which is what lets #call and #recognize_path use the return value. + expect(base.compile!).to be(compiled) + end + + it 'serves the compiled instance when change! lands in the compile window' do + subject.get('/x') { 'x' } + base = subject.base_instance + + allow(base).to receive(:compile!).and_wrap_original do |original| + original.call.tap { base.change! } + end + + expect { subject.call(Rack::MockRequest.env_for('/x')) }.not_to raise_error + end + + it 'recognizes a path when change! lands in the compile window' do + subject.get('/x') { 'x' } + base = subject.base_instance + + allow(base).to receive(:compile!).and_wrap_original do |original| + original.call.tap { base.change! } + end + + expect { subject.recognize_path('/x') }.not_to raise_error + end end describe '.prefix' do