views:

496

answers:

3

I was reading this question (which you do not have to read because I will copy what is there... I just wanted to give show you my inspiration)...

So, if I have a class that counts how many instances were created:

class Foo(object):
  instance_count = 0
  def __init__(self):
    Foo.instance_count += 1

My question is, if I create Foo objects in multiple threads, is instance_count going to be correct? Are class variables safe to modify from multiple threads?

A: 

I would say it is thread-safe, at least on CPython implementation. The GIL will make all your "threads" to run sequentially so they will not be able to mess with your reference count.

Is Foo.instance_count += 1 and atomic unit of work ?
Sam Saffron
Maybe I don't understand how the GIL works... but I still don't see it. Can't Thread1 read instance_count. Then thread1 stops. Thread2 reads instance_count, then stops. Thread1 modifies and writes. Thread2 writes. So you lose an increment? How does the GIL ensure the thread runs through the entire += operation?
Tom
Ha, I basically was asking what Sam Saffron asked just before me.
Tom
@Sam Saffron: I don't get the "atomic unit of work", it's a function call to __iadd__ method, so if you didn't overwrite it yes it is.@Tom: It depends if instance_count is immutable or not. If it is immutable, you will lose the increment, else not (float and integers are immutable).
woops, looks I still need to learn :) Ants Aasma is damn right:http://stackoverflow.com/questions/1072821/is-modifying-a-class-variable-in-python-threadsafe/1073230#1073230
+10  A: 

It's not threadsafe even on CPython. Try this to see for yourself:

import threading

class Foo(object):
    instance_count = 0

def inc_by(n):
    for i in xrange(n):
        Foo.instance_count += 1

threads = [threading.Thread(target=inc_by, args=(100000,)) for thread_nr in xrange(100)]
for thread in threads: thread.start()
for thread in threads: thread.join()

print(Foo.instance_count) # Expected 10M for threadsafe ops, I get around 5M

The reason is that while INPLACE_ADD is atomic under GIL, the attribute is still loaded and store (see dis.dis(Foo.__init__)). Use a lock to serialize the access to the class variable:

Foo.lock = threading.Lock()

def interlocked_inc(n):
    for i in xrange(n):
        with Foo.lock:
            Foo.instance_count += 1

threads = [threading.Thread(target=interlocked_inc, args=(100000,)) for thread_nr in xrange(100)]
for thread in threads: thread.start()
for thread in threads: thread.join()

print(Foo.instance_count)
Ants Aasma
I believe in your second example you want the Thread target to be interlocked_inc instead of inc_by.
tgray
Ants Aasma
Thank you Ants Aasma :-). This is as I suspected. Thank you for proving it to me. As tgray points out, your second target should be interlocked_inc. But once you change that... looks flawless.
Tom
whoops, and I spoke too soon... thanks for fixing that :-).
Tom
+5  A: 

No it is not thread safe. I've faced a similar problem a few days ago and I choosed to implement the lock thanks to a decorator. The benefit is that it makes the code readable:

def threadsafe_function(fn):
    """decorator making sure that the decorated function is thread safe"""
    lock = threading.Lock()
    def new(*args, **kwargs):
        lock.acquire()
        try:
            r = fn(*args, **kwargs)
        except Exception as e:
            raise e
        finally:
            lock.release()
        return r
    return new

class X:
    var = 0

    @threadsafe_function     
    def inc_var(self):
        X.var += 1    
        return X.var
luc
that's a useful decorator :)
Geo
off-topic, but can you remove the two lock.release() calls to a "else:" section after the exception handler?
Jeffrey Kemp
Do you mean in a finally section? Doing that in an else would not release when an exception is raised
luc
ah yes, that's what I meant. thanks!
Jeffrey Kemp