Memory leak on repositories

issues
1

I’ve been running Hanami in production with a custom ROM integration (with ROM SQL, PostgreSQL) without any issues for more than a year now but ever since upgrading to Hanami 2.2, ROM 5.4 and replacing my custom integration with the one built-in I’ve been battling a nasty memory leak without much success. I currently have three pods (each running a Slice): API, RabbitMQ consumer and background jobs (with Sidekiq). The most impacted is the background job pod but it seems mostly due to the much higher workload than the others. Its memory goes from 100MB to 1GB in about 10 hours.

Through the use of the logger and ObjectSpace I’ve managed to narrow it down to the repositories, precisely object space reports a tons of ICLASS objects retained from rom-repository-5.4.2/lib/rom/repository/relation_reader.rb:75 and rom-repository-5.4.2/lib/rom/repository/class_interface.rb:63. Using the logger I also noticed that the repositories object_id changes every time it is injected via Deps while this isn’t true for the logger, the relations or even the container (and its cache) used within each repository.

So far the only thing out of place that I have seen is this line within ROM: rom/repository/lib/rom/repository/relation_reader.rb at d2de00f6249d17aea7965573972633677018f4cf · rom-rb/rom · GitHub which I guess should have been @cache instead of a regular variable but redefining this method doesn’t solve the issue, it just makes the cache accessible from outside.

I’m a bit lost and running short on time to investigate further, I was wondering if some of you with deeper knowledge of Hanami and ROM had an idea on what could be the cause or at least where I should look at next.

2

Yes, this is the norm for injected dependencies. The reason why the logger and relations do not is because they are explicitly registered as values by the provider.

You can memoize the dependency in two ways. If it is being manually registered, the register method accepts a memoize: true argument.

Likewise, if it’s an auto-registered component, you can add a # memoize: true magic comment at the top of the file.

This is documented under dry-system.

1 Like
3

Thanks for raising this, @rubyistdotjs. It looks like you’ve come across a legit memory leak in ROM, unearthed by our DI behaviour, which, as @alassek explains, defaults to creating new instances for most dependencies.

I’d like us to explore having Hanami memoize more dependencies by default, which would help with this, but that’s a change that I’d like to properly analyse and benchmark etc. before releasing. It’s the kind of thing I’d see as going into significant 2.x version bump.

In the meantime, the advice from @alassek is solid and should help you work around the issue. There’s also one other approach, which would be you providing a custom memoize block to the Hanami container’s underlying config. Something like this:

module MyApp
  class App < Hanami::App
    prepare_container do |container|
      container.config.component_dirs.dir("app") do |dir|
        dir.memoize = proc do |component|
          component.key.start_with?("repos.")
        end
      end
    end
  end
end

With an approach like this, you wouldn’t need to annotate every file in your repos/ dir with the # memoize: false magic comment, which could be a win.

n.b. I typed that out without testing it locally, so let us know how you go if you try it.

4

From the description, it looks like you generate repo classes in runtime. Classes, not objects; making new objects woudn’t be a problem. Is there a place in your code that can do this? So far, this is my best guess

5

Thank you very much for your help.

I added the memoize comment to every single repository on Thursday and deployed that into our staging environment to let potential side effects manifest themselves over the weekend. We had none, so I deployed it into production this morning, and it did solve the memory issue.

I will implement your solution of memoizing the repos by default, @timriley. I don’t see any reason why they shouldn’t be.

@flash-gordon, my latest theory at the time was: there is something during the repository instantiation that causes new classes to be generated. But I don’t see anything in my code that would do such a thing.

Some context

When I updated to Hanami v2.2, I replicated the default structure for repositories (which is close to what I had before):

.
├── app
│   └── db
│       └── repo.rb (root app repo)
└── slices
    └── slice
        ├── db
        │   └── repo.rb (root slice repo)
        └── repos
            └── the_repo.rb

# ./app/db/repo.rb
# frozen_string_literal: true

require 'hanami/db/repo'

module App
  module DB
    class Repo < Hanami::DB::Repo
    end
  end
end

# ./slices/slice/db/repo.rb
# frozen_string_literal: true

module Slice
  module DB
    class Repo < App::DB::Repo
    end
  end
end

# ./slices/slice/repos/the_repo.rb
# frozen_string_literal: true

module Slice
  module Repos
    class TheRepo < Slice::DB::Repo[:the_relation]
      auto_struct false

      def find_all(column:)
        the_relation.where(the_relation[:other_column].not('VALUE')).where(column:).to_a
      end
    end
  end
end

There is no metaprogramming in any of the repositories. I do not override anything, I don’t include any modules, etc… The repos just contain a bunch of methods to fetch, create, update, and destroy, using sometimes custom commands and changesets.

Within the code, the repos are always injected via Deps. Some dependencies may themselves use the same repos (by that I mean: Deps['repos.the_repo', 'operations.the_operation'], and the operation may itself have Deps['repos.the_repo']).

I have only one db provider in the root app which is passed down to the slices using config.db.import_from_parent = true.

Lastly, to note: we currently use an old version of Sidekiq (6.5.12). We can’t upgrade due to our also old version of Redis (which will hopefully change soon).

1 Like
6

Thanks, I’ll play around with a clean environment. In my apps I have everything frozen head to toe, this may be one of the reasons I haven’t come across the issue. In the extreme case, even if an app generates repo classes non-stop I would expect them GC-ed anyway. If it doesn’t happen for some reason it may be a bug too.