laminas-cache icon indicating copy to clipboard operation
laminas-cache copied to clipboard

[RFC]: Removal of increment/decrement functionality

Open boesing opened this issue 4 years ago • 1 comments

RFC

Q A
Proposed Version(s) 4.0.0
BC Break? Yes

Goal

It seems that some storage backends (redis for example) does not preserve type-safety when storing values. Incrementing and decrementing of cache items is therefore mostly done by this library by reading the value, Incrementing/decrementing the value and then it is written back to the storage. This could also be done in userland code and/or by a decorator and thus does not need a requirement for a storage anymore.

Background

Recently, there were 2 bugs discovered when using the serializer plugin in combination with decrement/increment which were undetected for around 9 years.

Considerations

Providing a decorator which consumes the storage along with the key & amount could provide the same functionality without having every storage do handle it by itself.

Proposal(s)

Thus said, I would mark that whole functionality deprecated in 3.0 and remove it in 4.0

Decorator to increment/decrement could be created in 3.0 if needed. The whole functionality could be done in userland without having to use a decorator at all and thus the decorator could also be just an example in the docs.

Appendix

Bugs mentioned in this RFC which are related to this functionality: #116 #115

boesing avatar May 24 '21 18:05 boesing

I support the proposal, adding some important thought: What is a cache increment/decrement supposed to do? Add or subtract 1 from a value. Having 1000 concurrent requests incrementing a value better ends up having the value incremented by 1000, but concurrency and some backend's inability to provide this operation atomically will prevent it from happening.

Several backends like APC(u), Memcache(d), Redis do provide an atomic increment operation on the server, which can be triggered, and I am confident the result with concurrent requests will be correct. Using the filesystem, MongoDB or DBA backend however, this will definitely fail. Or would require some locking mechanism to be available to prevent other processes to access the value being worked on, but not all backends even support them natively. Locking also is alien to a caching layer, it is supposed to provide write-once-read-many operations to speed up things, not make requests wait for each other.

If software requires distributed access to counters, it should use dedicated access layers for this task - not using a generic caching layer with interesting additional features. Requiring reliable counters probably means you have to familiarize yourself with the counter providing software anyways.

SvenRtbg avatar Jun 11 '21 10:06 SvenRtbg

Moving this to a dedicated interface might be the way to go. Adapters which have native increment/decrement functionality (such as redis) can implement that interface while those which do depend on reading the value, incrementing the value in runtime and overwriting the value (which might lead to race conditions) do not implement that interface anymore.

As of now it is implicitly provided by the AbstractAdapter and thus, this functionality will be removed with v4.0.0

boesing avatar Jun 22 '23 20:06 boesing

Another little "fun-fact" is that there are adapters where decrement stops decrementing when reaching 0 (so there is no way of having negative integers): https://github.com/laminas/laminas-cache-storage-adapter-memcached/issues/17

I really tend to remove this feature. If projects really need this, they might want to implement this on their own which can be done with a small decorator.

boesing avatar Jan 19 '24 21:01 boesing

Removed with #294

boesing avatar Mar 02 '24 22:03 boesing