Skip to content

Query with timer can throw IllegalThreadStateException#474

Merged
cheenamalhotra merged 2 commits intomicrosoft:devfrom
maiken2051:master
Oct 12, 2017
Merged

Query with timer can throw IllegalThreadStateException#474
cheenamalhotra merged 2 commits intomicrosoft:devfrom
maiken2051:master

Conversation

@maiken2051
Copy link
Copy Markdown

@maiken2051 maiken2051 commented Sep 1, 2017

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.

@mnhubspot
TimeoutTimer: Check for destroyed TheadGroup
4213afc 
Running a query that uses a timer can cause an
IllegalThreadStateException if the underlying ThreadGroup has been
destroyed
@mnhubspot
TimeoutTimer: Forgot reference
9da95c5 
Forgot to add AtomicReference
@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 1, 2017 •
edited
Loading

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
Copy Markdown
Contributor

AfsanehR-zz commented Sep 1, 2017

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

@ajlam
Copy link
Copy Markdown
Member

ajlam commented Sep 5, 2017 •
edited
Loading

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
Copy Markdown
Author

maiken2051 commented Sep 5, 2017

Thanks for rebasing the PR...

@ajlam
Copy link
Copy Markdown
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.

@microsoft microsoft deleted a comment from msftclas Sep 26, 2017
@peterbae peterbae self-requested a review October 2, 2017 22:06
@cheenamalhotra
Copy link
Copy Markdown
Member

cheenamalhotra commented Oct 5, 2017

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
Copy Markdown
Author

maiken2051 commented Oct 6, 2017

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
Copy Markdown
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
Copy Markdown
Author

maiken2051 commented Oct 11, 2017

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
Copy Markdown
Contributor

peterbae commented Oct 12, 2017

I see, thanks - please merge this pull request 👍

@cheenamalhotra cheenamalhotra merged commit 06cb754 into microsoft:dev Oct 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cheenamalhotra cheenamalhotra cheenamalhotra approved these changes

+1 more reviewer

@peterbae peterbae peterbae approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@maiken2051 @codecov-io @AfsanehR-zz @ajlam @cheenamalhotra @peterbae @lilgreenbird @msftclas @msftgits @mnhubspot