From 6c2080a50a24158b20e46d61b8777fadca24c3c2 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Mon, 27 Jul 2026 23:26:07 +0200 Subject: [PATCH] Return the compiled instance from Grape::API::Instance.compile! .call and .recognize_path compiled the API and then read @instance again to serve the request: def call(env) compile! @instance.call(env) end change! (which `helpers` and `mount` invoke on a live API) sets @instance to nil, so a change! landing between the two reads leaves both methods calling nil -- NoMethodError: undefined method 'call' for nil. Have compile! hand back the instance it compiled and let both callers use that value, so the read is atomic. compile! previously returned nil when the API was already compiled, so always returning the instance is a strict superset of its old contract. The window is small in practice -- a request thread spends nearly all of its time inside `new` when recompiling -- so this is a latent race rather than one reproduced under natural contention; the specs drive change! into the window deterministically. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + lib/grape/api/instance.rb | 13 ++++++------- spec/grape/api_spec.rb | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-) 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