Don't hold unnecessary references to subjects in @rules_index

[mtoneil]

The @rules_index ends up holding a reference to any subject that is
authorized, by virtue of the hash having a default value of an empty
array. This in effect facilitiates a memory leak, by which objects can't
be garbage collected until the Ability instance is discarded, even
though the Ability has no use for these objects. This is especially
problematic for long-running background jobs that need to authorize many
objects (e.g. hundreds of thousands) against a single Ability instance.

By getting rid of @rule_index's default empty array value, we can allow
for better garbage collection and reduce the size of the hash. These
differences add up greatly when authorizing objects at scale.

[mtoneil]

Hi @coorasse, wondering if you could give a review or suggest someone who could. Thanks!

[coorasse]

I cannot really say if this has any impact on the GC. I'd have expected a test to cover this, but I am not an expert on this. What I can say is that this code is perfectly fine for me

[mtoneil]

I cannot really say if this has any impact on the GC. I'd have expected a test to cover this, but I am not an expert on this. What I can say is that this code is perfectly fine for me

@coorasse Thanks for your review!

Before submitting this patch I confirmed in a long-running Sidekiq job that ActiveRecord objects I pass in as subjects (e.g. can?(:read, foo)) do not get garbage collected unless I include this patch. It was obvious when looking at the process size (which grew indefinitely as I fetched ActiveRecord objects and authorized them in batches). I was also able to confirm via ObjectSpace.each_object(Foo) that the objects my code no longer referenced still existed longer after I had discarded them. I used the ruby-mass gem to track the references preventing garbage collection to the @rules_index, which led me to this fix. With the patch applied, process size stayed flat and ObjectSpace indicated objects were being garbage collected at a regular pace.

On normal web requests or background tasks this change has no appreciable effect. Only when authorizing objects at scale in one go (e.g. tens or hundreds of thousands of objects) would the memory issue start to become apparent.

Unfortunately I can't think of an automated test that would confirm an object can be garbage collected after being authorized, since garbage collection can't be invoked on-demand (GC.start is merely a suggestion). The best I think we can do is confirm the behavior of the @rules_index is improved to prevent regressions.

I've fixed the linting issue and updated the CHANGELOG. Please let me know if there is anything else you need from me in order to include this change in the next release.

Thanks!

[coorasse]

That's really nice! Your explanation, here, as a comment will suffice.