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

Fix unhandled rejection if errors occur in a committed transaction. Fixes #3689 #3726

Merged
merged 1 commit into from May 17, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.md
Expand Up @@ -2,6 +2,7 @@
- [BUG] fix showIndexQuery so appropriate indexes are returned when a schema is used
- [BUG] Fix addIndexQuery error when the model has a schema
- [BUG] Fix app crash in sqlite while running in special unique constraint errors [3730](https://github.com/sequelize/sequelize/pull/3730)
- [BUG] Fix trying to roll back a comitted transaction if an error occured while comitting resulting in an unhandled rejection [3726](https://github.com/sequelize/sequelize/pull/3726)

# 2.1.3
- [BUG] Fix regression introduced in 2.1.2: updatedAt not set anymore [3667](https://github.com/sequelize/sequelize/pull/3667)
Expand Down
8 changes: 6 additions & 2 deletions lib/sequelize.js
Expand Up @@ -1149,9 +1149,13 @@ module.exports = (function() {
});
});
}).catch(function(err) {
transaction.rollback().finally(function () {
if (transaction.finished === 'commit') {
reject(err);
});
} else {
transaction.rollback().finally(function () {
reject(err);
});
}
});
};

Expand Down
24 changes: 24 additions & 0 deletions test/integration/transaction.test.js
Expand Up @@ -73,6 +73,30 @@ describe(Support.getTestDialectTeaser('Transaction'), function() {
expect(t.finished).to.be.equal('rollback');
});
});

if (dialect === 'postgres' || dialect === 'mssql') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the issue with mysql here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mysql returns the values and does not throw. But that does not really matter as my fix works for every dialect if something like that happens in that dialect.

it('do not rollback if already committed', function() {
var SumSumSum = this.sequelize.define('transaction', {
value: {
type: Support.Sequelize.DECIMAL(10, 3),
field: 'value'
}
})
, transTest = function (val) {
return self.sequelize.transaction({isolationLevel: 'SERIALIZABLE'}, function(t) {
return SumSumSum.sum('value', {transaction: t}).then(function (balance) {
return SumSumSum.create({value: -val}, {transaction: t});
});
});
}
, self = this;

return SumSumSum.sync({force: true}).then(function () {
return (expect(Promise.join(transTest(80), transTest(80))).to.eventually.be.rejectedWith('could not serialize access due to read/write dependencies among transactions'));
});
});
}

});

it('does not allow queries after commit', function() {
Expand Down
5 changes: 5 additions & 0 deletions test/support.js
Expand Up @@ -17,7 +17,12 @@ chai.config.includeStack = true;
chai.should();

// Make sure errors get thrown when testing
process.on('uncaughtException', function(e, promise) {
console.error('An unhandled exception occured:');
throw e;
});
Sequelize.Promise.onPossiblyUnhandledRejection(function(e, promise) {
console.error('An unhandled rejection occured:');
throw e;
});
Sequelize.Promise.longStackTraces();
Expand Down