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

async_creation_runners broken by v0.7.0 #139

Closed
goodspark opened this Issue Dec 11, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@goodspark
Copy link

goodspark commented Dec 11, 2018

Specifically, the changes to pull out creator_args from the creator function.

Here's a minimal test expanded from the current documentation:

from dogpile.cache import *
import threading
import time

def async_creation_runner(cache, somekey, creator, mutex):
    ''' Used by dogpile.core:Lock when appropriate  '''
    def runner():
        try:
            value = creator()
            cache.set(somekey, value)
        finally:
            mutex.release()

    thread = threading.Thread(target=runner)
    thread.start()

region = make_region(
    async_creation_runner=async_creation_runner,
).configure(
    'dogpile.cache.memcached',
    expiration_time=1,
    arguments={
        'url': '127.0.0.1:11211',
        'distributed_lock': True,
    }
)

@region.cache_on_arguments()
def something(x):
    return 2 * x

assert something(1) == 2
time.sleep(2)
# This call will fail
assert something(1) == 2

The last call fails with:

<decorator-gen-114> in something(x)

/home/test/.tox/py27/lib/python2.7/site-packages/dogpile/cache/region.pyc in get_or_create_for_user_func(key_generator, user_func, *arg, **kw)
   1270                 else expiration_time
   1271             return self.get_or_create(key, user_func, timeout,
-> 1272                                       should_cache_fn, (arg, kw))
   1273 
   1274         def cache_decorator(user_func):

/home/test/.tox/py27/lib/python2.7/site-packages/dogpile/cache/region.pyc in get_or_create(self, key, creator, expiration_time, should_cache_fn, creator_args)
    877                 get_value,
    878                 expiration_time,
--> 879                 async_creator) as value:
    880             return value
    881 

/home/test/.tox/py27/lib/python2.7/site-packages/dogpile/lock.pyc in __enter__(self)
    184 
    185     def __enter__(self):
--> 186         return self._enter()
    187 
    188     def __exit__(self, type, value, traceback):

/home/test/.tox/py27/lib/python2.7/site-packages/dogpile/lock.pyc in _enter(self)
     91             createdtime = -1
     92 
---> 93         generated = self._enter_create(value, createdtime)
     94 
     95         if generated is not NOT_REGENERATED:

/home/test/.tox/py27/lib/python2.7/site-packages/dogpile/lock.pyc in _enter_create(self, value, createdtime)
    164 
    165                 # so...run it!
--> 166                 self.async_creator(self.mutex)
    167                 _async = True
    168 

/home/test/.tox/py27/lib/python2.7/site-packages/dogpile/cache/region.pyc in async_creator(mutex)
    865                     return self.async_creation_runner(
    866                         self, orig_key, creator, mutex,
--> 867                         creator_args=creator_args)
    868                 else:
    869                     return self.async_creation_runner(

TypeError: async_creation_runner() got an unexpected keyword argument 'creator_args

Basically #136 broke backwards compatibility. I'm not sure if dogpile uses semver, but if so, then it should probably have a major version increment now.

The fix is trivial, but the documentation should also be updated:

def async_creation_runner(cache, somekey, creator, mutex, creator_args=None):
    ''' Used by dogpile.core:Lock when appropriate  '''
    def runner():
        try:
            if creator_args:
                value = creator(*creator_args[0], **creator_args[1])
            else:
                value = creator()
            cache.set(somekey, value)
        finally:
            mutex.release()

    thread = threading.Thread(target=runner)
    thread.start()
@zzzeek

This comment has been minimized.

Copy link
Member

zzzeek commented Dec 12, 2018

I dont use semver for any of my projects since I can't isolate releases to be only features or only bugfixes, so the .0 at the end would never be used and the major version would be huge.

the async runner change is a mistake since I don't use that feature I forgot that people would be using it with decorators.

the only solution I can see for now is to revert the change.

@zzzeek

This comment has been minimized.

Copy link
Member

zzzeek commented Dec 12, 2018

part of the change, at least. i have no interest in breaking compatibility.

@sqlalchemy-bot

This comment has been minimized.

Copy link

sqlalchemy-bot commented Dec 12, 2018

Mike Bayer has proposed a fix for this issue:

Restore the API for async_creation_runner in all cases https://gerrit.sqlalchemy.org/1010

@zzzeek

This comment has been minimized.

Copy link
Member

zzzeek commented Dec 12, 2018

when that review passes, it's going to be release 0.7.1 tonight. if would be very helpful if you could take a review of it. added some tests for async_creation_runner which was obviously a problem as well.

@zzzeek

This comment has been minimized.

Copy link
Member

zzzeek commented Dec 12, 2018

it's ready to go. I'd like to release right now.

@goodspark

This comment has been minimized.

Copy link
Author

goodspark commented Dec 12, 2018

Looks good! Thank you!

@zzzeek

This comment has been minimized.

Copy link
Member

zzzeek commented Dec 12, 2018

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment