Add transaction-aware client

[adamniedzielski]

Yet another attempt at #5239

[mperham]

👏🏻 Well done.

I like the simplicity, we're getting to an implementation I can fully understand now. The drawbacks seem to be (1) no rollback support and (2) globally enabled for all jobs or none. (2) seems ok. For (1), there was a usecase around rescuing AR::Rollback and enqueuing a job to inform a 3rd party service about the rollback. Is that possible?

[adamniedzielski]

globally enabled for all jobs or none

That's not true. I added tests to show how to enable / disable it for one job.

[adamniedzielski]

there was a usecase around rescuing AR::Rollback and enqueuing a job to inform a 3rd party service about the rollback. Is that possible?

I don't think that it's a valid use case and I'm not interested in implementing it.

1. Jobs "on success of the operation" can be scheduled at multiple stages of the transaction, sometimes multiple levels deep. That's why we have this PR.
2. Error handling in turn should rather happen in one place and not be scattered throughout the operation. The implementation can be as simple as:

begin
  MyOperation.call
rescue ActiveRecord::Rollback
  MyErrorHandlingJob.perform_async
end

I struggle to see the real need for anything else. However, if you or somebody else wants to extend the implementation, go ahead 🙂

[mperham]

Fair enough, it’s not critical functionality.

Would you be interested in getting this into main/6.5 as a “beta” feature? I don’t have a good sense for how much support load this feature might require so I’m hesitant to launch it as fully supported right out of the box. That’s why I originally thought 7.0 but the 7 release is looking a bit far off now.

[adamniedzielski]

@mperham I changed the base branch to main. It can go in 6,5 as "beta feature". I like this release idea 💡

[mperham]

🎉

[adamniedzielski]

Thanks for merging 💛 I'm excited to see what things we discover once folks start using it.

[Galathius]

JFYI: this implementation may lead to leaking database connections from pool, I posted more info here.
cc: @adamniedzielski @mperham

[Envek]

Fixed unexpected implicit connection checkout that was leading to database connection leak outside of Rails executor in Envek/after_commit_everywhere#21 and Envek/after_commit_everywhere#22 (released in after_commit_everywhere 1.2.1 and 1.2.2 respectively)

[physics141]

@adamniedzielski thanks for implementing this.
just wanted to ask is this still in beta or is it working fine from your end?
i checked from my side and workers were enqueueing fine.
can i use this feature in my production?

[taylorthurlow]

@physics141 Mike noted in the 6.5 release notes that the feature was in beta but would be fully supported upon the release of 7.0. There wasn't any specific mention of this in any of the subsequent release notes, but generally Mike is not the type of person to just forget about this (the PR was also included in a 7.0 release checklist), so I don't think you should have any worry about using this feature in production at this point.

[mperham]

Yep, ^^^^. Aside from a few bits of polish after the initial release, the transactional client has seen very few issues. It's stable and isn't considered beta anymore.