Introduce Deferred Parent Cache Expiry

[Stivaros]

The case in the tests models the situation this solves for. In summary, if a parent record has three children records which are all deleted, we don't want to expire the parent record cache three times. Once will do, at the end of the operation.

This PR introduces an "escape" for the expiration of a parent's cache. It is currently reliant upon the user of the gem to utilise this functionality safely. Therefore, it is strongly recommended to be used within a transaction.

To introduce the feature we are simply using a class method that wraps the existing code. If this feature has further use, we may choose to build upon this approach in two ways:

1. Control it via configuration (rather than this API)
2. Handle more cases than parent cache expiry

[danmayer]

A few answers:

• for the changelog, we can add a section that briefly covers adding a feature to avoid duplicate cache invalidation to this file https://github.com/Shopify/identity_cache/blob/main/CHANGELOG.md
• since this is a public / open source gem, the release is a bit different than internal, ping me on slack and I can help with that when we get there... but it is mostly just done through github actions, tags, and releases.
• depending on your integration we could also make a RC release to fully test and deploy elsewhere to verify the functionality and the integration interface.

[danmayer]

I agree we just want to empty this vs trying to process it in the ensure block... all real processing we expect to be handled normally in the around yield this cleans up in both standard and exceptional cases

[karmakaze]

The ergonomics of disallowing nested use make it unpredictable to compose. We could instead allow nested use with the outermost usage doing the actual invalidations.

def with_deferred_parent_expiration
  if Thread.current[:parent_records_for_cache_expiry]
    return yield
  end

  Thread.current[:parent_records_for_cache_expiry] = Set.new

  result = yield

  Thread.current[:parent_records_for_cache_expiry].each(&:expire_primary_index)
  result
ensure
  Thread.current[:parent_records_for_cache_expiry] = nil
end

Other suggestions:

• may not need a separate boolean with :parent_records_for_cache_expiry being nil before/after use
• return value of with_deferred_parent_expiration can be that of the yield block passthru.
• since Thread.current is accessible by name from any method it would be good to use a prefix for the key, e.g. idc_parent_records_for_cache_expiry

[Stivaros]

Hmm, perhaps I'm misunderstanding but, in the code above, return yield will lead to unintended side effects. I believe the code will do the following:

IdentityCache.with_deferred_expiration do
  @parent.transaction do
    @parent.children.destroy_all # Thread.current[:parent_records_for_cache_expiry] => [@parent]
    IdentityCache.with_deferred_expiration do # The `ensure` block runs when we `return yield`, so Thread.current[:parent_records_for_cache_expiry] => `nil`
      @unrelated_record.update # Thread.current[:parent_records_for_cache_expiry] => [<parent of @unrelated_record>]
    end
  end
end

I'm fairly sure ensure always runs, even when returning early, which means that @parent wouldn't be expired here, even though its associated children records were destroyed and had it queued for expiry.

may not need a separate boolean with :parent_records_for_cache_expiry being nil before/after use

I believe we will need some "global" marker to inform the ParentModelExpiration class when to alter its behaviour as this is not default behaviour and we don't want to alter behaviour in all cases.

return value of with_deferred_parent_expiration can be that of the yield block passthru.

To my shame, I missed this and it definitely needed fixing, thanks for flagging this! I have amended, rebased and pushed the changed behaviour.

since Thread.current is accessible by name from any method it would be good to use a prefix for the key, e.g. idc_parent_records_for_cache_expiry

Great suggestion, have prefixed the Thread.current hash keys to prefix idc_.

[karmakaze]

That's right about early niling with nested usage, we only want the ensure behaviour on the outermost one. We can get that by using a nest level counter, incremented on each entry of with_deferred_parent_expiration decremented for each exit and only do the niling when zero.

Would we also need levels of Thread.current[:parent_records_for_cache_expiry].each(&:expire_primary_index) as well for nesting to work properly? e.g.

  Thread.current[:parent_records_for_cache_expiries] ||= []
  Thread.current[:parent_records_for_cache_expiries] << Set.new
  result = yield
  Thread.current[:parent_records_for_cache_expiries].last.each(&:expire_primary_index)
  result
ensure
  Thread.current[:parent_records_for_cache_expiries].pop

[kirs]

Looking great!