Should ActionLoop._dispatch_action be locked?

[robnagler]

The lock is on object life cycle. If destroy is called during dispatch (e.g. and exception), the thread will deadlock (or get an exception?). Since the lock is only about the object life cycle, it doesn't really help the actions that are dispatched to be locked. The thread enforces only one action is called at a time and in order in the queue. Another option is that destroy is handled by the thread, which makes more sense from an ordering issue.

[eloise-nebula]

If the lock is on the object's lifecycle, then its current implementation is overloaded. _dispatch_action and destroy cannot happen at the same time, meaning ActionLoop protects its own state. Presumably subclasses use _destroy in a way that could result in race conditions if not locked. The queue should be solely responsible for preventing race conditions by ordering, and the lock should be the same for managing object lifecycle.

In view of this, _dispatch_action should happen outside the lock, and the thread should handle destroy, but destroy should still immediately set the lifecycle flag destroyed. ActionLoop would only touch the the lock when dealing with destroyed. Calling _destroy() upon exiting the loop would be an easy way to implement this.

There's one caveat. The lifecycle flag destroyed is currently a public variable, meaning that any ActionLoop subclass or caller can see/change the lifecycle at any time with or without a lock. ActionLoop should restrict access to this flag if it's supposed to be solely responsible for its lifecycle. destroyed should be moved to __destroyed, and ActionLoop should provide the following for checking its lifecycle externally:

    def is_destroyed(self):
        with self.__lock:
            return self.__destroyed

Then, destroy would become:

    def destroy(self):
        try:
            with self.__lock:
                if self.__destroyed:
                    return
                self.__destroyed = True

[robnagler]

The check for destroy can technically happen outside the lock, because it is getting a value and Python (even without the GIL) will either set a value or not. Writing destroy has to happen in the lock. Reading destroyed doesn't necessarily have to happen, since setting destroyed is not actually destroying the object and all actions (including destroy) are ordered inside the thread.

[robnagler]

You analysis is otherwise spot on. :)