Sinatra conflicting with Dry::AutoInject, resulting in memoisation of dependency tree

I’m working on a Sinata project that is using Dry::AutoInject to insert dependancies into our controllers. However we have discovered that our dependencies are being memoized. It is usually not a problem because our dependencies do not have state but it is unexpected.

Long story short our controllers are initialized once and Sinatra creates a duplicate for each request

Unfortunately Dry::AutoInject creates an initialize method to inject dependencies. This is obviously incompatible. What solutions do we have?

As a note perhaps Dry::AutoInject could document the frameworks it is incompatible with.

cheers
Jerome

1 Like

Jerome and I think this is the underlying issue that was experienced in another thread regarding Sinatra from a few years ago: Turning off dry-auto_inject memoization

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