Skip to content

Loading…

Better multithreading support #6

Closed
Tinche opened this Issue · 6 comments

2 participants

@Tinche

Example at https://gist.github.com/Tinche/e092adeb8171fd0843dd will print out "Cached method called 10 times" where ideally it would only be called once.

I'd suggest documenting this fact or doing something about it :)

@pydanny
Owner

I've confirmed this with both Python 3.3 and Python 2.7. My modified code, done so it works in both major implementations of Python AND displays that the result of Test.val is 15:

"""Cached property multithreading test."""
from threading import Lock, Thread
from time import sleep
from cached_property import cached_property

COUNT = 0
COUNT_LOCK = Lock()
NUM_THREADS = 10

class Test(object):
    def __init__(self, val):
        self._val = val

    @cached_property
    def val(self):
        """Val getter."""
        sleep(1)
        global COUNT_LOCK, COUNT
        with COUNT_LOCK:
            COUNT = COUNT + 1
        self._val += 1
        return self._val

t = Test(5)
threads = []
for _ in range(NUM_THREADS):
    thread = Thread(target=lambda: t.val)
    thread.start()
    threads.append(thread)

for th in threads:
    th.join()

msg = "Cached method called {} times".format(COUNT)
print(msg)
print(t.val)
@pydanny pydanny added bug enhancement and removed bug labels
@pydanny
Owner

Please see the test case of https://github.com/pydanny/cached-property/blob/master/tests/test_cached_property.py#L86-L110

The test is currently commented out because currently it fails. Instead of c.add_cached returning 1 during the test, it returns 1.

@Tinche

Just using a threading.Lock inside cached_property.get would work and impose a very slight performance dip in a non-threaded scenario, since in a non-threaded scenario that method would be called once before shadowing itself in obj.dict, and the lock would not be contended. I could put something like this together in a pull request.

Another thing to keep in mind is asyncio (i.e. pep 3156). Ideally the cached_property decorator would transparently work in the async context as well. I don't actually know how to make it 'just work' in both contexts (threading and async) automatically, though.

Maybe offer specialized versions of the decorator for different circumstances? Or not worry about thread/coroutine safety at all, document it and let the users deal with locking themselves.

@pydanny
Owner

@Tinche A pull request would be awesome. My shameful secret is that my experience with working with threads is limited. My experience with locking them is non-existent. I would love to see how you implement this, as it would kickstart my knowledge of them.

As for implementation, I think a second decorator, possibly called threaded_cached_property is the way to go. That way we can keep the standard approach small and easily tested. Eventually, as threaded_cached_property becomes more battle tested, we might just alias cached_property to it.

asyncio is great idea. I've opened #7 for work on it.

@Tinche

Here's a pull request with a thread-safe version: #9

I have to admit my concurrency experience is substantial but largely related to the JVM, so while I'm pretty sure my implementation is sound there might be a more efficient/elegant way of implementing the same idea in Python I'm not aware of.

I'm open for any questions or comments of course :)

@pydanny
Owner

Looks good to me! Delivered in the 0.1.5 release. Thanks!

Closed by #9

@pydanny pydanny closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.