Fixing cache

[Chengxuan]

The existing cache logic is not aware of TTL. So I don't believe TTL configuration in caching ever worked.

https://github.com/karlseguin/ccache#get Here is the explanation of why the Get method from ccache returns expired values.

I also found that we were extending the TTL of an item on all the Get calls. I couldn't figure out why it was needed. It was there before the refactor: https://github.com/hyperledger/firefly/pull/1005/files#diff-980438bfe0743cd28ea9e330bee535be87f8de11bc8cdc83595046f9a0fb2144L862

I propose we remove it and added test for the new behaviour

[peterbroadhurst]

@Chengxuan - the intention of this code was as follows (which I think I worked out way back in the mists of time reading the code of the ccache library, but that's not certainly to say it's right!)

• 9:00am: something goes into the catch, it gets a 10m expiry - so it would be evicted at 9:10am
• 9:05am: we get that same entry again from the cache - it has not expired, but now we want the cache to expire at 9:15am - because it was last used at 9:05am
• 9:15am - it gets automatically expired from the cache, so the memory is freed

Sounds like you've inspected the cache further an the behavior is different. Specifically:

• The call to cached.Extend is not required to extend the TTL from 9:10am to 9:15am in the above scenario
  • See below that Andrew did an investigation into this point
• There is no automated purging of the memory from the cache 😳 ... instead you have to get the item from the cache, in order to expire it

[peterbroadhurst]

Even with all of ^^^ fully explained, I don't think we want && !cached.Expired() included here.

The only reason the TTL expiry is added in this cache is to reap back memory. If we've got the item in our hand, the intention is it is always valid, and has saved us an expensive database lookup.

So returning cached.Value() is the right thing in all use cases of the cache I'm aware of, even if it's expired.

So if we need to implement our own memory reaping with this library, then maybe it's just a sign the library isn't for us.

[peterbroadhurst]

Think there's some outstanding questions to work through here, to determine what the right change is.

While the cache as it is today might be using more memory than we hope due to TTL not functioning as we hoped to re-coup unused memory, I don't think we want to throw away in-memory results if we've already paid the memory cost to hold them.

[awrichar]

From a glance back at the docs, it seems like there are two competing elements in this ccache library.

The primary intent is to provide a "least recently used" cache - so it has a fixed max size, and when it's full, the item that was least-recently used is evicted. The docs indicate that items are automatically "promoted" upon Get (by default, only once per every 3 Gets... but can be configured).

Additionally, it provides some separate functionality of attaching an expiry time to an item. This is informational only. It does not cause things to be reaped from the cache automatically, and is never changed as a side effect of Get. You can manually call Extend to change it, and you can inspect it on returned items to decide if you care that they're expired.

[peterbroadhurst]

So in net:

• The original code that calls Extend() on get was "correct" according to the original intent... but
• The assumption that the TTL was happening by magic for us when we set it correctly, was completely invalid 🤷

So TTL in all FireFly caches does not work. They are simply last-access prioritized caches with a maximum size (this library does have another feature that we rely on, which is that we can make that size a byte size, not just a count).

I think that's good to know, and there could be a defect on the backlog to make TTL work.
But I'm not sure we need the change in this PR right now.

[Chengxuan]

Thanks for the explanation. the logic makes sense to me given the design intention is to extend ttl for recent used keys