Skip to content

Conversation

@jliuhtonen
Copy link
Contributor

Summary

Don't attempt to get connection for closed Statement

Description

It is possible that .getConnectionFromSqlObject() is called for a statement that has already been closed. This is the case when e.g. calling .isClosed() for a closed Statement. In this case, an SQLException is thrown from statement.getConnection() unnecessarily. The exception might also be detected in various APM tool instrumentations leading to false positive errors.

Additional Reviewers

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@davecramer
Copy link
Contributor

thx for the PR!

@jliuhtonen jliuhtonen force-pushed the topic/check-statements-not-closed branch from 1560b70 to 3e6c6f8 Compare October 13, 2023 15:50
@jliuhtonen
Copy link
Contributor Author

I fixed the checkstyleTest failure by amending the previous commit to avoid separate "fix imports" commit. The problem was static wildcard import caused by IDE automatic imports. Thanks!

It is possible that .getConnectionFromSqlObject() is called for a statement that has already been closed. This is the case when e.g. calling .isClosed() for a closed Statement. In this case, an SQLException is thrown from statement.getConnection() unnecessarily. The exception might also be detected in various APM product instrumentations leading to false positive errors.
@jliuhtonen jliuhtonen force-pushed the topic/check-statements-not-closed branch from 3e6c6f8 to 935fa9b Compare October 14, 2023 06:42
@jliuhtonen
Copy link
Contributor Author

Rebased with latest changes from main.

@jliuhtonen
Copy link
Contributor Author

Hi @karenc-bq, would it be possible to rerun the tests on CI? They pass for me locally and I suspect that could there be a timing issue insoftware.amazon.jdbc.plugin.efm.MultiThreadedMonitorThreadContainerTest.testThreadPoolExecutorNotClosedPrematurely?

@karenc-bq karenc-bq merged commit 3ccc721 into aws:main Oct 16, 2023
@karenc-bq
Copy link
Contributor

Thank you for catching and fixing the issue.

@jliuhtonen jliuhtonen deleted the topic/check-statements-not-closed branch October 17, 2023 14:21
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.

3 participants