Eagerly check for store validity

[byroot]

Ref: #315

respond_to? is a relatively expensive method that doesn't benefit from inline caching, so it has to keep looking up the method every time. Hence it's best to avoid it in hotspots.

In this case, @store can only change through the store= method, so we might as well eagerly validate it there.

First because this way it's a one time fixed cost, but also because this way the error is raised early during boot/configuration rather than at runtime where it has availability impact.

[santib]

Makes sense! Thanks for the contribution

[byroot]

Hum, CI failure is legit, gonna dig into it.

[byroot]

Should go green now. (sorry, I couldn't run the tests on 4.0, so I did a quick change, now I fixed the 4.0 compat issue and added it to the matrix).

[santib]

Changes look good to me, but now I realize that we were allowing stores that partially complied with the store interface, and now we are checking it upfront.

cc @grzuy do you think this could be a problem? Maybe for example someone not using some features (e.g. fail2ban) don't need to implement all of the methods? Should we consider this change to be backwards incompatible?

[byroot]

If eagerly checking ins't possible, then i'd recommend just not checking at all.

At the end of the day, NoMethodError: undefined method 'read' for for an instance of MyCacheStore isn't that bad of an error to understand, and all this extra checking isn't that helpful?

[grzuy]

Overall this sounds like a good idea.

cc @grzuy do you think this could be a problem? Maybe for example someone not using some features (e.g. fail2ban) don't need to implement all of the methods? Should we consider this change to be backwards incompatible?

Yeah, good call.
Maybe it can unnecessarily error for someone not actually needing to implement some method...

If eagerly checking ins't possible, then i'd recommend just not checking at all.

At the end of the day, NoMethodError: undefined method 'read' for for an instance of MyCacheStore isn't that bad of an error to understand, and all this extra checking isn't that helpful?

Yeah...

What if if only eagerly check and warn (not error) on missing methods?

[byroot]

What if if only eagerly check and warn (not error) on missing methods?

So I'm not that familiar with how the gem is supposed to be used, I only recently joined a company using it.

My reasoning would be:

• Ideally fail during boot/config, that prevent raising error in prod, which means a 500, which means user disruption.
• A warning doesn't prevent such mistake assuming the store in prod and in dev are different.

But ultimately i'm no maintainer, so happy to change the PR to what you think is best.