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 leaked db connections in SpringTransactionManager #167

Merged
merged 1 commit into from
Aug 11, 2021
Merged

Fix leaked db connections in SpringTransactionManager #167

merged 1 commit into from
Aug 11, 2021

Conversation

amseager
Copy link
Contributor

@amseager amseager commented Aug 11, 2021

Fixes #134

Thanks to @GrannyOlli and @kzkvv for pointing it out.

To sum up: the method inTransactionReturnsThrows of SpringTransactionManager is marked as @Transactional but this annotation takes effect only if this method is called outside its class (e.g. from here).

Currently, there are 4 default methods of TransactionManager which both call it too, and, in fact, from the same class (SpringTransactionManager inherits this interface), so @Transactional doesn't work in this case because of Spring AOP restrictions. In some cases, this can cause errors (e.g. a connection can be left open after an exception occurring inside the transaction).

In this PR, I overrode those methods with the same implementations but also added @Transactional for them. So now, a new transaction will be created while calling them too. On the other hand, inTransactionReturnsThrows can be safely called inside those methods because, in this case, its @Transactional is ignored and there won't be extra transactions spawned (as it could be if we call interface default methods with the super keyword).

Another solution would be to move inTransactionReturnsThrows to a different class, but since it's a Spring-only related issue, I think it's better to leave it as is.

@badgerwithagun
Copy link
Member

Great! I will take the Spring users' word for this. Looks good.

@badgerwithagun badgerwithagun merged commit b5659de into gruelbox:master Aug 11, 2021
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

Successfully merging this pull request may close these issues.

Connections not closed with SpringTransactionManager
2 participants