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

Transaction finished race condition #5222

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

Transaction finished race condition #5222

greghart opened this issue Jan 14, 2016 · 2 comments

Comments

@greghart
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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
…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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants