dirmgr: add Store trait & MemoryStore impl

As discussed in #267 (closed), here goes the Store trait to abstract the caching done in tor-dirmgr. To actually try it out, I've also added a MemoryStore (behind a feature flag) that shows how to select between two Store implementation (and that can be used in wasm!).

• Store trait is mostly all pub fn from SqliteStore, minus the constructors
  • also, removed the use of iterator as dynamic object can't be parametrized per function
• MemoryStore is selected for tests when the feature is activated
• configuration is a bit more complex now, as one can select between two Store impl. I'm sure that you've a nicer way to do it
  • also, it kinda duplicates configuration as arti_client::StorageCacheConfig::Sqlite uses CfgPath but tor_dirmgr::StorageConfig::Sqlite uses PathBuf
  • and sadly derive_builder doesn't supports nested builder, so that less nice than wanted (config.cache(StorageCacheConfig::Sqlite{ directory: CfgPath::new("/tmp".to_owned()) }) vs conf.storage().sqlite("/tmp") )

[storage]
cache = { sqlite = { directory = "${ARTI_CACHE}" } }
# cache = "memory"

NB: this PR is actually not really useful for fixing #267 (closed), as having hooks or a custom DirProvider does the trick, but I coded it, hopefully it'll help you

requested review from @nickm

Integration testing fails because of changed config, but I don't think I can fix it via this PR.

mentioned in issue #267 (closed)

changed the description

Thanks for the patches!

The first commit (creating a Storage trait) looks great. I have a smaller comment abut the third commit (shared ExpirationConfig), which I've left above.

As for the second commit, I have some thoughts about the details, but before I get too far in the review, I'd like to talk about the big picture.

In particular, I'm wondering what application you had in mind for the MemoryStorage. IMO it's a pretty risky feature to provide, since any client that uses MemoryStorage will fetch a complete directory from the network any time it starts, which would put a pretty big burden on the fallback directories. (After all, we cache for a reason!)

Is MemoryStorage something that you see anybody using in real life, or is it there more as an example?

In particular, I'm wondering what application you had in mind for the MemoryStorage.

Is MemoryStorage something that you see anybody using in real life, or is it there more as an example?

It was more meant as an example, to see how it can be selected via configuration. Also, it can be used as a very basic Store impl on platform without filesystem (wasm, no_std, ..), potentially prefilled some other way.

I splitted the commit into two, one abstracting the cache Store impl in the configs, one adding and exposing the MemoryStore. The first one is probably the most relevant to you, because when another "real" Store impl will be developped, parts of the config changes I made here will need to happen anyway.

Anyway, as it was more for me to understand how configs were linked and how to nicely abstract Store, we can remove the MemoryStore without impact on our side.

mentioned in merge request !318 (merged)

added 2 commits

• 47cd1430 - dirmgr: add MemoryStore
• 03b9918c - dirmgr::Store.expire_all takes config

Compare with previous version

resolved all threads

added 4 commits

• 725fd66c - dirmgr: add Store trait
• 5c4d4ad3 - dirmgr: select Store via config
• ab67fe65 - dirmgr: add MemoryStore
• b4276d3a - dirmgr::Store.expire_all takes config

Compare with previous version

Okay. At this point I'd think that the best option is for me to cherry-pick the first and fourth commits on this branch, to improve the Store backend, and make it easier to have other Stores in the future. I think we wouldn't take MemoryStore or the configuration option for now, for the reasons discussed above.

mentioned in merge request !345 (merged)

I've opened !345 (merged) to be a new branch with just those commits, so that CI can test it.

Alright for me, closing this in favor of !345 (merged).

closed