[Lock] [RFC] Deprecate/Remove Memcached Store #22452

patrick-mcdougle · 2017-04-16T23:01:47Z

Q	A
Bug report?	no
Feature request?	no
BC Break report?	no
RFC?	yes
Symfony version	N/A

Can we eliminate the memcached store of the lock component? Memcached is notorious for evicting keys that aren't even to their exptime because they are LRU (less recently used) than something else in the same slab class. By having the memcached store in the lock component, developers will trust it more than they should. It's great as a cache, but really shouldn't be used as a persistent concurrent locking mechanism. Thoughts?

Reference: #21093

nicolas-grekas · 2017-04-17T20:07:41Z

ping @jderusse

jderusse · 2017-04-17T20:34:11Z

Other links to usage of Memcached : https://github.com/memcached/memcached/wiki/ProgrammingTricks#ghetto-central-locking

My opinion about memcached is to not use it at all (included session, cache, and everything else), and to use redis instead. But because some people don't have choice, I wrote this Store.

I wonder if we should provide a MemcachedStore as a fallback solution with a big warning in documentation. Beca 8000 use if the memcached server is not overused (the size of stored elements since the lock acquisition is lower than the available memory) then the situation is OK.

@core-team Should symfony provide only 100% reliable tools? or can we provide weak tools with pointer in documentation defining "How to use safely the tool"?

fabpot · 2017-04-17T20:39:26Z

If Memcached is widely used, then we could provide support with a big warning upfront. But is it really the case? Is memcached widely used nowadays?

jderusse · 2017-04-18T05:04:26Z

Given this trend comparison https://db-engines.com/en/ranking_trend/key-value+store Redis is 5 time more used than Memcached.

Do you know the usage of Memcached in the Cache component (ping @nicolas-grekas)?

I may create a quick poll on Twitter but I'm not sure to collect enough responses from representative users to get an efficient result.

fabpot · 2017-04-18T22:13:33Z

I don't think we have hard numbers on the usage of the Cache component (because we don't track that and we cannot, but also because the component is relatively new).

Pool created: https://twitter.com/fabpot/status/854457738713210880

robfrawley · 2017-04-18T23:06:34Z

Yes: it is used, especially in legacy or enterprise applications. Also of note, Memcached can easily be configured as to the max memory, as well as the ability to error when memory is exhausted instead of use the LRU algorithm. Our assumption should be people understand the configuration of whatever store they are using and have configured it appropriately for their circumstances. Why would we remove a perfectly functional store implementation?

lolautruche · 2017-04-19T06:14:37Z

It's widely used by eZ users. We try to encourage Redis but IT people tend to consider it as "to new".
I think it should be supported, with a warning as @jderusse suggests

nicolas-grekas · 2017-04-19T06:32:45Z

Let's deprecate it now, and remove it either in 4.0 or 5.0.
That will allow people to see the warning with the existing reports, and will still make us maintain it for a few years.

robfrawley · 2017-04-19T18:39:35Z

Do we also remove the Redis lock because "[using it] the locks are only approximate and may occasionally fail [1]"? Obviously, the answer is no: people should simply be aware of the drawbacks of their selected store engine. Memcached is no different. Given @fabpot's Twitter poll it looks like ~41% of people (or ~150 people polled) want it to remain, as well.

[1] https://martin.kleppmann.com/2016/02/08/how-to-do-distributed-locking.html

jderusse · 2017-04-20T10:02:43Z

First of all, if you use lock for efficiency (ie. to avoid dog-piling) and admit concurrency in rare cases this is fine
if you want 100% reliable lock
and if you configure your infrastructure (see bellow)
and if you are aware of TTL sensitivity this is fine

Second point: A cluster of nodes (or distributed) won't provide stronger locks: A cluster provide an higher availability. BUT If only one of the nodes is not reliable, then the entier pool is not reliable

After re-reading the http://martin.kleppmann.com/2016/02/08/how-to-do-distributed-locking.html and the rebbutal http://antirez.com/news/101 and the discussion about the 2 blog posts https://news.ycombinator.com/item?id=11065933
The 2 reasons why Martin says redlock is not 100% reliable

An expired lock may be used (because of network delay or the application is too slower or whatever...) => The choice of wise TTL is already addressed in the documentation. I'll also implement PR to improve this part (see the end of this comment)
Redis may release lock sooner than expected if the clock is changed => This can be adressed by a NTP configuration as suggested by antirez

There is actually 3 questionable store implementations using expiable lock:

MemcachedStore

The server may remove locks by itself (LRU evictions) => can be disabled or tell user to not overuse the server or to use a dedicated instance for lock purpose
The server may crash/restart => ? how to deal with it ? need to document a suggestion like delayed start

RedisStore

The server may remove lock after clock change => configure ntp as suggested by antirez
The server may crash/restart before persisting on disk => configure redis to delay start or use fsync

CombinedStore / Redlock

Nothing in articles point an issue with the algorithm itself, every points (expired lock, clock change, ...) are applicable to the node itself.

One word about RedisCluster vs CombinedStore

for the record, CombinedStore allow you ot use a pool of stores instead of a single one to improve avaibility.
CombinedStore is not only limited to Redis, but can be use with memcached, futur stores or to mix stores
RedisCluster need to sync write in nodes to improve consistency. I'll create a PR to fix that

Should we deprecate?
MemcachedStore 40% of voter thinks we shouldn't. But I suggest to display a big warning about memcached consistency
RedisStore I suggest to provide documentation about how to configure infrastructure to support more robust locks
CombinedStore no

I summary, I suggest to add documentation about the limitations;

in symfony-docs
in classes dockblock
about:
- configure a redis (delay restart or fsync)
- configure a memcached (disable LRU, delayed restart)
- configure infra (smooth ntp adjustement)
- configure app/symfony
  - small redis connexion timeout (to reduce time to acquire locks)
  - be sure to define a correct TTL

I postponed the implementation of time checking during lock acquiring and overlook the delay to acquire the lock in servers. I will fix that in nexts days with new PR:

Provide public methods isExpired and expireAt in the class Lock (to let the user know the remaining time)
Check expiration after acquiring the lock (in the Lock class and affected stores)
Implement the WAIT command in redisCluster

To fit the need of 100% reliable and consitence lock, we may provide an other Store (ie. ZooKeeper like suggested in the martin's article).

patrick-mcdougle · 2017-04-22T16:19:42Z

I'm all for using our documentation to educate/guide the proper use cases for such technologies. I guess a deprecation/removal might be an aggressive play (since 40% of people want it). Might as well keep it with the stipulation that we add these gotcha's to the documentation.

Thanks for the conversation 😄

…Lock (jderusse) This PR was squashed before being merged into the 3.4 branch (closes #22543). Discussion ---------- [Lock] Expose an expiringDate and isExpired method in Lock | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22452 | License | MIT | Doc PR | NA This PR store the expiration date of the lock in the key and exposes public method to let the user know the state of the Lock expiration. Commits ------- 2794308 [Lock] Expose an expiringDate and isExpired method in Lock

…sse) This PR was squashed before being merged into the 3.4 branch (closes #22542). Discussion ---------- [Lock] Check TTL expiration in lock acquisition | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22452 | License | MIT | Doc PR | NA This PR improve lock acquisition by throwing exception when the TTL is expired during the lock process. Commits ------- f74ef35 [Lock] Check TTL expiration in lock acquisition

xabbuh added Lock RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Apr 18, 2017

This was referenced Apr 26, 2017

[Lock] Check TTL expiration in lock acquisition #22542

Closed

[Lock] Expose an expiringDate and isExpired method in Lock #22543

Closed

jderusse mentioned this issue Aug 18, 2017

[Lock] Re-add the Lock component in 3.4 symfony/symfony-docs#7866

Merged

fabpot closed this as completed Sep 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Lock] [RFC] Deprecate/Remove Memcached Store #22452

[Lock] [RFC] Deprecate/Remove Memcached Store #22452

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!