Add cancellation support to ReadWriteLock

[squahtx]

Best reviewed commit by commit.

Part of #3528.

delay_cancellation comes from #12180.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[squahtx]

I'm not sure of the benefit of using async context managers.
It mainly benefits the cleanup for the read path, where the cleanup for not-holding-the-lock and holding-the-lock happen to be the same. On the write path we have to handle both cases differently, by deferring the cleanup until we actually acquire the lock.

[richvdh]

nice. a few comments though.

[richvdh]

I'm wondering if we should, for strict correctness, do all the to_wait_on calculation etc inside the ctx_manager. Otherwise it's possible for someone to do:

ctx = self.pagination_lock.write(room_id)
await something_asynchronous()
with await ctx:
    ...

... which is a stupid thing to do, but still.

[squahtx]

I'm all in favour of removing footguns. I'lk move the setup of read() and write() into the context manager.

[richvdh]

it's hard to follow what _assert_called_before_not_after is doing. Assuming you've figured it out, could you add a comment/docstring/type annotation/etc or two?

Actually generally this test could do with a few comments, for example:

Suggested change

	self._assert_called_before_not_after(acquired_ds, 2)
	# we should have acquired the locks for the first two readers, but nothing else.
	self._assert_called_before_not_after(acquired_ds, 2)

[squahtx]

I found this test case hard to understand too. I'll try to make it more readable.

[richvdh]

presumably we need to do this even if to_wait_on_defer ends up erroring, so this needs to be

Suggested change

	to_wait_on_defer.addCallback(lambda _: release())
	to_wait_on_defer.addBoth(lambda _: release())

[squahtx]

I'll make the change for the sake of robustness, but all of the Deferreds that represent lock release, and thus also to_wait_on_defer, should never error.

[richvdh]

could we, instead of all this, do:

             to_wait_on_defer = defer.gatherResults(to_wait_on)
             to_wait_on_defer.addBoth(lambda _: release())
             await make_deferred_yieldable(to_wait_on_defer)
             yield

... possibly with more stop_cancellation ? Or does that not work?

[squahtx]

release() must only be called after the yield, so I'm guessing you meant:

try:
    await make_deferred_yieldable(stop_cancellation(to_wait_on_defer))
    yield
finally:
    to_wait_on_defer.addBoth(lambda _: release())

which I was originally hesitant to do because of re-entrancy in the happy path:

1. When we exit the await, it will be because we are inside a call to to_wait_on_defer.callback(...).
2. During the yield / body of the context manager, we may or may not do a blocking await. If we block, then the to_wait_on_defer.callback() call gets exited, otherwise we remain within the to_wait_on_defer.callback() call.
3. We reach the finally block.
   If we're no longer inside the callback() call, release() will be called instantly with the current logging context.
   Otherwise if we're still inside the callback() call, release() won't be called until we next block or terminate and will have the sentinel context.

The sometimes-extra delay before release() is tricky to reason about, but I think the proposed code would be alright.

---

As an alternative, I'm very tempted by @erikjohnston's suggestion of delaying CancelledErrors:

try:
    await make_deferred_yieldable(delay_cancellation(to_wait_on_defer))
    yield
finally:
    release()  # but inline again

Where delay_cancellation would hold on to the CancelledError and only raise it once to_wait_on_defer resolves.

We'd get better maintainability at the cost of having the crud associated with cancelled requests lingering a little while longer, but maybe that's a worthwhile tradeoff.

[richvdh]

release() must only be called after the yield,

oh, duh. Yes, of course.

The sometimes-extra delay before release() is tricky to reason about, but I think the proposed code would be alright.

ugh, right, yes, let's avoid that.

And yes, delay_cancellation looks like it might be worth exploring.

[squahtx]

I had a go at using a delay in cadfe0a.
We now depend on #12180 for delay_cancellation of course.

[richvdh]

some docs on the params and return vals would be helpful.

[squahtx]

Sounds like a good idea. Added some docs.

[richvdh]

it's a bit confusing that this is slightly different to the class-level _start_reader_or_writer. Can we combine them, even if it means returning three deferreds rather than two?

[squahtx]

Yep, let's combine them.

[squahtx]

I had a go at using a delay to simplify the write path in cadfe0a.
We now depend on #12180 for delay_cancellation of course.

[richvdh]

lgtm, subject to updates for consistency with #12180