Return the compiled instance from Grape::API::Instance.compile!

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

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.

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