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: Coming soon
--
Ticket URL: <https://code.djangoproject.com/ticket/33161>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Old description:
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:1>
* owner: nobody => Krzysztof Jagiełło
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:2>
* version: 3.2 => dev
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:3>
* cc: David Seddon, Adam Johnson, Ian Foote (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:4>
Comment (by 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 performance penalty.
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:5>
* cc: Hannes Ljungberg (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:6>
Comment (by 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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:7>
* stage: Unreviewed => Accepted
Comment:
> (I also don't believe there's any potential for notable slowdown from
the initial implementation - it only adds one stack.)
Persuaded.
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:8>
Old description:
> 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
New description:
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/14922
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:9>
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:10>
Old description:
> 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/14922
New description:
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!
Original PoC patch: https://github.com/django/django/pull/14919
Current patch: https://github.com/django/django/pull/14922
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:11>
Comment (by Krzysztof Jagiełło):
It took me a while to realize that a boolean flag on `BaseDatabaseWrapper`
might not be enough for cases like these:
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:12>
Old description:
> 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!
>
> Original PoC patch: https://github.com/django/django/pull/14919
> Current patch: https://github.com/django/django/pull/14922
New description:
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!
Original PoC patch: https://github.com/django/django/pull/14919
Alternative patch: https://github.com/django/django/pull/14922
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:13>
Comment (by 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).
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:14>
Comment (by Krzysztof Jagiełło):
Replying to [comment:14 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:15>
Old description:
> 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!
>
> Original PoC patch: https://github.com/django/django/pull/14919
> Alternative patch: https://github.com/django/django/pull/14922
New description:
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:16>
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:17>
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8d9827c06ce1592cca111e7eafb9ebe0153104ef" 8d9827c0]:
{{{
#!CommitTicketReference repository=""
revision="8d9827c06ce1592cca111e7eafb9ebe0153104ef"
Fixed #33161 -- Enabled durability check for nested atomic blocks in
TestCase.
Co-Authored-By: Adam Johnson <m...@adamj.eu>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33161#comment:18>