Possibly add a reentrant lock

[bob1de]

Hi trio team,

First, thanks for this great and well-designed library!

I don't know whether this has already been discussed, but at least googling didn't led me to an existing issue or something.

It sometimes comes handy to have reentrant locks like they're offered by threading. I know that everything that can be achieved with reentrant locks can also be done without them, but it can make code paths easier and stop people from adding separate _nolock methods all over the place.

I currently use something like this:

@attr.s(cmp=False, hash=False, repr=False)
class RLock(trio.Lock):
    """
    A reentrant Lock. It can be acquired multiple times by the same task. Other than
    that, its API and semantics are identical to those of :class:`trio.Lock`.
    """

    _rcount = attr.ib(default=0, init=False)

    def acquire_nowait(self):
        if trio.hazmat.current_task() is self._owner:
            self._rcount += 1
            return
        super().acquire_nowait()

    def release(self):
        if trio.hazmat.current_task() is self._owner:
            if self._rcount:
                self._rcount -= 1
                return
        super().release()

It works great so far and cancellation semantics should be correct as well, but I'm not entirely sure. Would you like a PR with this?

Another approach would be to add a keyword-only reentrant=False parameter to the existing trio.Lock, the two checks could then be added to the existing acquire_nowait and release methods with the (tiny) overhead of if self._rcount in release().

What do you think, is this something we want in trio?

Best regards
Robert

[bob1de]

The more I think about it, the more I like the idea of extending trio.Lock, which should make documenting it simpler and we'd just have to extend the docstrings slightly and add some tests.

[njsmith]

@efficiosoft Hello! Thanks for trying Trio, and giving us feedback :-)

The main reason I didn't add re-entrant originally was just a vague impression from more experienced folks that they're messy and not necessarily a good idea. But you're right that they're common (in fact Trio even uses threading.RLock internally to solve an exotic problem involving signal handlers and threads), and vague impressions aren't a good basis for API decisions. So we should try to figure out what these alleged downsides are and whether we agree :-).

I did a bit of searching, and found this interesting article: http://www.zaval.org/resources/library/butenhof1.html

I'm not sure how compelling his argument is in the Trio context. All the stuff about needing to hold locks for as-little-as-possible doesn't really apply for us; we're single-threaded regardless of whether you use trio.Lock or not. For us locks are something you use for certain types of complex coordination, not something you use constantly every time you touch any kind of shared state.

The argument that RLock tends to promote fuzzy thinking about locking seems somewhat compelling. It's not a slam dunk; it could be that it promotes fuzzy thinking, but is still useful enough to include (with maybe some warnings). I'm not sure.

I do think the point that you can't use an RLock and a Condition together is totally convincing. So I think that rules out the idea of having a reentrant=True/False argument to trio.Lock.__init__ – this is at least one way that RLock and Lock are definitely not interchangeable.

If anyone else knows of arguments for/against reentrant locking, please post them here :-)

BTW: trio.Lock wasn't designed to support subclassing, and subclassing it is dangerous... see #1021 and #1044, and especially this comment on #1044. I'd recommend using composition instead, like:

class RLock:
    def __init__(self):
        self._owner = None
        self._depth = 0
        self._lock = trio.Lock()

    async def acquire(self):
        if self._owner is trio.hazmat.current_task():
            self._depth += 1
        else:
            await self._lock.acquire()
            assert self._owner is None
            assert self._depth == 0
            self._owner = trio.hazmat.current_task()
            self._depth += 1

    def release(self):
        if self._owner is not trio.hazmat.current_task():
            raise RuntimeError
        assert self._owner is trio.hazmat.current_task()
        assert self._depth >= 1
        self._depth -= 1
        if self._depth == 0:
            self._lock.release()
            self._owner = None

    # to do: add trivial __aenter__/__aexit__ etc.

(This could be made a bit simpler if Lock._owner was public... maybe that would be a good idea? I think the reason I didn't in the first place is that task objects are a low-level kind of thing and we don't want to accidentally communicate to users that they should be messing with them in regular code. _owner is exposed as part of Lock.current_statistics though, which is intended for debugging rather than regular usage.)

[bob1de]

I do think the point that you can't use an RLock and a Condition together is totally convincing. So I think that rules out the idea of having a reentrant=True/False argument to trio.Lock.init – this is at least one way that RLock and Lock are definitely not interchangeable.

Agreed, so let's make it a separate object.

BTW: trio.Lock wasn't designed to support subclassing, and subclassing it is dangerous...

You're right, relying on the implementation details is a bad idea for external code, however I thought it would be ok if the RLock implementation was included in trio core.

[njsmith]

I found another article that I think makes a pretty good case for why _nolock methods might actually be better than reentrant locks: https://blog.stephencleary.com/2013/04/recursive-re-entrant-locks.html

In particular this point:

The entire purpose of a “lock” is to protect shared mutable state so that reads and writes appear atomic. In other words, the state is consistent whenever the lock is unlocked; while the lock is locked (held) by a section of code, the state may be inconsistent. Another way of saying this is that while the lock is held, it’s possible that invariants will not hold; acquiring a lock temporarily suspends all contractural invariants until the time the lock is released.

So the point is that _nolock methods don't just skip taking locks; they also have much more complex and subtle semantics, where they have to be prepared for the program to be in some odd state, and need to carefully document which invariants they depend on and which ones can be relaxed.

The "Uncertainty about lock state" section is also particularly compelling: it makes that point that reentrant locks create new deadlock hazards, because you can release the lock and then block on some other operation, which would be fine if the lock were actually released, but with a reentrant lock then you might actually still hold the lock, which might end up preventing the thing you're waiting on from happening...

[njsmith]

You're right, relying on the implementation details is a bad idea for external code, however I thought it would be ok if the RLock implementation was included in trio core.

Right, inside trio core we can do whatever; that section was just a side-comment since you said you're currently subclassing trio.Lock in your code.

[bob1de]

So the point is that _nolock methods don't just skip taking locks; they also have much more complex and subtle semantics, where they have to be prepared for the program to be in some odd state, and need to carefully document which invariants they depend on and which ones can be relaxed.

For the general case, this is surely true. In my particular project, I'm building a framework for running custom user-written apps and am serializing management operations for those apps (starting/stopping/removing) using a per-app lock. Removing, for instance, consists of stopping + some extra work to remove the app from the system, such as cancelling watcher tasks. It now is convenient to rely on the fact that no one can start the app between remove() stopped it and it's finally marked as removed. Of course, I could introduce a flag that, when set, prevents the app from being started again or simply add an internal API for stopping which doesn't take the lock, but a reentrant lock seemed more straightforward in this case.

[oremanj]

It sounds like a reentrant lock probably isn't a great fit for Trio core right now, so going to close this.