Ok, so after spending the good part of a day debugging this, we finally worked out where our problem is: Rack. Sorry, we’d been mistaken that dry-auto_inject and Sinatra were in conflict - somehow, despite both of them customising class initialisation, they do work together perfectly!
TL;DR: The real problem is that our Sinatra controller is only ever being instantiated once - when first passed to Rack’s run method!
Background
This all started from noticing that uploading a CSV of telemetry in our running app worked fine the first time, but failed subsequent times. Our unit and feature tests had not caught this because they don’t test actions twice.
The service object that handles this CSV upload memoises some data inside it, and it turned out that subsequent requests were reprocessing the data from the first request! From the dry-auto-inject documentation and our previous experience with Sinatra, we were led to believe that it would be safe to do this, since the whole dependency tree would be re-initialised for each request.
Finding the real issue
The fact that the controller was being instantiated only once (when first passed to Rack’s run method) - and so never recreating its dependency tree - was obscured by a few things:
-
We pull the controller out of the container when setting up Rack in config.ru. Pulling out of the container always returns a new instance.
map('/participants') { run AppContainer['controllers.participants'] }
-
We auto-register the controllers in the container
class AppContainer < Dry::System::Container
load_paths!('lib')
configure do |config|
#...
config.auto_register = %w[lib/anm/api/controllers]
end
end
-
From pry-ing, the Sinatra controller’s object_id is different in each request - misleading us into thinking that it was a brand new instance of the controller in each request; rather, this is due to Sinatra’s mechanism of cloning a pre-initialised controller for each request. The dry-auto-inject’d dependencies having the same object_id, whilst the controller had a new one, really screwed with our heads, since all the reading and experimentation we’d done indicated that a new instance’s dependencies would also be new instances.
I eventually worked out how to put together a minimal test case in order to share it here; but instead of reproducing the problem, it refuted our understanding! (code below)
I had expected the RegularClass one to pass. But after a rethink, if the method call is happening on the same instance, of course it’s reusing the dependency. And in this way, it showed that SinatraApp behaves as would be expected.
This test revealed that the true issue was not with Sinatra. Being new to dry-rb containers and injection, we had a lot of unfamiliar components acting together. I’ll try harder to get to this minimal reproduction much earlier in future!
Shortly after that, I was able to determine it was with our use of Rack. I hypothesised that there could be a difference in the Rack methods, use vs run - the former takes a class (for a middleware Rack layer) and the latter takes an instance (for the final Rack layer); perhaps use would instantiate the class for each request. And indeed, after writing the tests, this was the case:
Here’s the spec for this minimal reproduction:
#!/usr/bin/env ruby
require 'bundler/inline'
gemfile do
source 'https://rubygems.org'
gem 'dry-system', '0.17.0'
gem 'rack-test', '1.1.0'
gem 'sinatra', '2.0.8.1'
gem 'rspec', '3.8.0'
end
require 'rspec/autorun'
require 'dry/system/container'
require 'rack/test'
require 'sinatra/base'
include Rack::Test::Methods
class AppContainer < Dry::System::Container
register('repo') { 'i am a dependency' }
end
Import = AppContainer.injector
class SinatraApp < Sinatra::Base
include Import[repo: 'repo']
get '/' do
repo.object_id.to_s
end
end
class RegularClass
include Import[repo: 'repo']
def call
repo.object_id.to_s
end
end
RSpec.describe 'sinatra dry-auto-inject memoisation' do
describe SinatraApp do
let(:app) do
SinatraApp.new
end
it 'uses a new instance of the injected dependency for each request' do
repo_id_1 = get('/').body
repo_id_2 = get('/').body
expect(repo_id_1).not_to eq(repo_id_2)
end
end
describe RegularClass do
let(:regular_class) do
RegularClass.new
end
it 'uses a new instance of the injected dependency for each call' do
repo_id_1 = regular_class.call
repo_id_2 = regular_class.call
expect(repo_id_1).not_to eq(repo_id_2)
end
end
describe "rack app via 'run'" do
let(:app) do
Rack::Builder.new do
run SinatraApp.new
end
end
it 'uses a new instance of the injected dependency for each request' do
repo_id_1 = get('/').body
repo_id_2 = get('/').body
expect(repo_id_1).not_to eq(repo_id_2)
end
end
describe "rack app via 'use'" do
let(:app) do
Rack::Builder.new do
use SinatraApp
run ->(env) { [404, {'Content-Type' => 'text/plain'}, ['Not found']] }
end
end
it 'uses a new instance of the injected dependency for each request' do
repo_id_1 = get('/').body
repo_id_2 = get('/').body
expect(repo_id_1).not_to eq(repo_id_2)
end
end
end
Where to next
We’ve implemented a workaround in our impacted controller to stop using dry-auto-injection for now, instead instantiating the dependencies subsequently to the controller itself being instantiated. These can actually safely be memoised (scoped only within a request) thanks to the dup functionality of Sinatra. So within the controller:
def process_transaction
@process_transaction ||= AppContainer['telemetry.process.transaction']
end
This is not optimal, and it’d be great to be able to use dry-auto-inject everywhere; so I’d like to pose a question to the developers/community: What would you suggest instead of this post-initialisation technique? We could try Rack’s use (with a class) instead of run (with an instance), but that would require some sort of no-op final Rack app. Our config.ru has many runs, from using several Rack maps; so we’d end up specifying to run a lot of these extra no-op Rack apps. Perhaps we could use a lambda to return a new controller instance to each request call…?
I would strongly recommend that this behaviour with Rack be mentioned in the dry-auto-inject documentation. It does make sense when you spend a day going through it, but Rack is common enough that I suspect there may be subtle memoisation bugs from this out there in people’s apps.
Thanks for all the work on dry-rb! You guys rock
