Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ensure transaction tracking finishes when reconnecting #49391

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Ensure transaction tracking finishes when reconnecting #49391

merged 1 commit into from
Oct 4, 2023

Conversation

ipc103
Copy link
Contributor

@ipc103 ipc103 commented Sep 26, 2023

Motivation / Background

We noticed a couple of spots where transaction tracking events were potentially incorrect.

  • When the connection reconnects with restore_transactions: true, we were keeping the original start time for the transaction. In this case, it makes more sense to treat these as two separate transactions for tracking purposes.
  • If you manually call reconnect! within a transaction opened via being_transaction rather than transaction do, we were never marking the event as finished and thus never broadcasting the event. This isn't public API and likely isn't very common, but we did run into one scenario in a test file that was doing this in a setup block.

Detail

We added a line to the reset_transactions method to mark any open transactions as finished. We went back and forth on the naming for this new method, but eventually settled on suspend_transactions since the transactions may potentially be restarted.

Additional information

This is a followup to #49192 and #49262

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@ipc103 ipc103 force-pushed the transaction-tracking-on-reconnect branch 2 times, most recently from a88ff07 to 9e46520 Compare September 29, 2023 20:45
rafaelfranca
rafaelfranca reviewed Sep 29, 2023
View reviewed changes
activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb Outdated
@@ -364,6 +364,8 @@ def transaction_open?
end

def reset_transaction(restore: false) # :nodoc:
@transaction_manager.suspend_transactions if defined?(@transaction_manager)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need to use defined? here and not in the line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question - since this method is called in initialize without the restore option, the @transaction_manager was always initialized before being accessed. Maybe it would be better to do something like this here:

if restore
  @transaction_manager.suspend_transactions
  old_state = @transaction_manager if @transaction_manager.restorable?
end

but I'm going to rework this based on the additional feedback, so I'll remove this line entirely.

@rafaelfranca rafaelfranca added this to the 7.1.0 milestone Sep 29, 2023
@matthewd
Copy link
Member

Agree naming is hard here... but I don't like "suspend". "detach" is probably the closest active verb I can come up with ("abandon" feels too final), but perhaps "lost" is a better fit: it's something that happened to us, not something we want to do. Or maybe "break"?

In the current state of things1, I'd much prefer that the finish live in restore!, to avoid propagating knowledge of the "we've been disconnected, but might try to fix it" pseudo-state -- right now that knowledge doesn't go deeper than reset_transaction, and the TransactionManager (and Transaction) live in blissful ignorance until the moment they're asked to restore. But that obviously only works for the restore-true case. 😕

It's certainly true that Transaction is already rather sensitive to its caller knowing what methods are permissible to call at any given time, but I think most existing possible confusions will produce an exception at some point. It seems like TransactionInstrumenter#start-when-started and TransactionInstrumenter#finish-when-not-started should almost always be errors (modulo some currently-absent if materialized? checks)... if it would be practical to make those raise, I think that would at least keep it closer to "sensitive, but it'll break" rather than slipping in to "sensitive, and we just have to be careful"?

Footnotes

  1. I can see a future where we keep a known-broken-and-unrestorable transaction on the txn stack until we have unwound to the corresponding point in the ruby stack, and outright refuse to touch the connection as long as it's there... but that means (a bunch of edge-case behaviour [improvements, but] changes, and) more actual state transitions. ↩

@rafaelfranca rafaelfranca force-pushed the transaction-tracking-on-reconnect branch from 0b014b0 to a1bab92 Compare October 4, 2023 23:31
@ipc103 @composerinteralia
Ensure transaction tracking finishes when reconnecting
306ef99 
We noticed a couple of spots where transaction tracking events
were potentially incorrect.

When the connection reconnects with `restore_transactions: true`,
we were keeping the original start time for the transaction. In this case
it seems more accurate to treat these as separte transactions where the first
one finishes as incomplete.

Instead of forcing the adapter to tell the transaction manager to
suspend (or `detach` or `lost`) the transactions, we can keep that
logic encapsulated inside of the Transaction class for when to broadcast
a finish event. This means that we won't capture a finish event if you
manually call `reconnect!` without `restore_transactions: true`, but
that might be worthy of the tradeoff since this is a rare use-case anyway.

We also start raising here if the TransactionInstrumenter receives `#start`
when already started, or `#finish` when not started. This ensures that we don't
quietly end up in any weird states.

When marking that a transaction is incomplete, we also want to check
that the transaction is materialized to avoid finishing our instrumentation
if it hasn't already started. Also added a test to simulate losing a connection
without ever materializing a transaction.

Co-authored-by: Daniel Colson <[email protected]>
@ipc103 ipc103 force-pushed the transaction-tracking-on-reconnect branch from a1bab92 to 306ef99 Compare October 4, 2023 23:38
@rafaelfranca rafaelfranca merged commit b71c75e into rails:main Oct 4, 2023
rafaelfranca added a commit that referenced this pull request Oct 4, 2023
@rafaelfranca
Merge pull request #49391 from ipc103/transaction-tracking-on-reconnect
b6842b2 
Ensure transaction tracking finishes when reconnecting
@ipc103 ipc103 deleted the transaction-tracking-on-reconnect branch October 5, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaelfranca rafaelfranca rafaelfranca left review comments

Assignees
No one assigned
Labels
activerecord
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ipc103 @matthewd @rafaelfranca