Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC]: Removal of increment/decrement functionality #117

Closed
boesing opened this issue May 24, 2021 · 4 comments · Fixed by #294
Closed

[RFC]: Removal of increment/decrement functionality #117

boesing opened this issue May 24, 2021 · 4 comments · Fixed by #294
Milestone

Comments

@boesing
Copy link
Member

boesing commented May 24, 2021

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 boesing added the RFC label May 24, 2021
@boesing boesing added this to the 4.0.0 milestone May 24, 2021
@SvenRtbg
Copy link

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.

@boesing
Copy link
Member Author

boesing commented Jun 22, 2023

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
Copy link
Member Author

boesing commented Jan 19, 2024

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): laminas/laminas-cache-storage-adapter-memcached#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 added a commit to boesing/laminas-cache that referenced this issue Feb 28, 2024
As per laminas#117, the increment and decrement functionality (at least decrement) leads to issues with several backends. This commit removes that functionality at all as it should be either part of specific adapters instead (via dedicated interface) or as part of project specific code. This cache component can not guarantee increment/decrement for all adapters out there (especially for adapters which do not support this feature natively to prevent race conditions).

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing linked a pull request Feb 28, 2024 that will close this issue
boesing added a commit to boesing/laminas-cache that referenced this issue Feb 28, 2024
As per laminas#117, the increment and decrement functionality (at least decrement) leads to issues with several backends. This commit removes that functionality at all as it should be either part of specific adapters instead (via dedicated interface) or as part of project specific code. This cache component can not guarantee increment/decrement for all adapters out there (especially for adapters which do not support this feature natively to prevent race conditions).

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
Copy link
Member Author

boesing commented Mar 2, 2024

Removed with #294

@boesing boesing closed this as completed Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants