Replace timeout handling to use SharedTimer

[sehrope]

PR to fix #909 following the reference counting model outlined in the comments. This is an alternative to #914. Was easier to put together a separate PR as the code changes were completely different.

This PR does not track the actual references or connection IDs, just a total count of how many times the SharedTimer has been requested (increment reference) and released (decrement reference). Connections that make use of the SharedTimer cache a reference to it and clean it up in Connection.close(). If the reference count drops to zero then the SharedTimer is shutdown and the core thread used for scheduling interrupt tasks is stopped.

Timeout that are actually triggered, i.e. the timer expired and the action hasn't completed yet, create a new thread on the fly to execute the interrupt action. This is to ensure that they do not block the core timer thread. Timeouts that are cancelled prior to expiring, i.e. query completed prior to timeout, do not create a new handler thread.

All current tests, including timeout tests, pass with this PR but there are no new tests yet to check if things are being cleaned up correctly. Manual testing via jconsole shows that it is but it'd be nice to eventually include something in the core test suite to cover that. Something similar to cheenamalhotra#20 should work though we'd have to deal with the issue of the rest of the test suite cleaning up all its resources prior to running (probably a good idea anyway!).

Alternatively we could have the thread leak tests be in their own test suite that is only run when explicitly enabled. Travis / Appveyor could then run them in isolation as a secondary step via: ENABLE_TIMEOUT_TESTS=true mvn -Dtest=com.microsoft.sqlserver.jdbc.timeouts.TimeoutThreadLeakTest test

I'll be taking a look at adding the tests when I have some more time but if someone else wants to do it in the meantime I can review.

Couple of notes on minor changes in behavior:

• Timeout operations are scheduled via ScheduledThreadPoolExecutor which internally sleeps until the next operation will execute. Previously they were being manually polled every 1-second so it was possible for a timeout to not execute for 2-seconds (ex: poller is running at t=0, t=1, t=2, ... and timeout is scheduled at t=.01). They will now execute as soon as they need to run.
• The clock for cancellation of bulk inserts now starts when the bulk command is going to execute as opposed to when it is created. The only blocking operation in between is waiting on an internal synchronization monitor, schedulerLock, so in practice it'd be microseconds of difference.

[codecov-io]

Codecov Report

Merging #920 into dev will decrease coverage by 0.1%.
The diff coverage is 70%.

@@             Coverage Diff              @@
##                dev     #920      +/-   ##
============================================
- Coverage     50.46%   50.36%   -0.11%     
+ Complexity     2932     2929       -3     
============================================
  Files           120      120              
  Lines         28117    28100      -17     
  Branches       4696     4694       -2     
============================================
- Hits          14190    14152      -38     
- Misses        11629    11645      +16     
- Partials       2298     2303       +5

Flag	Coverage Δ	Complexity Δ
#JDBC42	49.92% <70%> (-0.01%)	2888 <16> (-3)
#JDBC43	50.28% <70%> (-0.14%)	2925 <16> (-5)

Impacted Files	Coverage Δ	Complexity Δ
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java	51.47% <50%> (-0.21%)	255 <0> (-2)
...a/com/microsoft/sqlserver/jdbc/TDSTimeoutTask.java	59.09% <59.09%> (ø)	4 <4> (?)
.../microsoft/sqlserver/jdbc/SQLServerConnection.java	46.51% <75%> (+0.07%)	342 <2> (+3)	⬆️
...java/com/microsoft/sqlserver/jdbc/SharedTimer.java	80% <80%> (ø)	10 <10> (?)
...in/java/com/microsoft/sqlserver/jdbc/IOBuffer.java	55.51% <90.9%> (-0.25%)	0 <0> (ø)
...a/com/microsoft/sqlserver/jdbc/PLPInputStream.java	58.33% <0%> (-1.2%)	33% <0%> (-1%)
...om/microsoft/sqlserver/jdbc/ReaderInputStream.java	43.95% <0%> (-1.1%)	15% <0%> (ø)
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java	46.2% <0%> (-1.09%)	111% <0%> (-1%)
...c/main/java/com/microsoft/sqlserver/jdbc/Util.java	61.77% <0%> (-0.44%)	89% <0%> (-1%)
... and 3 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 57b504f...e3e188b. Read the comment docs.

[cheenamalhotra]

Hi @sehrope

The implementation looks definitely cleaner although a few improvements can be done further to reduce code and achieve final design. I created PR sehrope#2 to your branch, which when merged will be final implementation for the timer thread that we would like to go ahead with.

[sehrope]

Rebased atop latest dev branch and couple of additional commits for cleanup and tests added.

The first commit is the same as before (rebased on dev). The rest are broken down into small chunks to simplify review. Here's a summary:

• Replace timeout handling to use SharedTimer (main work of this PR)
• Normalize newlines in TimeoutTest - Replaces CRNL with just NL (no actual changes)
• Move TimeoutTest to parent package so that it can access package private members (no actual changes)
• Centralize SharedTimer thread checks in TimeoutTest - Adds @Before/@After methods to handle ensuring timer threads don't exist before and after tests run (rather than manually doing it in each test).
• Centralize and reduce timeout value used in TimeoutTest - Pulls up the 10-second timeout used by the existing tests and changes it to two-seconds (test time goes down by 20 seconds, yay!)
• Centralize SQL for WAIT FOR DELAY in TimeoutTest - Pulls up the WAITFOR SQL and formats it a bit to make it easier to read (00:01 always looks like 1-second to me whereas 00:01:00 is clearly 1-minute)
• Clean up TimeoutTest to use assertThrows(...) - Changes existing tests to use assert
• Add additional SharedTimer tests - Adds a couple tests to verify creation and destruction of core timer thread (similar to @cheenamalhotra's tests but split out into a couple separate ones).

I've left in the id field on SharedTimer as one of the tests uses it to verify that the same instance is being used by multiple timer retrievals. The comparison could be done on the actual reference instead but I think the id based one is better long term as getTimer() could be expanded to provide a proxy that only allows the original caller a single dereference call so it wouldn't be the exact same physical reference to each caller (only same logical reference).

[sehrope]

Looks like Travis passed fine but AppVeyor. I'm thinking it's just AppVeyor running a tad slower so it didn't have enough time to complete and clean up the thread.

I've pushed another commit that changes the wait logic to repeatedly check if the thread is stopped in a loop for up to 10-seconds (previously it was 500ms).

[cheenamalhotra]

A few changes required for standardization (Reported by Static Code Analysis)

[sehrope]

🎉