Do not ignore transaction durability errors within TestCase

[Krzysztof Jagiełło]

Currently there is a discrepancy in how durable atomic blocks are handled in TransactionTestCase vs TestCase. Using the former, nested durable atomic blocks will, as expected, result in a RuntimeError. Using the latter however, the error will go unnoticed as the durability check is turned off.

I have faced some issues with this behaviour in a codebase where we heavily utilize TestCase and where we recently started to introduce durable atomic blocks – the durability errors do not surface until the code hits staging/production. The solution could be to switch over to TransactionTestCase for the test classes that hit code paths with durable atomic blocks, but having to identify which tests could be affected by this issue is a bit inconvenient. And then there is the performance penalty of using TransactionTestCase.

So, to the issue at hand. The durability check is disabled for TestCase because otherwise durable atomic blocks would fail immediately as TestCase wraps its tests in transactions. We could however add a marker to the transactions created by TestCase, keep a stack of active transactions and make the durability check take the stack of transactions with their respective markers into account. This way we could easily detect when a durable atomic block is directly within a transaction created by TestCase and skip the durability check only for this specific scenario.

To better illustrate what I am proposing here, I have prepared a PoC patch. Let me know what you think!

Patch: ​https://github.com/django/django/pull/14919

[Mariusz Felisiak]

I don't think it's worth additional complexity and potential slowdown of TestCase for all users. I will wait for a second opinion before making a decision.

[Krzysztof Jagiełło]

Could you elaborate on what would cause the slowdown? I have noticed the mention about it in the docs, but couldn't really figure out why this would incur any significant performance penalty.

[Adam Johnson]

I think it's reasonable for tests to still detect such problematic durable atomic blocks, without requiring TransactionTestCase.

I've reviewed the initial PR and I came up with an idea for a simpler implementation: ​https://github.com/django/django/pull/14922

My implementation removes ensure_durability and adds another boolean flag on BaseDatabaseWrapper. This leaves us with about the same level of complexity as before the PR, so hopefully that alleviates any concerns.

(I also don't believe there's any potential for notable slowdown from the initial implementation - it only adds one stack.)

[Mariusz Felisiak]

(I also don't believe there's any potential for notable slowdown from the initial implementation - it only adds one stack.)

Persuaded.

[Krzysztof Jagiełło]

Thanks Adam! Your approach seems more straightforward to me than the initial implementation. Keeping around the stack of atomic blocks just to handle this is specific scenario was a bit unnecessary.

[Krzysztof Jagiełło]

It took me a while to realize that a boolean flag on BaseDatabaseWrapper might not be enough for cases like these:

    def test_sequence_of_durables(self):
        with transaction.atomic(durable=True):
            ...
        with transaction.atomic(durable=True):
            ...

Keeping the atomic block stack around makes it easier to handle it correctly.

[David Seddon]

Thanks for looking into this!

We've been using a hand-rolled equivalent of durable=True extensively for a couple of years on a large code base. Our implementation consults a setting which we typically override along the lines of @override_settings(DISABLE_DURABILITY_CHECKS=True). Two years in, I don't think there's been much value in requiring the override for each relevant test, and I imagine we'll switch to using a global setting (at least for the transaction-wrapped test cases which, conveniently, use a different configuration to the unwrapped ones).

So on balance I think it makes more sense to disable the behaviour than require developers to turn it off on a test-by-test basis, though it might be worthwhile providing a setting so that people can choose the default behaviour.

Having said that, there is a downside which reminds me of the difficulties of testing code that uses select_for_update. I have seen many occasions where wrapped test cases are the only coverage of such code, and we don't see the issue until it hits production. If we just turn off the checking wholesale, it's more likely that unrunnable code will make its way onto production.

An even better solution would be for wrapped TestCases to be able to tell the difference between the transaction begun by the TestCase itself and any subsequent atomic blocks entered. They would then allow durable blocks to be entered providing the only block we're in is that initial block (possibly this is what has been implemented, I wasn't able to tell at first glance).

[Krzysztof Jagiełło]

Replying to David Seddon:

It reminds me of the difficulties of testing code that uses select_for_update. I have seen many occasions where wrapped test cases are the only coverage of such code, and we don't see the issue until it hits production.

Got hit by that one to in the past too! I think that once this patch gets merged we should be able to apply a similar fix to select_for_update too.

Regarding the patch, I have decided to keep working on my PR (​https://github.com/django/django/pull/14919) as the one proposed by Adam did not work for sequential durable transactions (see the repro case in my previous comment). Let me know if there is anything you would like me to adjust in my PR before its merge-ready.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 8d9827c0:

Fixed #33161 -- Enabled durability check for nested atomic blocks in TestCase.

Co-Authored-By: Adam Johnson <me@…>