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

Transaction `finished` race condition #5222

Closed
greghart opened this Issue Jan 14, 2016 · 2 comments

Comments

3 participants
@greghart

greghart commented Jan 14, 2016

There is logic in a transaction that basically flags a transaction as finished, which is then checked against when querying against that transaction. However, there is a race condition as both .rollback and .commit don't set this flag until after the "ROLLBACK" and "COMMIT" queries have succeeded.

See https://github.com/sequelize/sequelize/blob/master/lib/transaction.js#L209 that the flag is set in the finally
See the guard at

if (options.transaction && options.transaction.finished) {

Basically something like below is happening:

transaction.rollback()
  queryInterface.rollbackTransaction()
...
some other code gets to run and does an update
...
  .finally is triggered

The below code will reproduce

database.transaction(function(t){
  // Forced, but original use case was something like Promise.all([...]).catch (err) -> t.rollback()
  t.rollback();
  database.query("UPDATE entities SET name='something or other'");
});

Observe something like

Executing (5f84266c-9e62-4c96-8040-531e8d765ede): START TRANSACTION;
...
Executing (5f84266c-9e62-4c96-8040-531e8d765ede): ROLLBACK;
...
Executing (5f84266c-9e62-4c96-8040-531e8d765ede): UPDATE entities SET name`='something or other'

The update goes out even though the transaction was just rolled back.
I believe the fix would be to just set the finished flag right after sending the ROLLBACK out, rather than in the .finally.

Minor overall, as easily avoided by just making sure all your parallel requests are done before running the rollback.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 15, 2016

Moving to before the call seems the best course of action.
Thanks for a good reports with steps to fix, a PR would be welcome.

ajfranzoia added a commit to ajfranzoia/sequelize that referenced this issue Jan 17, 2016

ajfranzoia added a commit to ajfranzoia/sequelize that referenced this issue Jan 17, 2016

@ajfranzoia

This comment has been minimized.

Contributor

ajfranzoia commented Jan 17, 2016

Tried to move the this.finished = 'rollback' assignment before the call in https://github.com/sequelize/sequelize/blob/master/lib/transaction.js#L207 but it causes the final rollback query generated by QueryInterface.rollbackTransaction() to fail because the query detects the transaction is now finished (https://github.com/sequelize/sequelize/blob/master/lib/sequelize.js#L777).

Moving the assignment after the rollback transaction call in QueryInterface.rollbackTransaction() seems to prevent this issue and solve the original problem as well.
Created PR #5243.

ajfranzoia added a commit to ajfranzoia/sequelize that referenced this issue Jan 23, 2016

mickhansen added a commit that referenced this issue Jan 24, 2016

Merge pull request #5243 from ajfranzoia/fix-transaction-finished-rac…
…e-condition

Fix #5222: prevent race condition after transaction finished
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment