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

Default Redis Locking (thread locals) Incompatible with Async Creators #171

Closed
goodspark opened this issue Feb 4, 2020 · 9 comments
Closed
Labels

Comments

@goodspark
Copy link
Contributor

@goodspark goodspark commented Feb 4, 2020

Python 3.7
dogpile 0.9.0
redis 3.4.1

The redis backend ends up using the redis Lock object to create distributed locks.

Note that by default Redis' Lock objects use thread locals to store locking tokens.

But if a region is configured to use async creation runners, then the lock that's passed to the background thread will not be able to release the lock because the thread local token doesn't exist.

Perhaps lock creation should pass an additional param to signify the lock will be shared across threads? Or a custom Redis lock that wraps the main one to handle this. Yet another option is to have dogpile itself deal with the lock code (thus within the same thread as the lock's creation).

Minimal example:

#!/usr/bin/env python3
from concurrent.futures import ThreadPoolExecutor
from dogpile.cache import make_region
import logging
import time

logging.basicConfig()
logging.getLogger().setLevel(logging.DEBUG)
log = logging.getLogger(__name__)
pool = ThreadPoolExecutor(max_workers=1)

def asyncer(cache, key, creator, mutex):
    def _call():
        log.info("async creator")
        try:
            value = creator()
            cache.set(key, value)
        finally:
            try:
                mutex.release()
            except Exception:
                log.exception("uhoh")
    return pool.submit(_call)

cache = make_region(async_creation_runner=asyncer)
cache.configure(
    "dogpile.cache.redis",
    expiration_time=2,
    arguments={
        # Local redis instance in Docker
        "url": "redis://redis:6379",
        "distributed_lock": True,
        "lock_timeout": 0.1,
    },
)

@cache.cache_on_arguments()
def doubler(x):
    log.info("recompute: %s", x)
    return 2 * x

log.info(doubler(4))
time.sleep(3)
log.info(doubler(4))
DEBUG:dogpile.cache.region:No value present for key: '__main__:doubler|4'
DEBUG:dogpile.lock:NeedRegenerationException
DEBUG:dogpile.lock:no value, waiting for create lock
DEBUG:dogpile.lock:value creation lock <redis.lock.Lock object at 0x7f857abc4c18> acquired
DEBUG:dogpile.cache.region:No value present for key: '__main__:doubler|4'
DEBUG:dogpile.lock:Calling creation function for not-yet-present value
INFO:__main__:recompute: 4
DEBUG:dogpile.cache.region:Cache value generated in 0.000 seconds for key(s): '__main__:doubler|4'
DEBUG:dogpile.lock:Released creation lock
INFO:__main__:8
DEBUG:dogpile.lock:value creation lock <redis.lock.Lock object at 0x7f857abc4c18> acquired
DEBUG:dogpile.lock:Passing creation lock to async runner
INFO:__main__:async creator
INFO:__main__:recompute: 4
INFO:__main__:8
ERROR:__main__:uhoh
Traceback (most recent call last):
  File "./code.py", line 20, in _call
    mutex.release()
  File "/home/sam/tmp/py37/.tox/python/lib/python3.7/site-packages/redis/lock.py", line 222, in release
    expected_token = self.local.token
AttributeError: '_thread._local' object has no attribute 'token'
@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 4, 2020

would exposing the Redis lock "thread_local" parameter be sufficient ?

@goodspark

This comment has been minimized.

Copy link
Contributor Author

@goodspark goodspark commented Feb 4, 2020

Yeah, but it might be better to have dogpile 'handle' it so that people can't configure themselves into using async creators with thread local locking. iow: dogpile can use thread local locking for synchronous creations and then disable it when creating locks for asynchronous flows. Or perhaps always disable thread locals? Because the tokens used are per key, not for global locking.

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 4, 2020

Yeah, but it might be better to have dogpile 'handle' it so that people can't configure themselves into using async creators with thread local locking.

It's already news to me, since I dont pay attention to redis, that their lock uses a thread local. so they might change this later. i would be OK emitting a warning but assuming redis' use of this thread local is somehow required, I'd rather not guess our way around a changing set of third party behaviors.

iow: dogpile can use thread local locking for synchronous creations and then disable it when creating locks for asynchronous flows. Or perhaps always disable thread locals? Because the tokens used are per key, not for global locking.

is that why they are using a threadlocal ? this really seems like a bug on their end, that's crazy? of course, if the lock doesnt need this behavior we should just turn it off, sure.

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 4, 2020

cc @jvanasco since I think you follow redis etc. any insight on this?

@goodspark

This comment has been minimized.

Copy link
Contributor Author

@goodspark goodspark commented Feb 4, 2020

Here's the Python client's docs on the purpose of the thread local implementation:
https://redis-py.readthedocs.io/en/latest/#redis.Redis.lock

time: 0, thread-1 acquires `my-lock`, with a timeout of 5 seconds.
         thread-1 sets the token to "abc"
time: 1, thread-2 blocks trying to acquire `my-lock` using the
         Lock instance.
time: 5, thread-1 has not yet completed. redis expires the lock
         key.
time: 5, thread-2 acquired `my-lock` now that it's available.
         thread-2 sets the token to "xyz"
time: 6, thread-1 finishes its work and calls release(). if the
         token is *not* stored in thread local storage, then
         thread-1 would see the token value as "xyz" and would be
         able to successfully release the thread-2's lock.

I think in this example, a single lock is being used to coordinate activity for multiple tokens (1 'thread global' lock for all cache key being recomputed in said thread). Whereas in dogpile's case with asynchronous creators, it's more like multiple locks are being used (1 per key) for 1 token.

And at the end of that function's doc, we have:

For example, if you have code where one thread acquires a lock and passes that lock instance to a worker thread to release later. ... Our assumption is that these cases aren’t common and as such default to using thread local storage.

I also don't follow redis actively so insight and corrections are more than welcome here.

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 4, 2020

IMO that's a pretty broken API. the lock should generate a unique token for the acquire() operation, return it to the caller, then the caller makes use of that token when they call release(). this is basically what they are doing with thread local thing now, just that it's hardcoded to the current thread id. it should not be linked to the thread, it should be linked to the caller.

@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Feb 4, 2020

see I'd want redis.py to support this case more explciitly. their note "these cases aren't common so it will just break" is not really good enough

goodspark added a commit to goodspark/dogpile.cache that referenced this issue Mar 11, 2020
Resolves sqlalchemy#171
@zzzeek

This comment has been minimized.

Copy link
Member

@zzzeek zzzeek commented Mar 13, 2020

ill add a warning

@sqla-tester

This comment has been minimized.

Copy link
Collaborator

@sqla-tester sqla-tester commented Mar 13, 2020

Sam Park has proposed a fix for this issue in the master branch:

Add option for thread local Redis locks https://gerrit.sqlalchemy.org/c/sqlalchemy/dogpile.cache/+/1783

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.