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: EXPOSED-651 Try to close connection in ThreadLocalTransactionManager#connectionLazy if setup fails #2320

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

m-burst
Copy link
Contributor

@m-burst m-burst commented Nov 29, 2024

Description

Summary of the change: Try to close connection in ThreadLocalTransactionManager#connectionLazy if setup fails

Detailed description:

  • What: The connection setup in ThreadLocalTransactionManager#connectionLazy now catches exceptions and attempts to close the database connection in case of an exception.
  • Why: After the connection is obtained from the Database, Exposed tries to perform some initial setup before using the connection in a Transaction. If this setup fails, the connection is not closed (neither immediately nor when closing the transaction, because the connection is not stored inside the transaction due to lazy semantics), which in turn leads to connection leak.
  • How: The setup code is wrapped in a try-catch block, which catches all exceptions and tries to close the connection before rethrowing.

Type of Change

Please mark the relevant options with an "X":

  • Bug fix
  • New feature
  • Documentation update

Updates/remove existing public API methods:

  • Is breaking change

Affected databases:

  • MariaDB
  • Mysql5
  • Mysql8
  • Oracle
  • Postgres (not sure which other databases are affected, but we ran into this problem with Postgres)
  • SqlServer
  • H2
  • SQLite

Checklist

  • Unit tests are in place — not sure how to test this, we used a heavily mocked java.sql.Driver to reproduce
  • The build is green (including the Detekt check)
  • All public methods affected by my PR has up to date API docs (no API changes)
  • Documentation for my change is up to date — please let me know if documentation changes are needed

Related Issues

EXPOSED-651

@joc-a joc-a self-assigned this Dec 4, 2024
@joc-a
Copy link
Collaborator

joc-a commented Dec 4, 2024

Hi @m-burst. Thank you for your contribution. Please add a description to your pull request following the format that was set up when you first opened it as outlined here.

@m-burst
Copy link
Contributor Author

m-burst commented Dec 4, 2024

Hi @joc-a, I've added the description

@joc-a joc-a merged commit a17161d into JetBrains:main Dec 5, 2024
3 checks passed
@m-burst m-burst deleted the EXPOSED-651 branch December 9, 2024 09:29
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.

2 participants