Fix memory leak in get_with and friend methods of future::Cache

[tatsuya6502]

This PR fixes #329, a bug introduced to v0.12.0 causing memory leak in get_with(), entry().or_insert_with() and friend methods in future::Cache.

It also bumps the crate version to v0.12.1.

Details of the bug

The bug was introduced by PR #294 for v0.12.0 (src/future/value_initializer.rs diff L316-L330). So this PR basically reverts the change.

Here are some backgrounds. An instance of future::Cache has a few internal concurrent hash tables (cht), and the following tables are related to this bug:

1. The main cht called cache map, which stores cached entries.
2. A cht called waiter map, which stores per-key reader-writer locks used by get_with and friends methods.

The key of cache map is Arc<K>, and the key of waiter map is (Arc<K>, TypeId). These internal maps take a hash function at creation, and both cache map and waiter map are given the same hash function.

Also, these maps take hash value for the key when inserting, getting, or removing an entry (a key-value pair). The hash value is supposed to be calculated from the key by the hash function. However, the bug made get_with etc. to use a hash value for cache map for accessing waiter map. Since their keys are different, the hash value are also different.

Using a wrong hash value still works as long as waiter map is using the same memory region where the entry was inserted to. However, once the memory region gets half full with live entries and deleted keys, waiter map will allocate a new memory region and move live entries to it, using the correct hash values for the keys. Once this happens, get_with etc. (who use wrong hash) cannot find entries in waiter map anymore, leaving them in the map forever. (memory leak)

[Swatinem]

Thanks for finding and fixing this issue so quickly.

From an outsiders perspective, I’m wondering why you are passing these two key/hash pairs explicitly, instead of maybe wrapping them in two different newtypes, so the typesystem enforced that you are using the correct key/hash pair.

I’m also curious why it is necessary to use a TypeId here as well in the first place? I will have to dig a bit deeper and learn more of the inner working of moka :-)

[tatsuya6502]

From an outsiders perspective, I’m wondering why you are passing these two key/hash pairs explicitly, instead of maybe wrapping them in two different newtypes, so the typesystem enforced that you are using the correct key/hash pair.

Hi. Thank you for the suggestion. Maybe we can change the hash type from bare u64 to something like HashVal<K>(u64, PhantomData<K>) where K is the key type of cht? I will think about it when I have more time.

The reason that we are passing hash explicitly is that a Cache instance has multiple internal data structures using the same hash, and we want to avoid to calculate the hash more than once.

For example, moka::sync::SegmentedCache uses the same hash for a key in the following purposes:

• To select a Cache segment to store the entry.
• To select a hash table segment in the cht (moka::cht::SegmentedHashMap)
• To select a hash table bucket in the hash table segment.
• To set or get counter bits in the popularity estimator (FrequencySketch).

I’m also curious why it is necessary to use a TypeId here as well in the first place?

You may have already seen the answer when creating other pull request, but TypeId is for the E of the return type Result<V, Arc<E>> of the or_try_insert_with method. This TypeId is used in a key to group together the or_try_insert_with calls having the same E.