Ensure we always drop the federation inbound lock

[erikjohnston]

Hopefully fixes errors like: Lock for (federation_inbound_pdu, <room_id>) dropped without being released

(Sentry link: https://sentry.matrix.org/sentry/synapse-matrixorg/issues/224832/)

[richvdh]

I can't help wondering if Lock should really be implementing the context manager interface. What's the advantage over just mandating that people call lock.release() ?

[erikjohnston]

Personally I find using a context manager interface clearer than using a try/finally. We could also have an explicit lock.release() to use here

[richvdh]

Personally I find using a context manager interface clearer than using a try/finally

well maybe, but that seems a bit irrelevant.

basically:

lock.release()

seems a more intuitive way of dropping a lock than

async with lock():
    pass

I think it's basically a bit silly to have an aenter which does nothing.

[erikjohnston]

I've added a lock.release() method

Personally I find using a context manager interface clearer than using a try/finally

well maybe, but that seems a bit irrelevant.

How so? The other place we use the async with lock we'd need to convert to a try/finally if we got rid of the context manager.

[richvdh]

How so? The other place we use the async with lock we'd need to convert to a try/finally if we got rid of the context manager.

Oh I see. Well, yes, we would, and tbh I think that would be an improvement. Currently it looks like something magical happens at the start of that async with block, which is misleading. The with mechanism doesn't really work very well here, because in each case we already hold the resource (ie, the lock) when we enter the with block.

Instead we could put the whole body of _process_incoming_pdus_in_room_inner inside a try block:

async def _process_incoming_pdus_in_room_inner(...):
    try:
        # ...
        while True:
            await self.handler.on_receive_pdu(event)

            # drop the lock before checking for more events
            await lock.release()
            lock = None

            next = await self.store.get_next_staged_event_for_room()
            if not next:
                break
            
            lock = await self.store.try_acquire_lock()
            if not lock:
                break
    finally:
        if lock:
            lock.release()

... thus ensuring that we always drop the lock even if we end up bailing out at odd points due to exceptions or whatever.

Anyway, what you have now is fine.

[callahad]

Let's get this in on 1.38 and do an RC2

[richvdh]

lgtm