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

Query with timer can throw IllegalThreadStateException #474

Merged
merged 2 commits into from
Oct 12, 2017
Merged

Query with timer can throw IllegalThreadStateException #474

merged 2 commits into from
Oct 12, 2017

Conversation

maiken2051
Copy link

Running a query that uses a timer can cause an IllegalThreadStateException if the underlying ThreadGroup has been destroyed.

This error happens when the caller executes a query with a timeout and then doesn't execute another query with a timeout until some time has elapsed. The JVM can destroy the ThreadGroup behind the TimeoutTimer (because it contains no active threads) which causes any new query with a timeout to throw an IllegalThreadStateException.

Once the JDBC driver has entered this state any new query with a timeout will throw the exception. It's especially easy to run into this problem in demanding / multi-threaded applications.

My change makes sure the ThreadGroup is not destroyed before creating a new timer thread. If the ThreadGroup is destroyed, the code will create a new ThreadGroup.

Running a query that uses a timer can cause an
IllegalThreadStateException if the underlying ThreadGroup has been
destroyed
Forgot to add AtomicReference
@codecov-io
Copy link

codecov-io commented Sep 1, 2017

Codecov Report

Merging #474 into dev will decrease coverage by 0.03%.
The diff coverage is 83.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #474      +/-   ##
============================================
- Coverage     46.25%   46.21%   -0.04%     
+ Complexity     2206     2195      -11     
============================================
  Files           108      108              
  Lines         25231    25215      -16     
  Branches       4173     4166       -7     
============================================
- Hits          11671    11654      -17     
+ Misses        11541    11535       -6     
- Partials       2019     2026       +7
Flag Coverage Δ Complexity Δ
#JDBC41 46.04% <83.33%> (+0.03%) 2184 <0> (-9) ⬇️
#JDBC42 46.12% <83.33%> (-0.05%) 2192 <0> (-11)
Impacted Files Coverage Δ Complexity Δ
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java 53.7% <83.33%> (+0.34%) 0 <0> (ø) ⬇️
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 43.37% <0%> (-2.03%) 103% <0%> (-2%)
.../com/microsoft/sqlserver/jdbc/SQLServerDriver.java 75.44% <0%> (-1.52%) 25% <0%> (ø)
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 50.91% <0%> (-0.66%) 161% <0%> (-4%)
...oft/sqlserver/jdbc/SQLServerParameterMetaData.java 23.98% <0%> (-0.38%) 31% <0%> (ø)
...m/microsoft/sqlserver/jdbc/SQLServerStatement.java 59.06% <0%> (-0.31%) 129% <0%> (-1%)
...soft/sqlserver/jdbc/SQLServerDatabaseMetaData.java 29.47% <0%> (-0.23%) 48% <0%> (-1%)
...n/java/com/microsoft/sqlserver/jdbc/DataTypes.java 77.62% <0%> (-0.17%) 4% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.48% <0%> (-0.17%) 239% <0%> (-2%)
.../microsoft/sqlserver/jdbc/SQLServerConnection.java 45.71% <0%> (-0.1%) 273% <0%> (-1%)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7fee51...9da95c5. Read the comment docs.

@AfsanehR-zz
Copy link
Contributor

@maiken2051 Thank you for your effort in creating this pr. Could you please open this to dev branch?

@ajlam
Copy link
Member

ajlam commented Sep 5, 2017

Hi @maiken2051, I just rebased this against the dev branch.

@ajlam ajlam changed the base branch from master to dev September 5, 2017 22:22
@maiken2051
Copy link
Author

Thanks for rebasing the PR...

@ajlam
Copy link
Member

ajlam commented Sep 21, 2017

@maiken2051, just wanted to keep you posted. We are going to be doing a code review soon. Thanks again for your contributions.

@cheenamalhotra
Copy link
Member

Hi @maiken2051 Please sign CLA so that we can proceed with merging your PR.

@peterbae peterbae requested review from AfsanehR-zz and removed request for peterbae October 6, 2017 00:01
@maiken2051
Copy link
Author

I think the CLA should be OK now?

@peterbae peterbae requested review from peterbae and removed request for AfsanehR-zz October 6, 2017 17:26
@peterbae peterbae requested review from peterbae and removed request for peterbae October 6, 2017 18:38
@peterbae
Copy link
Contributor

peterbae commented Oct 6, 2017

Hi @maiken2051, thanks for the contribution. The changes look good, and I approved it - but I'm curious as to where I can find the information regarding JVM destroying a ThreadGroup that contains no active threads. Could you point me to where I can read more about this?

@maiken2051
Copy link
Author

Check out the source for ThreadGroup.threadTerminated. This is from OpenJDK but I assume all JVM implementations are similar. The docs say:

Notifies the group that the thread t has terminated. Destroy the group if all of the following conditions are true: this is a daemon thread group; there are no more alive or unstarted threads in the group; there are no subgroups in this thread group.

When a thread exits it will call this method to remove itself from the active list on its owner ThreadGroup. If it's the last active thread the group will be destroyed.

@peterbae
Copy link
Contributor

I see, thanks - please merge this pull request 👍

@cheenamalhotra cheenamalhotra merged commit 06cb754 into microsoft:dev Oct 12, 2017
@lilgreenbird lilgreenbird added this to Closed/Merged PRs in MSSQL JDBC Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed/Merged PRs
Development

Successfully merging this pull request may close these issues.

None yet

9 participants