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

Fix transactions not rolling back properly #3661

Merged
merged 1 commit into from May 7, 2015

Conversation

3 participants
@BridgeAR
Contributor

BridgeAR commented May 6, 2015

The pool of transactions will be drained if errors occur in the transaction block.

Without the patch the following will result in:

sequelize.transactions(function(t) {
    throw new Error('test');
});

Executing (280fa819-4f22-4c4d-a511-3a262e1040a0): START TRANSACTION;
Executing (280fa819-4f22-4c4d-a511-3a262e1040a0): SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ;
Executing (280fa819-4f22-4c4d-a511-3a262e1040a0): SET autocommit = 1;


Missing: Executing (280fa819-4f22-4c4d-a511-3a262e1040a0): ROLLBACK;

@janmeier

This comment has been minimized.

Member

janmeier commented May 6, 2015

LGTM - a test could run the code in your example and then check that transaction.finished = 'rollback' https://github.com/sequelize/sequelize/blob/master/lib/transaction.js#L109

Ruben Bridgewater
Fix transactions not rolling back properly
Fix test to actually check for the rollback happening
});
}).catch(reject);
}).catch(function(err) {

This comment has been minimized.

@mickhansen

mickhansen May 7, 2015

Contributor

We specifically changed it to then(null, cb) becaues catch is not in the Promises/A standard and not supported by a lot of non-bluebird lib.

This comment has been minimized.

@mickhansen

mickhansen May 7, 2015

Contributor

Nevermind that comment, that shouldn't matter with the new approach here actually.

}).catch(function(err) {
transaction.rollback().then(function () {
reject(err);
}, function () {

This comment has been minimized.

@mickhansen

mickhansen May 7, 2015

Contributor

.catch(reject) might be sufficient here.

mickhansen added a commit that referenced this pull request May 7, 2015

Merge pull request #3661 from BridgeAR/fix-transactions
Fix transactions not rolling back properly

@mickhansen mickhansen merged commit 74262d3 into sequelize:master May 7, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment