Make return value of Cache::Store#write consistent

[Mangara]

Motivation / Background

I'm trying to use memcached to de-duplicate some work. When I start the work, I want to write a key with a 5s TTL and if the key already exists, someone else is already doing the work and started at most 5s ago, so I don't have to start it again. To avoid a race condition, I'd like to use the unless_exist: true option for this. If write(..., unless_exist: true) returns false, the key was already there and we don't need to do the work.

The only problem with this approach is that the write method also returns false if there is some error with memcached. But in that case I do want to do the work, to ensure it gets done at least once.

Detail

The return value of Cache::Store#write was not specified before, and varied between backends. This PR makes it consistent:

• true indicates a successful write
• nil indicates an error talking to the cache backend
• false indicates a write that failed for another reason

This lets us distinguish the case where the key already exists from the case where there was a memcached (or Redis) error.

Additional information

I only changed the implementation of the Redis and memcached stores. From CI, it looks like all others already followed this pattern?

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Tests are added or updated if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

[byroot]

Does it make sense to add this to more / all cache stores?

We generally try to avoid backend specific capabilities yes. Ideally you should be able to easily swap your cache backend. (it's never perfect of course)

[Mangara]

I implemented it for all cache stores that use the FailureSafetyBehavior test suite, which is Redis and memcached.
Ready for another look.

[casperisfine]

Sorry for dragging my feets on this, I got quite busy lately, and also I can't really put my finger on it, but I'm not super fan of this change. Can't really explain why though 😞 .

Perhaps because that fallback option is specific to write, and I fear it will creep up.

What if we returned nil on failure and false when the command worked, but didn't lead to a write?

[Mangara]

What if we returned nil on failure and false when the command worked, but didn't lead to a write?

That would work for me. I'll see what that looks like.

[byroot]

Some very minor thing, other than that LGTM.