feat: add a feature flag to enable the cache

[tirz]

Sometime, we may already have a cache strategy with varnish or squid.
In that case, the cache offered by libreddit will never hit and just slow down the request (hashmap check from the cached crate) and use more ram for nothing.

[sigaloid]

Once we merge with Ferrit, we might have a better option - a runtime configuration option rather than a compile-time configuration.

#608

[tirz]

I don't think Ferrit is able to disable the cache right now : https://github.com/ferritreader/ferrit/blob/master/src/client.rs#L217

Are you talking about adding a key to https://github.com/ferritreader/ferrit/blob/master/src/settings.rs ?

Anyway, I think that having a compile-time flag may be better here since we should not need to toggle the cache on and off.
A runtime config is still a bonus for when libreddit was compiled with the cache flag enabled.

[Daniel-Valentine]

ping @spikecodes

I want agree with @tirz. I can't see any reason why a host would want to disable the cache on a Libreddit release binary, nor one for offering that feature as a command-line option. The only people who would want to toggle the cache are devs.

However, there's a problem: you can't disable an arbitrary default feature (at least not yet). Either all of them are honored, or none of them are if --no-default-features is given to a Cargo subcommands. That would be okay if it were just this feature, but we might have more default features in the future which we don't want to disable just to toggle one of them.

So, I propose a compromise between @tirz's and @sigaloid's positions. Make the cache toggle-able but only if the build is a debug build. This toggle can be either by way of an environment variable or a command-line option. I think an environment variable would be better.

Thoughts?

[spikecodes]

I support this feature and I think your compromise would be a great way of adding it @Daniel-Valentine though I'm not sure how we could make the cached attribute conditional to the existence of an environment variable as cfg can't read from env vars AFAIK.

[Daniel-Valentine]

@spikecodes:

I'm not sure how we could make the cached attribute conditional to the existence of an environment variable as cfg can't read from env vars AFAIK.

That's true. And because it's a attribute that we can't change at runtime, a command-line option won't work either -- I had just gotten a little confused.

The more that I think about it, the more that @tirz's is the way to go. Another option is to make a build.rs that checks the environment and, if our target is a development build, enables all default features by hand except for cached.

[sigaloid]

I agree - honestly, I feel like with a development build you wouldn't want caching on in general - it won't be facing high load and it would be hard for someone to manually deploy a public instance as a debug build. Should we consider disabling caching for debug builds in general?

[Daniel-Valentine]

@sigaloid: I'd advocate for keeping caching on in development. If we make improvements to HTTP compression in the future (if #612 goes in) then we certainly want to test for performance gains or losses against the cache.

[sigaloid]

That's true.

So we want:

• Caching on in release builds
• Caching on in debug builds, but optionally disable-able

It's bad form to make a feature that disables a feature (like disable-cache) and there is a slight problem with making it default as someone would need to disable all default features, and as mentioned we may add more which would add complexity about disabling default then re-enabling all other features.

We could split it into two different functions, and optionally call them based off of runtime/environment variables. A build.rs could work as well. Should that be implemented before merging this? Or once we actually add another feature justifying it?

[tirz]

So we want:

* Caching on in release builds

Personally, I do not want to cache the responses for the release build since my infra already have a cache with varnish (in front of libreddit).