| Commit message (Collapse) | Author | Age | Files | Lines |
|---|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
```
[NoMethodError]: undefined method `state' for nil:NilClass Method:[rescue in block in refresh]
```
In `within_new_transaction`, there is the possibility that
`begin_transaction` returns a `nil`. (i.e.: so `transaction = nil`)
So this method is checking `transaction` for nil in 2 spots.
Unfortunately, there is one line that is not checking `transaction` for `nil`
That line, `commit_transaction`, throws an exception for us in AR 5.0.0.1
The problem with the method is finally realized in the error checking itself.
it calls `transaction.state` (i.e.: nil.state) and that is the final exception
raised.
The actual underlying (user) issue is hidden by this line.
Solution is test transaction for nil.
|
| | |
|
| |
|
|
| |
Because the deprecation message is not yet released.
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Let's say you have a nested transaction and both records are saved.
Before the outer transaction closes, a rollback is performed. Previously
the record in the outer transaction would get marked as not persisted
but the inner transaction would get persisted.
```ruby
Post.transaction do
post_one.save # will get rolled back
Post.transaction(requires_new: true) do
post_two.save # incorrectly remains marked as persisted
end
raise ActiveRecord::Rollback
end
```
To fix this the PR changes transaction handling to have the child
transaction ask the parent how the records should be marked. When
there are child transactions, it will always be a SavpointTransaction
because the stack isn't empty. From there we pass the parent_transaction
to the child SavepointTransaction where we add the children to the parent
so the parent can mark the inner transaction as rolledback and thus mark
the record as not persisted.
`update_attributes_from_transaction_state` uses the `completed?` check to
correctly mark all the transactions as rolledback and the inner record as
not persisted.
```ruby
Post.transaction do
post_one.save # will get rolled back
Post.transaction(requires_new: true) do
post_two.save # with new behavior, correctly marked as not persisted
on rollback
end
raise ActiveRecord::Rollback
end
```
Fixes #29320
|
| |
|
|
|
|
|
|
|
|
|
|
|
| |
`set_state` was directly setting the transaction state instance
variable. It's better to set the state via specific methods (`rollback!`
and `commit!` respectively.
While undocumented and untested, it's possible someone is using
`set_state` in their app or gem so I've added a deprecation notice to
it.
No where in the app do we use `nullify!` but I wanted to keep existing
behavior while replacing the method with a better pattern.
|
| |
|
|
|
| |
This reverts commit 3420a14590c0e6915d8b6c242887f74adb4120f9, reversing
changes made to afb66a5a598ce4ac74ad84b125a5abf046dcf5aa.
|
| | |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Transactional-fixture using tests with racing threads and inter-thread
synchronisation inside transaction blocks will now deadlock... but
without this, they would just crash.
In 5.0, the threads didn't share a connection at all, so it would've
worked... but with the main thread inside the fixture transaction, they
wouldn't've been able to see each other.
So: as far as I can tell, the set of operations this "breaks" never had
a compelling use case. Meanwhile, it provides an increased level of
coherency to the operational feel of transactional fixtures.
If this does cause anyone problems, they're probably best off disabling
transactional fixtures on the affected tests, and managing transactions
themselves.
|
| | |
|
| |
|
|
|
| |
The current code base is not uniform. After some discussion,
we have chosen to go with double quotes by default.
|
| |\
| |
| |
| |
| | |
samphilipd/sam/properly_deallocate_prepared_statements_outside_of_transaction
Correctly deallocate prepared statements if we fail inside a transaction
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
- Addresses issue #12330
Overview
========
Cached postgres prepared statements become invalidated if the schema
changes in a way that it affects the returned result.
Examples:
- adding or removing a column then doing a 'SELECT *'
- removing the foo column then doing a 'SELECT bar.foo'
In normal operation this isn't a problem, we can rescue the error,
deallocate the prepared statement and re-issue the command.
However in PostgreSQL transactions, once any command fails, the
transaction becomes 'poisoned' and any subsequent commands will raise
InFailedSQLTransaction.
This includes DEALLOCATE statements, so the default deallocation
strategy instead of removing the cached prepared statement instead
raises InFailedSQLTransaction.
Why this is bad
===============
1. InFailedSQLTransaction is a fairly cryptic error and doesn't
communicate any useful information about what has actually gone wrong.
2. In the naive implementation the prepared statement never gets
deallocated - it stays alive for the length of the session taking up
memory on the postgres server.
3. It is unsafe to retry the transaction because the bad prepared
statement is still in the cache and we would see the exact same failure
repeated.
Solution
========
If we are outside a transaction we can continue to handle these failures
gracefully in the usual way.
Inside a transaction instead of issuing a DEALLOCATE command that will
certainly fail, we now raise
ActiveRecord::PreparedStatementCacheExpired.
This can be handled further up the stack, notably inside
TransactionManager#within_new_transaction. Here we can make sure to
first rollback the transaction, then safely issue DEALLOCATE statements
to invalidate the rest of the cached prepared statements.
This also allows the user (or some gem) the opportunity to catch this error and
voluntarily retry the transaction if a schema change causes the prepared
statement cache to become invalidated.
Because the outdated statement has been deallocated, we can expect the
transaction to succeed on the second try.
|
| | |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| |
| | |
When a `before_commit` callback raises, the database is rolled back but
AR's record of the current transaction is not, leaving the connection in
a perpetually broken state that affects all future users of the
connection: subsequent requests, jobs, etc. They'll think a transaction
is active when none is, so they won't BEGIN on their own. This manifests
as missing `after_commit` callbacks and broken ROLLBACKs.
This happens because `before_commit` callbacks fire before the current
transaction is popped from the stack, but the exception-handling path
they hit assumes that the current transaction was already popped. So the
database ROLLBACK is issued, but the transaction stack is left intact.
Common cause: deadlocked `#touch`, which is now implemented with
`before_commit` callbacks.
What's next:
* We shouldn't allow active transaction state when checking in or out
from the connection pool. Verify that conns are clean.
* Closer review of txn manager sad paths. Are we missing other spots
where we'd end up with incorrect txn state? What's the worst that can
happen if txn state drifts? How can we guarantee it doesn't and
contain the fallout if it does?
Thanks for @tomafro for expert diagnosis!
|
| |/
|
|
| |
Fixes https://github.com/rails/rails/issues/22819
|
| | |
|
| |
|
|
|
|
|
|
| |
This reverts commit 393e65b4170608593ad82377a9eadc918e85698d and
ec51c3fedd16b561d096dcc1a6705fdc02ab7666
We don't want the records to hold hard references to transactions
because they point at records that have callbacks.
|
| |
|
|
|
| |
this lets us keep singleton instances of "state" values and precalculate
return values of things like `finalized?` and `completed?`.
|
| |
|
|
|
| |
this way we don't have to mutate a state object, we can just change the
state of the txn
|
| | |
|
| |
|
|
| |
As far as I can tell nobody is setting this variable.
|
| |
|
|
| |
[fixes #18903]
|
| | |
|
| |
|
|
|
| |
the transaction object shouldn't know so much about active record
objects, so let's push the conditionals in to the instance.
|
| | |
|
| | |
|
| |
|
|
|
|
| |
after_commit callbacks run after committing a transaction whose parent
is not `joinable?`: un-nested transactions, transactions within test
cases, and transactions in `console --sandbox`.
|
| |
|
|
| |
It is up to the TransactionManager keep the state of current transaction, so after it commits it needs to copy any remaning record to the next current transaction
|
| |
|
|
| |
As discussed before, those methods should receive a keyword args instead of just parameters
|
| |
|
|
|
|
|
|
| |
Before this change any error raised inside a transaction callback
are rescued and printed in the logs.
Now these errors are not rescue anymore and just bubble up,
as the other callbacks.
|
| |\
| |
| |
| | |
Data corruption risk: Roll back open transactions when the running thread is killed.
|
| |/ |
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
Currently, Active Record will rescue any errors raised within
after_rollback/after_create callbacks and print them to the
logs. Next versions of rails will not rescue those errors anymore,
and just bubble them up, as the other callbacks.
This adds a opt-in flag to enable that behaviour, of not rescuing
the errors.
Example:
# For not swallow errors in after_commit/after_rollback
config.active_record.errors_in_transactional_callbacks = true
[fixes #13460]
|
| |
|
|
|
|
|
|
|
| |
after_commit should not run in nested transactions, however they should
run once the outermost transaction gets committed. This patch fixes the
problem copying the records from the Savepoint to its parent. So the
RealTransaction will have all records that needs to run callbacks on it.
[fixes #16425]
|
| |
|
|
|
|
| |
Use `commit_transaction`/`rollback_transaction` on
`within_new_transaction` method, so they make sure they `pop` the
transaction from the stack before calling the methods `commit`/`rollback`.
|
| |
|
|
|
|
|
|
|
| |
Transaction class doesnt need to encapsulate the transaction state using
inheritance.
This removes all Transaction subclasses, and let the Transaction object
controls different actions based on its own state. Basically the only
actions would behave differently are `being`,`commit`,`rollback` as they
could act in a savepoint or in a real transaction.
|
| | |
|
| | |
|
| | |
|
| | |
|
| | |
|
| |
|
|
|
|
|
|
|
| |
This piece of code was introduced on
67d8bb963d5d51fc644d6b1ca20164efb4cee6d7 , which was calling
`committed?` in the `transaction_state` before calling the `committed!`
method. However on 7386ffc781fca07a0c656db49fdb54678caef809, the
`committed?` check was removed and replaced by a `finalized?`, which
only checks if the state is not nil. Thus we can remove that line.
|
| | |
|
| |
|
|
| |
Also add test to assets the savepoint name
|
| |
|
|
|
|
|
| |
Add a transaction manager per connection, so it can controls the
connection responsibilities.
Delegate transaction methods to transaction_manager
|
| |
|
|
|
|
|
|
|
|
| |
The finishing variable on the transaction object was a work-around for
the savepoint name, so after a rollback/commit the savepoint could be
released with the previous name.
related:
9296e6939bcc786149a07dac334267c4035b623a
60c88e64e26682a954f7c8cd6669d409ffffcc8b
|
| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
This fixes a regression (#13744) that was caused by 67d8bb9.
In 67d8bb9, we introduced lazy rollback for records, such that the
record's internal states and attributes are not restored immediately
after a transaction rollback, but deferred until they are first
accessed.
This optimization is only performed when the model does not have any
transactional callbacks (e.g. `after_commit` and `after_create`).
Unfortunately, the models used to test the affected codepaths all
comes with some sort of transactional callbacks. Therefore this
codepath remains largely untested until now and as a result there are
a few issues in the implementation that remains hidden until now.
First, the `sync_with_transaction_state` (or more accurately,
`update_attributes_from_transaction_state`) would perform the
synchronization prematurely before a transaction is finalized (i.e.
comitted or rolled back). As a result, when the actuall rollback
happens, the record will incorrectly assumes that its internal states
match the transaction state, and neglect to perform the restore.
Second, `update_attributes_from_transaction_state` calls `committed!`
in some cases. This in turns checks for the `destroyed?` state which
also requires synchronization with the transaction stae, which causes
an infnite recurrsion.
This fix works by deferring the synchronization until the transaction
has been finalized (addressing the first point), and also unrolled
the `committed!` and `rolledback!` logic in-place (addressing the
second point).
It should be noted that the primary purpose of the `committed!` and
`rolledback!` methods are to trigger the relevant transactional
callbacks. Since this code path is only entered when there are no
transactional callbacks on the model, this shouldn't be necessary. By
unrolling the method calls, the intention here (to restore the states
when necessary) becomes more clear.
|
| | |
|
| |
|
|
| |
with new transaction state. If AR object has a callback, the callback will be performed immediately (non-lazily) so the transaction still has to keep records with callbacks.
|
| |
|
|
| |
new transaction state object upon initialization.
|
| | |
|
