Support a :savepoint option to after_commit and after_rollback.

[chanks]

Currently after_commit and after_rollback hooks are ignorant of savepoints, and always run when the top-level transaction commits or rolls back. This is inconvenient for those of us (most of us?) who wrap tests in transactions, because it means that even when using :auto_savepoint, logic in after_commit and after_rollback callbacks won't run in the scope of the test and can't be checked.

This PR tries to fix that by supporting a :savepoint option to after_commit and after_rollback that attaches them to the current savepoint, so that their blocks will be called when it is committed or rolled back, and not the top-level transaction. If there is no current savepoint, the callbacks are attached to the top-level transaction, just as they would be if the :savepoint option weren't given. Now when we want to wrap all tests in transactions, we can just use the :savepoint option everywhere to make sure that those actions are testable.

I feel like there should be a discussion about whether this should be the default behavior in Sequel, since I believe that most people would want to use it most of the time. But even if the community were generally on board with that, it'd be a change in behavior that would probably need to wait for Sequel 5.

[jeremyevans]

This is an interesting idea. I can see it's usefulness in testing certain types of code, but I don't think it could ever be made the default behavior. The point of after_commit and after_rollback is to execute code after the transaction has been committed or rolled back, so that external services/connections can then access it.

In production, you would never want after_commit to be called just because a savepoint was released, as the related transaction could still be rolled back. I suppose you could call after_rollback as soon as the enclosing savepoint was rolled back. Arguably that makes some sense to do as a default, but it would break backwards compatibility. For example:

DB.transaction do
  DB.transaction(:savepoint=>true, :rollback=>:always) do
    DB.after_rollback{it_rolled_back}
    DB.after_commit{it_committed}
  end
end

Currently, this will call it_committed, not it_rolled_back, even though the savepoint rolled back, as the transaction itself was committed.

I think if we wanted this, we may want to consider using a separate API (after_savepoint_rollback and after_savepoint_release). Since this is probably needed only in very rare cases, an extension is probably more appropriate than supporting this in core.

IMO, anyone using after_commit or after_rollback should be using non-transactional testing for those parts of their application.

Do you have a use case in mind for this outside of testing?

[chanks]

I suppose we're writing our applications differently, because I don't really see how it's practical to use non-transactional tests for after_commit and after_rollback logic? Should those parts of the application be separate enough from other concerns that that's doable? For instance, I have a couple places that call after_commit in the after_save callback of a model, so how should I test that behavior other than saving the model? And what's the reliable alternative to clean up that save other than wrapping each test in a transaction? I could iterate through the tables that I think will be touched by a given save and delete those new records, but that seems a lot more brittle and prone to failure to me.

So, I personally use transactional testing for every test that touches the DB at all (which necessarily includes all of the tests that invoke after_commit). I'm not really aware of a better alternative. And I'm going to want to use this :savepoint option (or whatever equivalent there is) in every place I use after_commit or after_rollback. At least, I can't think of a case where I wouldn't want to use it, if I want to make sure that my tests cover every case where I reach out to external services. We could take a poll on sequel-talk, I suppose, but I don't see this desire as being very rare at all.

As for use cases outside of testing, one current case I have is related to a database extension I have that handles advisory locks on Postgres, since they're non-transactional. Here's a simplified version of the code:

def with_advisory_lock(int)
  synchronize do |conn|
    begin
      self["SELECT pg_advisory_lock(?)", int].get
      yield
    ensure
      # If the transaction is being rolled back due to an error, the advisory
      # lock won't be released automatically but the unlock query will fail. So,
      # in that case, issue the unlock after the DB rolls back.
      if conn.transaction_status == PG::PQTRANS_INERROR
        after_rollback { self["SELECT pg_advisory_unlock(?)", int].get }
      else
        self["SELECT pg_advisory_unlock(?)", int].get
      end
    end
  end
end

So in this case I'm waiting until after the rollback to run some SQL, because Postgres will fail if I try to do it before the rollback. And since this is an extension I'd like this code to be able to behave properly when it's called within a savepoint as well, but unfortunately, currently, that advisory lock won't be released until the entire transaction has completed, which I think is going to be unexpected behavior to someone using my extension.

[jeremyevans]

Non-transactional testing is very common in certain types of tests, basically any that require external programs, such as browser-simulator tests. It's definitely a pain compared to transactional testing, but sometimes you have to do it.

I can't see how you could use this correctly in your code without doing something like after_commit(:savepoint=>in_test_mode?) (i.e. make it depend on whether you are testing). That seems like it would cause problems with nested savepoints, as the after_commit in testing would be called at a different point than when doing testing than when it is in production. I just can't see how after_commit(:savepoint=>true) could make sense in production code.

I can think of an alternative approach to implement what you want. Add a :callbacks Database#transaction option. If given and true, callbacks are run for the current transaction/savepoint. If given and false, callbacks are not run for the current transaction/savepoint. If not given (or nil), callbacks are only run if this is the end of the transaction and not a savepoint. If given and :savepoint, the callbacks will be run inside each savepoint inside the current transaction (as if the savepoint used :callbacks=>true). Then you could just have your test script use :auto_savepoint=>true, :callbacks=>:savepoint in the containing transaction, without requiring changes or conditionals in your code. Do you see any issues with that approach?

[chanks]

I think that's a good solution to the testing problem. What do you suggest as a solution to the advisory_lock situation I outlined? I think that using after_rollback to delay a bit of SQL until the savepoint has been rolled back to is a reasonable use case.

[jeremyevans]

Is there a problem with just requiring a savepoint for the advisory lock portion?:

def with_advisory_lock(int)
  transaction(:savepoint=>:only) do |conn|
    get{pg_advisory_lock(int)}
    yield
  end
ensure
  get{pg_advisory_unlock(int)}
end

[chanks]

I think it makes the abstraction a bit leaky to open a savepoint when the caller isn't necessarily expecting it. It means that a raised Sequel::Rollback won't do what the caller expects, though I guess there's rollback: :reraise for that. Not sure what other side effects a non-obviously opened savepoint might cause.

[jeremyevans]

You would probably want to use :rollback=>:reraise. But other than documenting that with_advisory_lock creates a savepoint if inside a transaction, I'm not sure what else you would need to do.

I'm going to close this now since it seems the transaction option is a better way forward. I think the transaction option could be implemented as an external extension, though it may be helpful for the transaction code to be refactored a bit to extract a method or two. Please do send in pull requests for refactoring that makes implementing the external extension easier. If you'd like the extension to be shipped with Sequel, please open a thread on sequel-talk and see what the community thinks about the idea.