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

Rollback transactions on panic #1646

Closed
Diggsey opened this Issue Apr 17, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@Diggsey
Contributor

Diggsey commented Apr 17, 2018

Related to sfackler/r2d2#31

Transactions should be rolled back if the inner function panics. The rollback code should take care not to panic in this case, to avoid double panics. Failure to do this can result in connections being returned to the pool with open transactions, which means database locks are held indefinitely, and future uses of the connection will cause further panics.

@sgrif

This comment has been minimized.

Member

sgrif commented Apr 19, 2018

I disagree that this should be handled in Diesel. Transactions that do not explicitly commit will be rolled back when the connection is dropped. This is a problem specific to connection pooling, and should be handled there.

@sgrif sgrif closed this Apr 19, 2018

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Apr 19, 2018

I strongly disagree with this decision: the transaction function creates a scope for the transaction, and it is fully expected that the transaction not be open after the end of that scope, regardless of how the scope is left. That's not a problem that's specific to connection pooling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment