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

exception handling in knex 0.8.0+ transactions #832

Closed
jurko-gospodnetic opened this Issue May 19, 2015 · 2 comments

Comments

Projects
None yet
1 participant
@jurko-gospodnetic
Copy link
Collaborator

jurko-gospodnetic commented May 19, 2015

When porting our application to use knex 0.8.5 instead of knex 0.7.5 I encountered the following problem:

When creating a transaction, as in:

knex.transaction(function f() {...})...

exceptions thrown by the passed transaction callback f are handled differently than before. In previous knex versions those exceptions would get caught and reported as a rejected transaction promise.

New knex version seems to simply ignore those exceptions, leaving the transaction effectively unresolved forever.

I can easily convert our explicitly thrown exceptions to returning an already rejected promise, but my fear is that some accidental exceptions will cause some transaction to hang and keep holding on to locked database records, thus effectively causing more and more application operations to hang indefinitely.

My suggestion would be to catch those transactions and report them as a rejected transaction promise, same as was done in earlier knex versions.

@tgriesser tgriesser closed this in 4704280 May 20, 2015

@jurko-gospodnetic

This comment has been minimized.

Copy link
Collaborator Author

jurko-gospodnetic commented May 20, 2015

+1 :-) nice commit...

@jurko-gospodnetic

This comment has been minimized.

Copy link
Collaborator Author

jurko-gospodnetic commented Mar 8, 2016

This fix actually broke something else, and even more dangerous - it will reject the transaction promise, but leave the underlying database transaction active.

I have not researched whether that was the case before or not, opened pull request #1257 fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.