"after" Hooks in combination with transactions - opinion wanted

[iamjochem]

this is a repost of #8115 ... because stalebot came past and I did not notice (I agree with @linpekka's comment on the close issues about stale bot being a bit aggressive - maybe it should leave issues alone so long as a maintainer (and/or contributor?) has not replied?)

hi guys,

I noticed that "after" hooks related to persistence (e.g. "afterUpdate" but not "afterBuild") do not honour transactions.

By which I mean the hooks fire before the transaction is commited and therefore also even when a transaction has been rolled back.

My feeling is that this is not correct because it leads to situations where "after" hooks are triggered for create, update & destroy actions where the actual DB mutation has never taken place (because it was rolled back).

This issue has been raised before (see here), but that is a discussion which has been running for over 2 years without any real resolution ... not suprising given that it's a big hairy thing to attempt to "fix", for most people an edge case that is not relevant and likely has a boat-load of potential BC implications.

My question is whether you agree that the following hooks should be transaction-aware and only fire if a transaction is committed:

afterCreate
afterUpdate
afterDestroy
afterBulkCreate
afterBulkUpdate
afterBulkDestroy

Assuming that you do agree with this, do you have any opinions/insights about how to best implement this?

P.S. Also I wonder whether it should be mentioned (the fact that the "after" hooks are not transaction aware) in the documentation.

---

To follow up on my question I offer the following snippet which I used in my own code to force "after" hook handlers to only fire if a transaction is committed - it works for my use case, maybe it's helpful for someone else and/or could form the basis of a "proper" fix (assuming you agree the issue is something that should be fixed):

const transHooks    = Symbol('sequelize_transaction_hooks_property');   // used as an 'invisible' property on transaction objects, used to stored "after*" hook functions that should only run if the transaction actually commits successfully

function addTransCommitHookHandling(transaction, hookfn)
{
    if (!isfunc(hookfn))    return;
    if (!transaction)       return hookfn();

    if (!transaction[transHooks]) {
        transaction[transHooks] = [];

        const origfn = transaction.commit;

        transaction.commit = function(...args) {
            const prom      = origfn.call(this, ...args),
                  run_hooks = v => transaction[transHooks].forEach(fn => fn()) && v
            ;

            // the following is possibly an overly defensive check for a Promise ...
            return isfunc(prom && prom.then) ? prom.then(run_hooks) : run_hooks(prom);
        };
    }

    transaction[transHooks].push(hookfn);
}

the addTransCommitHookHandling function is then used inside a hook handler function like so:

function myAfterCreateHook(instance, options)
{
     // options.transaction is defined only if there is a transaction in play
     addTransCommitHookHandling(options.transaction, () => {
           // here we would implement our actual "after create" hook logic 
     });
}

[sushantdhiman]

Isnt options.transaction is available, so any database mutation in those hooks can be made transaction aware.

I remember someone requested for non db related mutation that needs to be fired in these hooks. I can think of afterCommit (#2805) or may be simple .then (after callback transaction) to achieve this

[iamjochem]

hi @sushantdhiman,

I'm not sure I understand your comment ... options.transaction is available in the "after" hooks ... the hack above shows it.

manually chaining a .then() after the transaction call is not really desirable, it implies that you have to add/call whatever hook logic you need in every instance in your you perform mutations on a given model ... someone will forget to do it eventually! (the whole point of hooks is that you define them once and they are garanteed to run - kind of "fire and forget")

the whole point here is that I think firing "afterUpdate/Insert/Delete" hooks while the transaction is still in progress is completely wrong ... the transaction may be rolled back! I personally am using "after" hooks for auditing purposes (amonst others) so I had situations where I am logging deletions/updates that never took place (because there was some error at the level of the DB)

I think it is wrong to expect every sequelize user to implement their own transaction aware hooks (it can be solved for everyone in sequelize, which avoids pain/frustration & people complaining that your stuff is "broken") - you say "afterUpdate", my expectation is that when a UPDATE query in a transaction is rolled back there is no "UPdate" therefore there is should be no "afterUpdate".

do you agree with my premise?
do you think the code I offer above could form the basis of some kind of [transparent] imlementation that delays "after" hooks until a transaction (when available) has been committed?

[linpekka]

I think there are good reasons for after hooks that can generate transaction aware db operations, and like @sushantdhiman mentions this is supported. I use it from time to time, and if the transaction rollbacks the operations are also rollbacked.
I also agree that it would be neat to have a hook that supports knowing that a commit has happened, to generate non transaction aware operations, like logs or webhooks or other background jobs, one does not exclude the other.
afterCommit would work for me, but since it does not exist yet workarounds and ideas on how to implement it is always nice. Using .then of the transaction promise very quickly becomes a nightmare, especially for nested transactions.

@iamjochem, I am currently not going to use your workaround, while tests shows that it works, even if I had to make small modifications to fit my needs, I don't feel comfortable overriding the api like this.

That said, not landed a decent solution yet, thanks for the post.

[iamjochem]

@linpekka - I understand your reluctance to do the monkey patching, I would add that I have been using this for some time now without problems :-)

I do not agree with the premise that there is a use case for firing non-transaction aware hooks - AFAIC firing an "afterUpdate" hook in the case that there is a transaction pending and that that transaction is rollbacked is incorrect - the hook is an event that stipulated that something took place, in the case of a rolled back transaction the update did not actually take place (so regardless of what action you wish to take in response to an update the action should not occur because the update did not occur.

In the case of logging update/delete/etc attempts this could/should be done in "before" hooks ... which necessarily are fired regardless of outcome.

for me "afterCommit" would not work because I am interested in the specific action (update/create/delete) ... adding an "afterCommit" hook is orthogonal to the individual "after[Mutation]" hooks, I don't think it is a good idea to conflate the two (it implies having to add extra logic/parameters to the "afterCommit" hook calls in order to facilitate information about the type of action ... that said I see no reason to have an "afterCommit" hook as such (it is a valid lifecycle event in it's own right after all), I do think that there should be an "afterRollback" event to match it. (maybe an "afterTransaction" hook with a parameter that signals success/failure ... people have made the argumen against endless proliferation of hooks ... and I feel that they make a strong point!)

[linpekka]

Valid points, especially the issue with afterCommit not being aware of the specific actions.

I do use afterUpdate with transaction aware operations, when one model i loosely coupled with another and update on one requires actions in another, and I think the use-case is valid and I would not like if the possibility was removed. If the after hook is conceptually fired only when commit has happened, or when the db operation is prepared is a matter of definition after all.

One thing that is nice with your work-around is that it allows both without adding extra hooks.

[iamjochem]

maybe it's an idea, as a first step, to implement something that looks like my workaround as a builtin "hook definition" helper ... this could then be documented and give people a leg up (i.e. make it easier to implement, not having to reinvent a wheel and/or footgunning oneself with regard to making to the wrong assumption with regard to afterHooks) ... what's your opinion @sushantdhiman ?

[asafigan]

I believe that moving all existing hooks to fire after a transaction would break a lot of existing code in non obvious ways. Plus there are use cases for not deferring all hooks. With active record, my team uses hooks to change other records when another is updated. It is nice to know that those changes are being done in a transaction, so that if the transaction is rolled back they are also rolled back. And if for some reason there is an error with one of the hooks, like validation, the entire transaction is rolled back. Having hooks that could be deferred till after commit would be nice. Active record does it pretty well. The even have options to have hooks only fire on some actions. For instance you can create a validation, or validation hook that only fires on create. This can be useful for after commit hooks, because you could make them only fire under some condition, like only after create.

[maenu]

The current behavior is confusing me. As far as I understand now, after hooks do not get a transaction over cls, see the following test:

const Sequelize = require('sequelize')
const cls = require('continuation-local-storage')
Sequelize.useCLS(cls.createNamespace('thing'))
let sequelize = new Sequelize({
	dialect: 'postgres'
	// [...]
})
let Thing = sequelize.define('thing', {
	name: {
		type: Sequelize.STRING
	}
}, {
	hooks: {
		beforeCreate: function () {
			console.log('beforeCreate', 'transaction is set:', Sequelize._cls.get('transaction') != null)
		},
		afterCreate: function () {
			console.log('afterCreate', 'transaction is set:', Sequelize._cls.get('transaction') != null)
		}
	}
})
sequelize.sync().then(() => {
	let thing = new Thing({
		name: 'thong'
	})
	console.log('start transaction')
	return sequelize.transaction(() => {
		return thing.save()
	}).then(() => console.log('end transaction'))
}).then(() => sequelize.drop()).then(() => sequelize.close())

This example outputs:

start transaction
beforeCreate transaction is set: true
afterCreate transaction is set: false
end transaction

I expected the transaction to be set for all queries until the transaction is finished. Not to have it in after hooks seems just wrong to me.

Which leads me to my question: How do I have to setup my after hooks to take the transaction from cls?

[heroic]

@papb this last example is a big problem for me as well. We use CLS with express to have one transaction per HTTP request. In the afterSave we want to notify our worker pool, when the transaction commits. However, right now, the after hook does not receive the transaction from CLS.

[heroic]

@papb Any thoughts on this? Our app relies heavily on sequelize, but current implementation of hooks and cls does not allow us to have auto wrapped transactions on each request, which is causing a lot of issues for the tech team in testing and missed transactions

[github-actions]

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂