Skip to content

tomcat-jdbc check if returned connection is closed #235

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

panchenko
Copy link

@panchenko panchenko commented Jan 16, 2020

https://bz.apache.org/bugzilla/show_bug.cgi?id=64083

I had a situation when driver closed connection because of I/O error, bur connection has been added to the avaialble ones and then returned to another caller.
The testOn* properties were not enabled in that case.

It looks like the pool can easily check if connection has been closed and don't accept it back.
WDYT?

Copy link
Contributor

@ChristopherSchultz ChristopherSchultz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this test-case actually tests the change. There is no check to see if the connection in the pool is not damaged, or a complaint if it is damaged.

@panchenko
Copy link
Author

panchenko commented Aug 28, 2021

The test checks that connection obtained from the pooll second time can be used for query execution.
Without this pool code change it returns the closed connection and query exection fails.

I agree the intent is not fully clear, as it tests it as black-box, I will check if some white-box asserts can be added.

@zyu-godaddy
Copy link

Also reported: https://bz.apache.org/bugzilla/show_bug.cgi?id=66502

This bug is the only bug that has been bugging us; otherwise the pool works flawlessly.

I wonder if you people want to revisit this issue; or I can offer to work on it.

@markt-asf
Copy link
Contributor

If you can address Chris's concerns with the test then I don't see why this PR (or one based on it) could not be merged.

@zyu-godaddy
Copy link

zyu-godaddy commented Jun 5, 2025

Cool, I will.

@zyu-godaddy
Copy link

While this change makes sense to me, and it should be inexpensive for the jdbc driver I am using (mysql-connector-java), I don't feel comfortable to add this check unconditionally without knowing the full impact. So I am not going to pursue this approach.

Instead, I will try setDiscarded(true) in an interceptor conditional on failed query and isClosed.

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.

4 participants