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 transaction callback throw handling #1257

Merged

Conversation

@jurko-gospodnetic
Copy link
Collaborator

@jurko-gospodnetic jurko-gospodnetic commented Mar 8, 2016

Originally if a transaction callback threw an error, knex's & database's transaction tracking got out of sync:

  • knex considered the transaction completed & rejected its transaction promise
  • DB was not told to roll back its transaction so the used database connection got returned to the connection pool without its transaction actually getting rolled back

Effect would be random database deadlocks and/or operations failures later on due to application code running under a transaction when it does not expect to be, e.g. it can cause that transaction to start holding database locks and thus block other regular transactions, and all of it at random depending on the order in which connections get grabbed from the knex connection pool.

This changes the behaviour by treating such thrown exceptions the same as if a rejected promise had been returned from them and rolls back the underlying database transaction.

The pull request also does some minor code cleanup to keep the code directly touched by it in sync with the rest of the code.

Initial cleanup:

  • 85b780d sync test comments about oracle & mssql not reporting BEGIN/ROLLBACK
  • 3d8131b add missing tape assertion descriptions

Actual fix:

  • 84f85be sync knex & DB transaction state on trx callback error throw

Test suite updates:

  • 3225ab4 test transaction callback throwing an error
  • 96bd74e remove now redundant test case

Doc updates:

  • 20843aa document handling errors thrown from an exception handler function
The default `should be equal` description is not of much use in tracking
down failed assertions.
Previously throwing an error directly from a transaction callback
resulted in knex reporting that transaction's promise as rejected, and
releasing used connection (back to the connection pool), but not
telling the database to roll back that connection's transaction.
Already covered directly by the `transaction rollback on error throw`
test case.
@jurko-gospodnetic
Copy link
Collaborator Author

@jurko-gospodnetic jurko-gospodnetic commented Mar 8, 2016

Could someone run the test suite on mssql and oracle? I don't see those tested on Travis, and I don't have the environment for them set up. The fix is DB agnostic, so that should not be affected but it would be great if someone could make sure the test code does not break when used with those systems (their knex interface is a bit different than the one for the other systems in that they do not report their BEGIN & ROLLBACK commands as knex queries).

jurko-gospodnetic added a commit to jurko-gospodnetic/knex that referenced this pull request Mar 8, 2016
Only left alone ones in `test/tape/transactions.js` which would just
cause unnecessary conflicts and get cleaned up by separate pull
request knex#1257 anyway.
@jurko-gospodnetic
Copy link
Collaborator Author

@jurko-gospodnetic jurko-gospodnetic commented Mar 8, 2016

One test currently reported as failing is completely random (happened only once and that after a documentation update commit 😅) - due to some unrelated and badly written test code that did not expect to be running on a database system performant enough to finish its query before its 1 ms timeout kicked in.

Same effect noticed on pull request #1258.

@jurko-gospodnetic
Copy link
Collaborator Author

@jurko-gospodnetic jurko-gospodnetic commented Mar 8, 2016

This pull request supersedes pull request #928.

@jurko-gospodnetic
Copy link
Collaborator Author

@jurko-gospodnetic jurko-gospodnetic commented Mar 9, 2016

@wubzz - whom to ask to review/approve/merge this? If it helps, we can do a Google hangout session or something similar and I can help by explaining things directly.

Not being altruistic here, I'd just like to get this in so I can stop maintaining my own project forks for some projects that need these fixes. 😜

@wubzz
Copy link
Member

@wubzz wubzz commented Mar 9, 2016

@jurko-gospodnetic So the gist of this PR is to enforce automatic Rollbacks if any type of error (intentional or not) is thrown within the transactors container?

Just wanna double check I understand this correctly.

@jurko-gospodnetic
Copy link
Collaborator Author

@jurko-gospodnetic jurko-gospodnetic commented Mar 9, 2016

Yup.

We already do that partially by rejecting the transaction promise (the one returned by the db.transaction() call) when that happens. Unfortunately we also run the begin()/savepoint() operation on that transaction but then forget to run a corresponding rollback one.

That is bad because we then put the database connection back into the underlying database connection pool, and anyone using it later on does their work in that transaction without knowing that the transaction even exists. Meaning - unexpected locks and/or errors...

So in fact, before this fix, knex is lying to the caller (and in fact, internally to itself) that the transaction has been rolled back. 😄

@wubzz
Copy link
Member

@wubzz wubzz commented Mar 9, 2016

@jurko-gospodnetic That makes sense. Thanks!

wubzz added a commit that referenced this pull request Mar 9, 2016
…w-handling

Fix transaction callback throw handling
@wubzz wubzz merged commit de22fd4 into knex:master Mar 9, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jurko-gospodnetic
Copy link
Collaborator Author

@jurko-gospodnetic jurko-gospodnetic commented Mar 9, 2016

thanks!!!!

@jurko-gospodnetic jurko-gospodnetic deleted the jurko-gospodnetic:fix-trx-callback-throw-handling branch Mar 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants