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.

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

Hook queries are not run inside surrounding transaction #9472

Closed
msuret opened this issue May 23, 2018 · 2 comments · Fixed by #9473
Closed

Hook queries are not run inside surrounding transaction #9472

msuret opened this issue May 23, 2018 · 2 comments · Fixed by #9473

Comments

@msuret
Copy link

msuret commented May 23, 2018

What are you doing?

I want to save a model instance with an afterCreate hook within a transaction.

const Sequelize = require('sequelize');
const namespace = require('cls-hooked').createNamespace('Sequelize');
Sequelize.useCLS(namespace);

const sequelize = new Sequelize(null, null, null, {
 dialect: 'sqlite',
 storage: ':memory:'
});


const User = sequelize.define('user', {
  name: Sequelize.STRING
}, {
  hooks: {
    afterCreate: (user, options) => {
        if(!namespace.get('transaction')){
            throw new Error('afterCreate hook is not running in a transaction!');
        }
    }
  }
});

User.sync().then(() => {
  sequelize.transaction(t => {
      return User.create({
        name: 'John'
      });
  })
});

What do you expect to happen?

The code above should not throw any exception because User.create is called inside a transaction.
Any query run in the hook should be part of that transaction.
More generally, the CLS context of the transaction should be accessible from the hook.

What is actually happening?

The code above throws an exception.
The CLS chain is broken and not accessible in hooks.

Output:

Executing (default): CREATE TABLE IF NOT EXISTS `users` (`id` INTEGER PRIMARY KEY AUTOINCREMENT, `name` VARCHAR(255), `createdAt` DATETIME NOT NULL, `updatedAt` DATETIME NOT NULL);
Executing (default): PRAGMA INDEX_LIST(`users`)
Executing (99b8a32e-17f0-4f52-af15-091eb5aa411e): BEGIN DEFERRED TRANSACTION;
Executing (99b8a32e-17f0-4f52-af15-091eb5aa411e): INSERT INTO `users` (`id`,`name`,`createdAt`,`updatedAt`) VALUES (NULL,'John','2018-05-23 13:36:43.884 +00:00','2018-05-23 13:36:43.884 +00:00');
Executing (99b8a32e-17f0-4f52-af15-091eb5aa411e): ROLLBACK;
Unhandled rejection Error: afterCreate hook is not running in a transaction !
    at Function.afterCreate (sequelize-issue\main.js:17:19)
    at Promise.each.hook (sequelize-issue\node_modules\sequelize\lib\hooks.js:130:35)
    at clsBind (sequelize-issue\node_modules\cls-hooked\context.js:172:17)
    at tryCatcher (sequelize-issue\node_modules\bluebird\js\release\util.js:16:23)
    at Object.gotValue (sequelize-issue\node_modules\bluebird\js\release\reduce.js:155:18)
    at Object.gotAccum (sequelize-issue\node_modules\bluebird\js\release\reduce.js:144:25)
    at Object.tryCatcher (sequelize-issue\node_modules\bluebird\js\release\util.js:16:23)
    at Promise._settlePromiseFromHandler (sequelize-issue\node_modules\bluebird\js\release\promise.js:512:31)
    at Promise._settlePromise (sequelize-issue\node_modules\bluebird\js\release\promise.js:569:18)
    at Promise._settlePromiseCtx (sequelize-issue\node_modules\bluebird\js\release\promise.js:606:10)
    at Async._drainQueue (sequelize-issue\node_modules\bluebird\js\release\async.js:138:12)
    at Async._drainQueues (sequelize-issue\node_modules\bluebird\js\release\async.js:143:10)
    at Immediate.Async.drainQueues (sequelize-issue\node_modules\bluebird\js\release\async.js:17:14)
    at runCallback (timers.js:794:20)
    at tryOnImmediate (timers.js:752:5)
    at processImmediate [as _immediateCallback] (timers.js:729:5)

This bug was introduced in v4.33.4 because sequelize.query(sql, options) now returns a promise generated by retry-as-promised which is not patched to maintain CLS context.
Surrounding it with Promise.resolve() solves the issue.
I'll do a PR with this fix.

return Promise.resolve(retry(retryParameters => Promise.try(() => {
....
}), retryOptions));

Dialect: any
Sequelize version: 4.33.4
Tested with latest release: Yes (4.37.8)

msuret pushed a commit to msuret/sequelize that referenced this issue May 23, 2018
encapsulate promises returned by `retry-as-promised` with `Promise.resolve` to maintain CLS context

fix sequelize#9472
@StevePavlin
Copy link

+1 for this. I thought this was a scoping issue with arrow functions, downgraded to 4.32.5 and it works.

I think 4.32.7 was the breaking change, downgrading to 4.33.4 still caused the bug for me.

@msuret
Copy link
Author

msuret commented May 28, 2018

@StevePavlin Do you still have this issue with 4.37.10 ? The merge of #9473 solved it for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants