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
merged 7 commits into from Mar 9, 2016

Conversation

Projects
None yet
2 participants
@jurko-gospodnetic
Collaborator

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

jurko-gospodnetic added some commits Mar 8, 2016

sync knex & DB transaction state on trx callback error throw
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.
add missing tape assertion descriptions
The default `should be equal` description is not of much use in tracking
down failed assertions.
remove now redundant test case
Already covered directly by the `transaction rollback on error throw`
test case.
@jurko-gospodnetic

This comment has been minimized.

Collaborator

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

trim trailing spaces
Only left alone ones in `test/tape/transactions.js` which would just
cause unnecessary conflicts and get cleaned up by separate pull
request tgriesser#1257 anyway.
@jurko-gospodnetic

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

jurko-gospodnetic commented Mar 8, 2016

This pull request supersedes pull request #928.

@jurko-gospodnetic

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

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

This comment has been minimized.

Collaborator

wubzz commented Mar 9, 2016

@jurko-gospodnetic That makes sense. Thanks!

wubzz added a commit that referenced this pull request Mar 9, 2016

Merge pull request #1257 from jurko-gospodnetic/fix-trx-callback-thro…
…w-handling

Fix transaction callback throw handling

@wubzz wubzz merged commit de22fd4 into tgriesser:master Mar 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jurko-gospodnetic

This comment has been minimized.

Collaborator

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