From 8bbbebcf96bad129ca5d3afb78ee0e4cd9ed63fa Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Sat, 5 Jan 2019 10:51:51 -0500 Subject: [PATCH 01/14] Replace timeout handling to use SharedTimer Replaces the timeout handling for basic and bulk TDS commands to use a new SharedTimer class. SharedTimer provides a static method for fetching an existing static object or creating one on demand. Usage is tracked through reference counting and callers are required to call removeRef() when they will no longer be using the SharedTimer. If the SharedTimer does not have any more references then its internal ScheduledThreadPoolExecutor will be shutdown. The SharedTimer is cached at the Connection level so that repeated invocations do not create new timers. Connections only create timers on first use so if no actions involve a timeout then no timer is fetched or created. If a Connection does create a timer then it will be released when the Connection closed. Properly written JDBC applications that always close their Connection objects when they are finished using them should not have any extra threads running after they are all closed. Applications that do not use query timeouts will not have any extra threads created as they are only done on demand. Applications that use timeouts and use a JDBC connection pool will have a single shared object across all JDBC connections as long as there are some open connections in the pool with timeouts enabled. Interrupt actions to handle a timeout are executed in their own thread. A handler thread is created when the timeout occurs with the thread name matching the connection id of the client connection that created the timeout. If the timeout is canceled prior to the interrupt action being executed, say because the command finished, then no handler thread is created. Note that the sharing of the timers happens across all Connections, not just Connections with the same JDBC URL and properties. --- .../microsoft/sqlserver/jdbc/IOBuffer.java | 64 ++++----------- .../sqlserver/jdbc/SQLServerBulkCopy.java | 77 +++++------------ .../sqlserver/jdbc/SQLServerConnection.java | 17 ++++ .../microsoft/sqlserver/jdbc/SharedTimer.java | 65 +++++++++++++++ .../sqlserver/jdbc/TdsTimeoutTask.java | 62 ++++++++++++++ .../sqlserver/jdbc/TimeoutCommand.java | 41 ---------- .../sqlserver/jdbc/TimeoutPoller.java | 82 ------------------- 7 files changed, 182 insertions(+), 226 deletions(-) create mode 100644 src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java create mode 100644 src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java delete mode 100644 src/main/java/com/microsoft/sqlserver/jdbc/TimeoutCommand.java delete mode 100644 src/main/java/com/microsoft/sqlserver/jdbc/TimeoutPoller.java diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java b/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java index 027cc5eae0..c066f27e48 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java @@ -52,6 +52,7 @@ import java.util.Set; import java.util.SimpleTimeZone; import java.util.TimeZone; +import java.util.concurrent.ScheduledFuture; import java.util.concurrent.SynchronousQueue; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; @@ -3016,6 +3017,10 @@ void setDataLoggable(boolean value) { dataIsLoggable = value; } + SharedTimer getSharedTimer() { + return con.getSharedTimer(); + } + private TDSCommand command = null; // TDS message type (Query, RPC, DTC, etc.) sent at the beginning @@ -6236,7 +6241,7 @@ final class TDSReaderMark { final class TDSReader { private final static Logger logger = Logger.getLogger("com.microsoft.sqlserver.jdbc.internals.TDS.Reader"); final private String traceID; - private TimeoutCommand timeoutCommand; + private ScheduledFuture timeout; final public String toString() { return traceID; @@ -6390,9 +6395,8 @@ synchronized final boolean readPacket() throws SQLServerException { // terminate the connection. if ((command.getCancelQueryTimeoutSeconds() > 0 && command.getQueryTimeoutSeconds() > 0)) { // if a timeout is configured with this object, add it to the timeout poller - int timeout = command.getCancelQueryTimeoutSeconds() + command.getQueryTimeoutSeconds(); - this.timeoutCommand = new TdsTimeoutCommand(timeout, this.command, this.con); - TimeoutPoller.getTimeoutPoller().addTimeoutCommand(this.timeoutCommand); + int seconds = command.getCancelQueryTimeoutSeconds() + command.getQueryTimeoutSeconds(); + this.timeout = con.getSharedTimer().schedule(new TdsTimeoutTask(command, con), seconds); } } // First, read the packet header. @@ -6413,8 +6417,9 @@ synchronized final boolean readPacket() throws SQLServerException { } // if execution was subject to timeout then stop timing - if (this.timeoutCommand != null) { - TimeoutPoller.getTimeoutPoller().remove(this.timeoutCommand); + if (this.timeout != null) { + this.timeout.cancel(false); + this.timeout = null; } // Header size is a 2 byte unsigned short integer in big-endian order. int packetLength = Util.readUnsignedShortBigEndian(newPacket.header, TDS.PACKET_HEADER_MESSAGE_LENGTH); @@ -7003,42 +7008,6 @@ final void trySetSensitivityClassification(SensitivityClassification sensitivity } -/** - * The tds default implementation of a timeout command - */ -class TdsTimeoutCommand extends TimeoutCommand { - public TdsTimeoutCommand(int timeout, TDSCommand command, SQLServerConnection sqlServerConnection) { - super(timeout, command, sqlServerConnection); - } - - public void interrupt() { - TDSCommand command = getCommand(); - SQLServerConnection sqlServerConnection = getSqlServerConnection(); - try { - // If TCP Connection to server is silently dropped, exceeding the query timeout - // on the same connection does - // not throw SQLTimeoutException - // The application stops responding instead until SocketTimeoutException is - // thrown. In this case, we must - // manually terminate the connection. - if (null == command && null != sqlServerConnection) { - sqlServerConnection.terminate(SQLServerException.DRIVER_ERROR_IO_FAILED, - SQLServerException.getErrString("R_connectionIsClosed")); - } else { - // If the timer wasn't canceled before it ran out of - // time then interrupt the registered command. - command.interrupt(SQLServerException.getErrString("R_queryTimedOut")); - } - } catch (SQLServerException e) { - // Unfortunately, there's nothing we can do if we - // fail to time out the request. There is no way - // to report back what happened. - assert null != command; - command.log(Level.FINE, "Command could not be timed out. Reason: " + e.getMessage()); - } - } -} - /** * TDSCommand encapsulates an interruptable TDS conversation. * @@ -7160,7 +7129,7 @@ protected void setProcessedResponse(boolean processedResponse) { private volatile boolean readingResponse; private int queryTimeoutSeconds; private int cancelQueryTimeoutSeconds; - private TdsTimeoutCommand timeoutCommand; + private ScheduledFuture timeout; protected int getQueryTimeoutSeconds() { return this.queryTimeoutSeconds; @@ -7576,8 +7545,8 @@ final TDSReader startResponse(boolean isAdaptive) throws SQLServerException { // If command execution is subject to timeout then start timing until // the server returns the first response packet. if (queryTimeoutSeconds > 0) { - this.timeoutCommand = new TdsTimeoutCommand(queryTimeoutSeconds, this, null); - TimeoutPoller.getTimeoutPoller().addTimeoutCommand(this.timeoutCommand); + SQLServerConnection conn = tdsReader != null ? tdsReader.getConnection() : null; + this.timeout = tdsWriter.getSharedTimer().schedule(new TdsTimeoutTask(this, conn), queryTimeoutSeconds); } if (logger.isLoggable(Level.FINEST)) @@ -7600,8 +7569,9 @@ final TDSReader startResponse(boolean isAdaptive) throws SQLServerException { } finally { // If command execution was subject to timeout then stop timing as soon // as the server returns the first response packet or errors out. - if (this.timeoutCommand != null) { - TimeoutPoller.getTimeoutPoller().remove(this.timeoutCommand); + if (this.timeout != null) { + this.timeout.cancel(false); + this.timeout = null; } } diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java index 2df1e090a5..d3ca92709d 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java @@ -42,6 +42,7 @@ import java.util.SimpleTimeZone; import java.util.TimeZone; import java.util.UUID; +import java.util.concurrent.ScheduledFuture; import java.util.logging.Level; import javax.sql.RowSet; @@ -246,31 +247,7 @@ class BulkColumnMetaData { */ private int srcColumnCount; - /** - * Timeout for the bulk copy command - */ - private final class BulkTimeoutCommand extends TimeoutCommand { - public BulkTimeoutCommand(int timeout, TDSCommand command, SQLServerConnection sqlServerConnection) { - super(timeout, command, sqlServerConnection); - } - - @Override - public void interrupt() { - TDSCommand command = getCommand(); - // If the timer wasn't canceled before it ran out of - // time then interrupt the registered command. - try { - command.interrupt(SQLServerException.getErrString("R_queryTimedOut")); - } catch (SQLServerException e) { - // Unfortunately, there's nothing we can do if we - // fail to time out the request. There is no way - // to report back what happened. - command.log(Level.FINE, "Command could not be timed out. Reason: " + e.getMessage()); - } - } - } - - private BulkTimeoutCommand timeoutCommand; + private ScheduledFuture timeout; /** * The maximum temporal precision we can send when using varchar(precision) in bulkcommand, to send a @@ -646,16 +623,14 @@ private void sendBulkLoadBCP() throws SQLServerException { final class InsertBulk extends TDSCommand { InsertBulk() { super("InsertBulk", 0, 0); - int timeoutSeconds = copyOptions.getBulkCopyTimeout(); - timeoutCommand = timeoutSeconds > 0 ? new BulkTimeoutCommand(timeoutSeconds, this, null) : null; } final boolean doExecute() throws SQLServerException { - if (null != timeoutCommand) { - if (logger.isLoggable(Level.FINEST)) - logger.finest(this.toString() + ": Starting bulk timer..."); - - TimeoutPoller.getTimeoutPoller().addTimeoutCommand(timeoutCommand); + int timeoutSeconds = copyOptions.getBulkCopyTimeout(); + if (timeoutSeconds > 0) { + connection.checkClosed(); + timeout = connection.getSharedTimer().schedule(new TdsTimeoutTask(this, connection), + timeoutSeconds); } // doInsertBulk inserts the rows in one batch. It returns true if there are more rows in @@ -671,21 +646,27 @@ final boolean doExecute() throws SQLServerException { } // Check whether it is a timeout exception. - if (rootCause instanceof SQLException) { - checkForTimeoutException((SQLException) rootCause, timeoutCommand); + if (rootCause instanceof SQLException && timeout != null && timeout.isDone()) { + SQLException sqlEx = (SQLException) rootCause; + if (sqlEx.getSQLState() != null + && sqlEx.getSQLState().equals(SQLState.STATEMENT_CANCELED.getSQLStateCode())) { + // If SQLServerBulkCopy is managing the transaction, a rollback is needed. + if (copyOptions.isUseInternalTransaction()) { + connection.rollback(); + } + throw new SQLServerException(SQLServerException.getErrString("R_queryTimedOut"), + SQLState.STATEMENT_CANCELED, DriverError.NOT_SET, sqlEx); + } } // It is not a timeout exception. Re-throw. throw topLevelException; } - if (null != timeoutCommand) { - if (logger.isLoggable(Level.FINEST)) - logger.finest(this.toString() + ": Stopping bulk timer..."); - - TimeoutPoller.getTimeoutPoller().remove(timeoutCommand); + if (timeout != null) { + timeout.cancel(true); + timeout = null; } - return true; } } @@ -1145,22 +1126,6 @@ private void writeColumnMetaData(TDSWriter tdsWriter) throws SQLServerException } } - /** - * Helper method that throws a timeout exception if the cause of the exception was that the query was cancelled - */ - private void checkForTimeoutException(SQLException e, BulkTimeoutCommand timeoutCommand) throws SQLServerException { - if ((null != e.getSQLState()) && (e.getSQLState().equals(SQLState.STATEMENT_CANCELED.getSQLStateCode())) - && timeoutCommand.canTimeout()) { - // If SQLServerBulkCopy is managing the transaction, a rollback is needed. - if (copyOptions.isUseInternalTransaction()) { - connection.rollback(); - } - - throw new SQLServerException(SQLServerException.getErrString("R_queryTimedOut"), - SQLState.STATEMENT_CANCELED, DriverError.NOT_SET, e); - } - } - /** * Validates whether the source JDBC types are compatible with the destination table data types. We need to do this * only once for the whole bulk copy session. diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java index 0807ae17dc..9bf284b4d6 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java @@ -141,6 +141,18 @@ public class SQLServerConnection implements ISQLServerConnection, java.io.Serial private Boolean isAzureDW = null; + private SharedTimer sharedTimer; + + SharedTimer getSharedTimer() { + if (state == State.Closed) { + throw new IllegalStateException("Connection is closed"); + } + if (sharedTimer == null) { + this.sharedTimer = SharedTimer.getTimer(); + } + return this.sharedTimer; + } + static class CityHash128Key implements java.io.Serializable { /** @@ -3174,6 +3186,11 @@ public void close() throws SQLServerException { // with the connection. setState(State.Closed); + if (sharedTimer != null) { + sharedTimer.removeRef(); + sharedTimer = null; + } + // Close the TDS channel. When the channel is closed, the server automatically // rolls back any pending transactions and closes associated resources like // prepared handles. diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java new file mode 100644 index 0000000000..ee8fed11f0 --- /dev/null +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java @@ -0,0 +1,65 @@ +/* + * Microsoft JDBC Driver for SQL Server Copyright(c) Microsoft Corporation All rights reserved. This program is made + * available under the terms of the MIT License. See the LICENSE file in the project root for more information. + */ +package com.microsoft.sqlserver.jdbc; + +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.ScheduledThreadPoolExecutor; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; + + +class SharedTimer { + private static final String CORE_THREAD_PREFIX = "mssql-jdbc-shared-timer-core-"; + private static final AtomicLong CORE_THREAD_COUNTER = new AtomicLong(); + private static SharedTimer INSTANCE; + + private final long id = CORE_THREAD_COUNTER.getAndIncrement(); + private int refCount = 0; + private ScheduledThreadPoolExecutor executor; + + private SharedTimer() { + executor = new ScheduledThreadPoolExecutor(1, new ThreadFactory() { + @Override + public Thread newThread(Runnable task) { + return new Thread(task, CORE_THREAD_PREFIX + id); + } + }); + executor.setRemoveOnCancelPolicy(true); + } + + public synchronized void removeRef() { + if (refCount <= 0) { + throw new IllegalStateException("removeRef() called more than actual references"); + } + refCount -= 1; + if (refCount == 0) { + // Removed last reference so perform cleanup + executor.shutdownNow(); + executor = null; + INSTANCE = null; + } + } + + public static synchronized SharedTimer getTimer() { + if (INSTANCE == null) { + // No shared object exists so create a new one + INSTANCE = new SharedTimer(); + } + INSTANCE.refCount += 1; + return INSTANCE; + } + + public ScheduledFuture schedule(TdsTimeoutTask task, long delaySeconds) { + return schedule(task, delaySeconds, TimeUnit.SECONDS); + } + + public ScheduledFuture schedule(TdsTimeoutTask task, long delay, TimeUnit unit) { + if (executor == null) { + throw new IllegalStateException("Cannot schedule tasks after shutdown"); + } + return executor.schedule(task, delay, unit); + } +} diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java b/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java new file mode 100644 index 0000000000..6e8406e174 --- /dev/null +++ b/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java @@ -0,0 +1,62 @@ +/* + * Microsoft JDBC Driver for SQL Server Copyright(c) Microsoft Corporation All rights reserved. This program is made + * available under the terms of the MIT License. See the LICENSE file in the project root for more information. + */ +package com.microsoft.sqlserver.jdbc; + +import java.util.UUID; +import java.util.concurrent.atomic.AtomicLong; +import java.util.logging.Level; + + +/** + * The TDS default implementation of a timeout command + */ +class TdsTimeoutTask implements Runnable { + private static final AtomicLong COUNTER = new AtomicLong(0); + + private final UUID connectionId; + private final TDSCommand command; + private final SQLServerConnection sqlServerConnection; + + public TdsTimeoutTask(TDSCommand command, SQLServerConnection sqlServerConnection) { + this.connectionId = sqlServerConnection == null ? null : sqlServerConnection.getClientConIdInternal(); + this.command = command; + this.sqlServerConnection = sqlServerConnection; + } + + @Override + public final void run() { + // Create a new thread to run the interrupt to ensure that blocking operations performed + // by the interrupt do not hang the primary timer thread. + String name = "mssql-timeout-task-" + COUNTER.incrementAndGet() + "-" + connectionId; + Thread thread = new Thread(this::interrupt, name); + thread.setDaemon(true); + thread.start(); + } + + protected void interrupt() { + try { + // If TCP Connection to server is silently dropped, exceeding the query timeout + // on the same connection does not throw SQLTimeoutException + // The application stops responding instead until SocketTimeoutException is + // thrown. In this case, we must manually terminate the connection. + if (null == command) { + if (null != sqlServerConnection) { + sqlServerConnection.terminate(SQLServerException.DRIVER_ERROR_IO_FAILED, + SQLServerException.getErrString("R_connectionIsClosed")); + } + } else { + // If the timer wasn't canceled before it ran out of + // time then interrupt the registered command. + command.interrupt(SQLServerException.getErrString("R_queryTimedOut")); + } + } catch (SQLServerException e) { + // Unfortunately, there's nothing we can do if we fail to time out the request. There + // is no way to report back what happened. + assert null != command; + command.log(Level.WARNING, "Command could not be timed out. Reason: " + e.getMessage()); + System.err.println(e.getMessage()); + } + } +} diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/TimeoutCommand.java b/src/main/java/com/microsoft/sqlserver/jdbc/TimeoutCommand.java deleted file mode 100644 index 65b1f68b26..0000000000 --- a/src/main/java/com/microsoft/sqlserver/jdbc/TimeoutCommand.java +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Microsoft JDBC Driver for SQL Server Copyright(c) Microsoft Corporation All rights reserved. This program is made - * available under the terms of the MIT License. See the LICENSE file in the project root for more information. - */ - -package com.microsoft.sqlserver.jdbc; - -/** - * Abstract implementation of a command that can be timed out using the {@link TimeoutPoller} - */ -abstract class TimeoutCommand { - private final long startTime; - private final int timeout; - private final T command; - private final SQLServerConnection sqlServerConnection; - - TimeoutCommand(int timeout, T command, SQLServerConnection sqlServerConnection) { - this.timeout = timeout; - this.command = command; - this.sqlServerConnection = sqlServerConnection; - this.startTime = System.currentTimeMillis(); - } - - public boolean canTimeout() { - long currentTime = System.currentTimeMillis(); - return ((currentTime - startTime) / 1000) >= timeout; - } - - public T getCommand() { - return command; - } - - public SQLServerConnection getSqlServerConnection() { - return sqlServerConnection; - } - - /** - * The implementation for interrupting this timeout command - */ - public abstract void interrupt(); -} diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/TimeoutPoller.java b/src/main/java/com/microsoft/sqlserver/jdbc/TimeoutPoller.java deleted file mode 100644 index 6c53d4d744..0000000000 --- a/src/main/java/com/microsoft/sqlserver/jdbc/TimeoutPoller.java +++ /dev/null @@ -1,82 +0,0 @@ -/* - * Microsoft JDBC Driver for SQL Server Copyright(c) Microsoft Corporation All rights reserved. This program is made - * available under the terms of the MIT License. See the LICENSE file in the project root for more information. - */ - -package com.microsoft.sqlserver.jdbc; - -import java.util.ArrayList; -import java.util.Iterator; -import java.util.List; -import java.util.logging.Level; -import java.util.logging.Logger; - - -/** - * Thread that runs in the background while the mssql driver is used that can timeout TDSCommands Checks all registered - * commands every second to see if they can be interrupted - */ -final class TimeoutPoller implements Runnable { - private List> timeoutCommands = new ArrayList<>(); - final static Logger logger = Logger.getLogger("com.microsoft.sqlserver.jdbc.TimeoutPoller"); - private static volatile TimeoutPoller timeoutPoller = null; - - static TimeoutPoller getTimeoutPoller() { - if (timeoutPoller == null) { - synchronized (TimeoutPoller.class) { - if (timeoutPoller == null) { - // initialize the timeout poller thread once - timeoutPoller = new TimeoutPoller(); - // start the timeout polling thread - Thread pollerThread = new Thread(timeoutPoller, "mssql-jdbc-TimeoutPoller"); - pollerThread.setDaemon(true); - pollerThread.start(); - } - } - } - return timeoutPoller; - } - - void addTimeoutCommand(TimeoutCommand timeoutCommand) { - synchronized (timeoutCommands) { - timeoutCommands.add(timeoutCommand); - } - } - - void remove(TimeoutCommand timeoutCommand) { - synchronized (timeoutCommands) { - timeoutCommands.remove(timeoutCommand); - } - } - - private TimeoutPoller() {} - - public void run() { - try { - // Poll every second checking for commands that have timed out and need - // interruption - while (true) { - synchronized (timeoutCommands) { - Iterator> timeoutCommandIterator = timeoutCommands.iterator(); - while (timeoutCommandIterator.hasNext()) { - TimeoutCommand timeoutCommand = timeoutCommandIterator.next(); - try { - if (timeoutCommand.canTimeout()) { - try { - timeoutCommand.interrupt(); - } finally { - timeoutCommandIterator.remove(); - } - } - } catch (Exception e) { - logger.log(Level.WARNING, "Could not timeout command", e); - } - } - } - Thread.sleep(1000); - } - } catch (Exception e) { - logger.log(Level.SEVERE, "Error processing timeout commands", e); - } - } -} From 240aea8cc5555930a1b95050e7f02b46000d232d Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 08:53:08 -0500 Subject: [PATCH 02/14] Normalize newlines in TimeoutTest --- .../sqlserver/jdbc/timeouts/TimeoutTest.java | 126 +++++++++--------- 1 file changed, 63 insertions(+), 63 deletions(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/timeouts/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/timeouts/TimeoutTest.java index a61e097b17..f9f348bffa 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/timeouts/TimeoutTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/timeouts/TimeoutTest.java @@ -1,63 +1,63 @@ -/* - * Microsoft JDBC Driver for SQL Server Copyright(c) Microsoft Corporation All rights reserved. This program is made - * available under the terms of the MIT License. See the LICENSE file in the project root for more information. - */ - -package com.microsoft.sqlserver.jdbc.timeouts; - -import java.sql.Connection; -import java.sql.DriverManager; -import java.sql.PreparedStatement; -import java.sql.SQLException; -import java.sql.SQLTimeoutException; - -import org.junit.Assert; -import org.junit.jupiter.api.Test; -import org.junit.platform.runner.JUnitPlatform; -import org.junit.runner.RunWith; - -import com.microsoft.sqlserver.testframework.AbstractTest; - - -@RunWith(JUnitPlatform.class) -public class TimeoutTest extends AbstractTest { - @Test - public void testBasicQueryTimeout() { - boolean exceptionThrown = false; - try { - // wait 1 minute and timeout after 10 seconds - Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", 10)); - } catch (SQLException e) { - exceptionThrown = true; - Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); - } - Assert.assertTrue("A SQLTimeoutException was expected", exceptionThrown); - } - - @Test - public void testQueryTimeoutValid() { - boolean exceptionThrown = false; - int timeoutInSeconds = 10; - long start = System.currentTimeMillis(); - try { - // wait 1 minute and timeout after 10 seconds - Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", timeoutInSeconds)); - } catch (SQLException e) { - int secondsElapsed = (int) ((System.currentTimeMillis() - start) / 1000); - Assert.assertTrue("Query did not timeout expected, elapsedTime=" + secondsElapsed, - secondsElapsed >= timeoutInSeconds); - exceptionThrown = true; - Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); - } - Assert.assertTrue("A SQLTimeoutException was expected", exceptionThrown); - } - - private boolean runQuery(String query, int timeout) throws SQLException { - try (Connection con = DriverManager.getConnection(connectionString); - PreparedStatement preparedStatement = con.prepareStatement(query)) { - // set provided timeout - preparedStatement.setQueryTimeout(timeout); - return preparedStatement.execute(); - } - } -} +/* + * Microsoft JDBC Driver for SQL Server Copyright(c) Microsoft Corporation All rights reserved. This program is made + * available under the terms of the MIT License. See the LICENSE file in the project root for more information. + */ + +package com.microsoft.sqlserver.jdbc.timeouts; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.SQLException; +import java.sql.SQLTimeoutException; + +import org.junit.Assert; +import org.junit.jupiter.api.Test; +import org.junit.platform.runner.JUnitPlatform; +import org.junit.runner.RunWith; + +import com.microsoft.sqlserver.testframework.AbstractTest; + + +@RunWith(JUnitPlatform.class) +public class TimeoutTest extends AbstractTest { + @Test + public void testBasicQueryTimeout() { + boolean exceptionThrown = false; + try { + // wait 1 minute and timeout after 10 seconds + Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", 10)); + } catch (SQLException e) { + exceptionThrown = true; + Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); + } + Assert.assertTrue("A SQLTimeoutException was expected", exceptionThrown); + } + + @Test + public void testQueryTimeoutValid() { + boolean exceptionThrown = false; + int timeoutInSeconds = 10; + long start = System.currentTimeMillis(); + try { + // wait 1 minute and timeout after 10 seconds + Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", timeoutInSeconds)); + } catch (SQLException e) { + int secondsElapsed = (int) ((System.currentTimeMillis() - start) / 1000); + Assert.assertTrue("Query did not timeout expected, elapsedTime=" + secondsElapsed, + secondsElapsed >= timeoutInSeconds); + exceptionThrown = true; + Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); + } + Assert.assertTrue("A SQLTimeoutException was expected", exceptionThrown); + } + + private boolean runQuery(String query, int timeout) throws SQLException { + try (Connection con = DriverManager.getConnection(connectionString); + PreparedStatement preparedStatement = con.prepareStatement(query)) { + // set provided timeout + preparedStatement.setQueryTimeout(timeout); + return preparedStatement.execute(); + } + } +} From 4f49dd36fecd5eb42a8c49eb63330a206261c741 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 09:21:09 -0500 Subject: [PATCH 03/14] Move TimeoutTest to parent package so that it can access package private members --- .../com/microsoft/sqlserver/jdbc/{timeouts => }/TimeoutTest.java | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename src/test/java/com/microsoft/sqlserver/jdbc/{timeouts => }/TimeoutTest.java (100%) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/timeouts/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java similarity index 100% rename from src/test/java/com/microsoft/sqlserver/jdbc/timeouts/TimeoutTest.java rename to src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java From 7b5f81f7c5e425844b18f0e10514095dd898efbb Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 09:38:18 -0500 Subject: [PATCH 04/14] Centralize SharedTimer thread checks in TimeoutTest Centralize checks for SharedTimer thread state in TimeoutTest to happen before and after all test invocations in TimeoutTest via JUnit @Before and @After annotations. Each test now requires that the SharedTimer thread not be running before the test starts and after the test completes. Also expands the thread name prefix in SharedTimer to package private so that TimeoutTest can reference it rather than having its own copy fo the string. --- .../microsoft/sqlserver/jdbc/SharedTimer.java | 10 ++++- .../microsoft/sqlserver/jdbc/TimeoutTest.java | 45 ++++++++++++++++++- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java index ee8fed11f0..1d4fae446d 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java @@ -12,7 +12,7 @@ class SharedTimer { - private static final String CORE_THREAD_PREFIX = "mssql-jdbc-shared-timer-core-"; + static final String CORE_THREAD_PREFIX = "mssql-jdbc-shared-timer-core-"; private static final AtomicLong CORE_THREAD_COUNTER = new AtomicLong(); private static SharedTimer INSTANCE; @@ -30,6 +30,14 @@ public Thread newThread(Runnable task) { executor.setRemoveOnCancelPolicy(true); } + public long getId() { + return id; + } + + static synchronized boolean isRunning() { + return INSTANCE != null; + } + public synchronized void removeRef() { if (refCount <= 0) { throw new IllegalStateException("removeRef() called more than actual references"); diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java index f9f348bffa..67a481135d 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java @@ -3,15 +3,21 @@ * available under the terms of the MIT License. See the LICENSE file in the project root for more information. */ -package com.microsoft.sqlserver.jdbc.timeouts; +package com.microsoft.sqlserver.jdbc; + +import static org.junit.Assert.assertFalse; import java.sql.Connection; import java.sql.DriverManager; import java.sql.PreparedStatement; import java.sql.SQLException; import java.sql.SQLTimeoutException; +import java.util.Set; +import org.junit.After; import org.junit.Assert; +import org.junit.Before; +import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import org.junit.platform.runner.JUnitPlatform; import org.junit.runner.RunWith; @@ -21,6 +27,25 @@ @RunWith(JUnitPlatform.class) public class TimeoutTest extends AbstractTest { + @BeforeAll + public static void beforeAll() throws SQLException, InterruptedException { + if (connection != null) { + connection.close(); + connection = null; + } + waitForSharedTimerThreadToStop(); + } + + @Before + public void before() throws InterruptedException { + waitForSharedTimerThreadToStop(); + } + + @After + public void after() throws InterruptedException { + waitForSharedTimerThreadToStop(); + } + @Test public void testBasicQueryTimeout() { boolean exceptionThrown = false; @@ -60,4 +85,22 @@ private boolean runQuery(String query, int timeout) throws SQLException { return preparedStatement.execute(); } } + + private static boolean isSharedTimerThreadRunning() { + Set threadSet = Thread.getAllStackTraces().keySet(); + for (Thread thread : threadSet) { + if (thread.getName().startsWith(SharedTimer.CORE_THREAD_PREFIX)) { + return true; + } + } + return false; + } + + private static void waitForSharedTimerThreadToStop() throws InterruptedException { + if (isSharedTimerThreadRunning()) { + // Timer thread is still running so wait a bit for it to stop + Thread.sleep(500); + } + assertFalse("SharedTimer thread should not be running", isSharedTimerThreadRunning()); + } } From 16ba2c4ec1fccb397cb10af8a63b17aad1877909 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 10:03:55 -0500 Subject: [PATCH 05/14] Centralize and reduce timeout value used in TimeoutTest Centralizes constant timeout value used in TimeoutTest and reduces it from ten seconds down to two seconds to speed up tests. --- .../microsoft/sqlserver/jdbc/TimeoutTest.java | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java index 67a481135d..b8c02ddb94 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java @@ -5,6 +5,7 @@ package com.microsoft.sqlserver.jdbc; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import java.sql.Connection; @@ -27,6 +28,8 @@ @RunWith(JUnitPlatform.class) public class TimeoutTest extends AbstractTest { + private static final int TIMEOUT_SECONDS = 2; + @BeforeAll public static void beforeAll() throws SQLException, InterruptedException { if (connection != null) { @@ -50,8 +53,7 @@ public void after() throws InterruptedException { public void testBasicQueryTimeout() { boolean exceptionThrown = false; try { - // wait 1 minute and timeout after 10 seconds - Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", 10)); + Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", TIMEOUT_SECONDS)); } catch (SQLException e) { exceptionThrown = true; Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); @@ -62,15 +64,14 @@ public void testBasicQueryTimeout() { @Test public void testQueryTimeoutValid() { boolean exceptionThrown = false; - int timeoutInSeconds = 10; long start = System.currentTimeMillis(); try { - // wait 1 minute and timeout after 10 seconds - Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", timeoutInSeconds)); + // wait 1 minute but timeout well before that + Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", TIMEOUT_SECONDS)); } catch (SQLException e) { int secondsElapsed = (int) ((System.currentTimeMillis() - start) / 1000); Assert.assertTrue("Query did not timeout expected, elapsedTime=" + secondsElapsed, - secondsElapsed >= timeoutInSeconds); + secondsElapsed >= TIMEOUT_SECONDS); exceptionThrown = true; Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); } @@ -86,6 +87,21 @@ private boolean runQuery(String query, int timeout) throws SQLException { } } + @Test + public void testSameSharedTimerRetrieved() { + SharedTimer timer = SharedTimer.getTimer(); + try { + SharedTimer otherTimer = SharedTimer.getTimer(); + try { + assertEquals("The same SharedTimer should be returned", timer.getId(), otherTimer.getId()); + } finally { + otherTimer.removeRef(); + } + } finally { + timer.removeRef(); + } + } + private static boolean isSharedTimerThreadRunning() { Set threadSet = Thread.getAllStackTraces().keySet(); for (Thread thread : threadSet) { From 09bab9f46a2f6366a81c353f7a5ae21c34a8d6e5 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 10:06:32 -0500 Subject: [PATCH 06/14] Centralize SQL for WAIT FOR DELAY in TimeoutTest Centralizes the SQL command for WAIT FOR DELAY in TimeoutTest and changes the format of the delay to use hour:minute:second for clarity. --- src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java index b8c02ddb94..78b62499d4 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java @@ -29,6 +29,7 @@ @RunWith(JUnitPlatform.class) public class TimeoutTest extends AbstractTest { private static final int TIMEOUT_SECONDS = 2; + private static final String WAIT_FOR_ONE_MINUTE_SQL = "WAITFOR DELAY '00:01:00'"; @BeforeAll public static void beforeAll() throws SQLException, InterruptedException { @@ -53,7 +54,7 @@ public void after() throws InterruptedException { public void testBasicQueryTimeout() { boolean exceptionThrown = false; try { - Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", TIMEOUT_SECONDS)); + Assert.assertTrue("Select succeeded", runQuery(WAIT_FOR_ONE_MINUTE_SQL, TIMEOUT_SECONDS)); } catch (SQLException e) { exceptionThrown = true; Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); @@ -67,7 +68,7 @@ public void testQueryTimeoutValid() { long start = System.currentTimeMillis(); try { // wait 1 minute but timeout well before that - Assert.assertTrue("Select succeeded", runQuery("WAITFOR DELAY '00:01'", TIMEOUT_SECONDS)); + Assert.assertTrue("Select succeeded", runQuery(WAIT_FOR_ONE_MINUTE_SQL, TIMEOUT_SECONDS)); } catch (SQLException e) { int secondsElapsed = (int) ((System.currentTimeMillis() - start) / 1000); Assert.assertTrue("Query did not timeout expected, elapsedTime=" + secondsElapsed, From 1a7b6a73b90b5bab66c72868728f89eb029b9cd1 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 10:36:25 -0500 Subject: [PATCH 07/14] Clean up TimeoutTest to use assertThrows(...) Changes test in TimeoutTest to use assertThrows(...) rather than manually checking for the thrown exception. Also cleans up and adds internal helpers for creating a connection and changes internal runQuery(...) to be void rather than returning a boolean. --- .../microsoft/sqlserver/jdbc/TimeoutTest.java | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java index 78b62499d4..92be5a973c 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java @@ -7,10 +7,12 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import java.sql.Connection; import java.sql.DriverManager; import java.sql.PreparedStatement; +import java.sql.ResultSet; import java.sql.SQLException; import java.sql.SQLTimeoutException; import java.util.Set; @@ -52,39 +54,38 @@ public void after() throws InterruptedException { @Test public void testBasicQueryTimeout() { - boolean exceptionThrown = false; - try { - Assert.assertTrue("Select succeeded", runQuery(WAIT_FOR_ONE_MINUTE_SQL, TIMEOUT_SECONDS)); - } catch (SQLException e) { - exceptionThrown = true; - Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); - } - Assert.assertTrue("A SQLTimeoutException was expected", exceptionThrown); + assertThrows(SQLTimeoutException.class, () -> { + runQuery(WAIT_FOR_ONE_MINUTE_SQL, TIMEOUT_SECONDS); + }); } @Test public void testQueryTimeoutValid() { - boolean exceptionThrown = false; long start = System.currentTimeMillis(); - try { - // wait 1 minute but timeout well before that - Assert.assertTrue("Select succeeded", runQuery(WAIT_FOR_ONE_MINUTE_SQL, TIMEOUT_SECONDS)); - } catch (SQLException e) { - int secondsElapsed = (int) ((System.currentTimeMillis() - start) / 1000); - Assert.assertTrue("Query did not timeout expected, elapsedTime=" + secondsElapsed, - secondsElapsed >= TIMEOUT_SECONDS); - exceptionThrown = true; - Assert.assertTrue("Timeout exception not thrown", e.getClass().equals(SQLTimeoutException.class)); + assertThrows(SQLTimeoutException.class, () -> { + runQuery(WAIT_FOR_ONE_MINUTE_SQL, TIMEOUT_SECONDS); + }); + long elapsedSeconds = (System.currentTimeMillis() - start) / 1000; + Assert.assertTrue("Query duration must be at least timeout amount, elapsed=" + elapsedSeconds, + elapsedSeconds >= TIMEOUT_SECONDS); + } + + private static Connection getConnection() throws SQLException { + return DriverManager.getConnection(connectionString); + } + + private static void runQuery(String query, int timeout) throws SQLException { + try (Connection conn = getConnection()) { + runQuery(conn, query, timeout); } - Assert.assertTrue("A SQLTimeoutException was expected", exceptionThrown); } - private boolean runQuery(String query, int timeout) throws SQLException { - try (Connection con = DriverManager.getConnection(connectionString); - PreparedStatement preparedStatement = con.prepareStatement(query)) { - // set provided timeout - preparedStatement.setQueryTimeout(timeout); - return preparedStatement.execute(); + private static void runQuery(Connection conn, String query, int timeout) throws SQLException { + try (PreparedStatement stmt = conn.prepareStatement(query)) { + if (timeout > 0) { + stmt.setQueryTimeout(timeout); + } + try (ResultSet rs = stmt.executeQuery()) {} } } From e76e819f9537c003dd58e2e2a5cdc29954081bb3 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 11:07:27 -0500 Subject: [PATCH 08/14] Add additional SharedTimer tests Adds additional tests to verify creation and shutdown of SharedTimer core thread when user commands specify a query timeout. --- .../microsoft/sqlserver/jdbc/TimeoutTest.java | 65 ++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java index 92be5a973c..1e924d3602 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java @@ -7,6 +7,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.junit.jupiter.api.Assertions.assertThrows; import java.sql.Connection; @@ -70,6 +71,60 @@ public void testQueryTimeoutValid() { elapsedSeconds >= TIMEOUT_SECONDS); } + @Test + public void testZeroTimeoutShouldNotStartTimerThread() throws SQLException { + try (Connection conn = getConnection()) { + // Connection is open but we have not used a timeout so it should be running + assertSharedTimerNotRunning(); + runQuery(conn, "SELECT 1", 0); + // Our statement does not have a timeout so the timer should not be started yet + assertSharedTimerNotRunning(); + } + } + + @Test + public void testNoTimeoutShouldNotStartTimerThread() throws SQLException { + try (Connection conn = getConnection()) { + // Connection is open but we have not used a timeout so it should not be running + assertSharedTimerNotRunning(); + runQuery(conn, "SELECT 1", 0); + // Ran a query but our statement does not have a timeout so the timer should not be running + assertSharedTimerNotRunning(); + } + } + + @Test + public void testPositiveTimeoutShouldStartTimerThread() throws SQLException { + try (Connection conn = getConnection()) { + // Connection is open but we have not used a timeout so it should not be running + assertSharedTimerNotRunning(); + runQuery(conn, "SELECT 1", TIMEOUT_SECONDS); + // Ran a query with a timeout so the thread should continue running + assertSharedTimerIsRunning(); + } + } + + @Test + public void testNestedTimeoutShouldKeepTimerThreadRunning() throws SQLException { + try (Connection conn = getConnection()) { + // Connection is open but we have not used a timeout so it should not be running + assertSharedTimerNotRunning(); + runQuery(conn, "SELECT 1", TIMEOUT_SECONDS); + // Ran a query with a timeout so the thread should continue running + assertSharedTimerIsRunning(); + + // Open a new connection + try (Connection otherConn = getConnection()) { + assertSharedTimerIsRunning(); + runQuery(otherConn, "SELECT 1", TIMEOUT_SECONDS); + assertSharedTimerIsRunning(); + } + + // Timer should still be running because our original connection is still open + assertSharedTimerIsRunning(); + } + } + private static Connection getConnection() throws SQLException { return DriverManager.getConnection(connectionString); } @@ -119,6 +174,14 @@ private static void waitForSharedTimerThreadToStop() throws InterruptedException // Timer thread is still running so wait a bit for it to stop Thread.sleep(500); } - assertFalse("SharedTimer thread should not be running", isSharedTimerThreadRunning()); + assertSharedTimerNotRunning(); + } + + private static void assertSharedTimerNotRunning() { + assertFalse("SharedTimer should not be running", isSharedTimerThreadRunning()); + } + + private static void assertSharedTimerIsRunning() { + assertTrue("SharedTimer should be running", isSharedTimerThreadRunning()); } } From 594e5b2fce3afe24939467037f0a73524f6c38cc Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 11:37:56 -0500 Subject: [PATCH 09/14] Improve waiting for SharedTimer thread stoppage Change waiting for SharedTimer thread to stop to happen in a loop with periodic checks to see if the thread has stopped up to a max amount of waiting (10 seconds). --- .../com/microsoft/sqlserver/jdbc/TimeoutTest.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java index 1e924d3602..2bb6c7cab9 100644 --- a/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java +++ b/src/test/java/com/microsoft/sqlserver/jdbc/TimeoutTest.java @@ -8,6 +8,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.junit.jupiter.api.Assertions.assertThrows; import java.sql.Connection; @@ -170,9 +171,15 @@ private static boolean isSharedTimerThreadRunning() { } private static void waitForSharedTimerThreadToStop() throws InterruptedException { - if (isSharedTimerThreadRunning()) { - // Timer thread is still running so wait a bit for it to stop - Thread.sleep(500); + long started = System.currentTimeMillis(); + long MAX_WAIT_FOR_STOP_SECONDS = 10; + while (isSharedTimerThreadRunning()) { + long elapsed = System.currentTimeMillis() - started; + if (elapsed > MAX_WAIT_FOR_STOP_SECONDS * 1000) { + fail("SharedTimer thread did not stop within " + MAX_WAIT_FOR_STOP_SECONDS + " seconds"); + } + // Sleep a bit and try again + Thread.sleep(100); } assertSharedTimerNotRunning(); } From 693f51c314e09e7a6d430c39cec585cc3bf3e7cf Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 14:52:59 -0500 Subject: [PATCH 10/14] Remove System.err.println(...) in TdsTimeoutTask --- src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java b/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java index 6e8406e174..ee7ee1a674 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java @@ -56,7 +56,6 @@ protected void interrupt() { // is no way to report back what happened. assert null != command; command.log(Level.WARNING, "Command could not be timed out. Reason: " + e.getMessage()); - System.err.println(e.getMessage()); } } } From 261a857613ae6dabb8f86e4a9f461835d6503866 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 16:13:34 -0500 Subject: [PATCH 11/14] Rename static SharedTimer singleton to instance --- .../com/microsoft/sqlserver/jdbc/SharedTimer.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java index 1d4fae446d..e8050be0e2 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java @@ -14,7 +14,7 @@ class SharedTimer { static final String CORE_THREAD_PREFIX = "mssql-jdbc-shared-timer-core-"; private static final AtomicLong CORE_THREAD_COUNTER = new AtomicLong(); - private static SharedTimer INSTANCE; + private static SharedTimer instance; private final long id = CORE_THREAD_COUNTER.getAndIncrement(); private int refCount = 0; @@ -35,7 +35,7 @@ public long getId() { } static synchronized boolean isRunning() { - return INSTANCE != null; + return instance != null; } public synchronized void removeRef() { @@ -47,17 +47,17 @@ public synchronized void removeRef() { // Removed last reference so perform cleanup executor.shutdownNow(); executor = null; - INSTANCE = null; + instance = null; } } public static synchronized SharedTimer getTimer() { - if (INSTANCE == null) { + if (instance == null) { // No shared object exists so create a new one - INSTANCE = new SharedTimer(); + instance = new SharedTimer(); } - INSTANCE.refCount += 1; - return INSTANCE; + instance.refCount += 1; + return instance; } public ScheduledFuture schedule(TdsTimeoutTask task, long delaySeconds) { From 8c3b1da21dcaa2c5efe8a982fbbcdff0ef9b9cf4 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 16:14:09 -0500 Subject: [PATCH 12/14] Replace SharedTimer ThreadFactory with inline lambda --- .../java/com/microsoft/sqlserver/jdbc/SharedTimer.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java index e8050be0e2..698571876e 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java @@ -6,7 +6,6 @@ import java.util.concurrent.ScheduledFuture; import java.util.concurrent.ScheduledThreadPoolExecutor; -import java.util.concurrent.ThreadFactory; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; @@ -21,12 +20,7 @@ class SharedTimer { private ScheduledThreadPoolExecutor executor; private SharedTimer() { - executor = new ScheduledThreadPoolExecutor(1, new ThreadFactory() { - @Override - public Thread newThread(Runnable task) { - return new Thread(task, CORE_THREAD_PREFIX + id); - } - }); + executor = new ScheduledThreadPoolExecutor(1, task -> new Thread(task, CORE_THREAD_PREFIX + id)); executor.setRemoveOnCancelPolicy(true); } From 9f30783e1922dc65d580f3cb9118c858de6419e6 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 16:35:47 -0500 Subject: [PATCH 13/14] Add comments for SharedTimer --- .../sqlserver/jdbc/SQLServerConnection.java | 5 ++++ .../microsoft/sqlserver/jdbc/SharedTimer.java | 27 +++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java index 9bf284b4d6..3116c83343 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerConnection.java @@ -143,6 +143,11 @@ public class SQLServerConnection implements ISQLServerConnection, java.io.Serial private SharedTimer sharedTimer; + /** + * Return an existing cached SharedTimer associated with this Connection or create a new one. + * + * The SharedTimer will be released when the Connection is closed. + */ SharedTimer getSharedTimer() { if (state == State.Closed) { throw new IllegalStateException("Connection is closed"); diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java index 698571876e..1236c21e13 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java @@ -15,7 +15,13 @@ class SharedTimer { private static final AtomicLong CORE_THREAD_COUNTER = new AtomicLong(); private static SharedTimer instance; + /** + * Unique ID of this SharedTimer + */ private final long id = CORE_THREAD_COUNTER.getAndIncrement(); + /** + * Number of outstanding references to this SharedTimer + */ private int refCount = 0; private ScheduledThreadPoolExecutor executor; @@ -28,10 +34,18 @@ public long getId() { return id; } + /** + * @return Whether there is an instance of the SharedTimer currently allocated. + */ static synchronized boolean isRunning() { return instance != null; } + /** + * Remove a reference to this SharedTimer. + * + * If the reference count reaches zero then the underlying executor will be shutdown so that its thread stops. + */ public synchronized void removeRef() { if (refCount <= 0) { throw new IllegalStateException("removeRef() called more than actual references"); @@ -45,6 +59,13 @@ public synchronized void removeRef() { } } + /** + * Retrieve a reference to existing SharedTimer or create a new one. + * + * The SharedTimer's reference count will be incremented to account for the new reference. + * + * When the caller is finished with the SharedTimer it must be released via {@link#removeRef} + */ public static synchronized SharedTimer getTimer() { if (instance == null) { // No shared object exists so create a new one @@ -54,10 +75,16 @@ public static synchronized SharedTimer getTimer() { return instance; } + /** + * Schedule a task to execute in the future using this SharedTimer's internal executor. + */ public ScheduledFuture schedule(TdsTimeoutTask task, long delaySeconds) { return schedule(task, delaySeconds, TimeUnit.SECONDS); } + /** + * Schedule a task to execute in the future using this SharedTimer's internal executor. + */ public ScheduledFuture schedule(TdsTimeoutTask task, long delay, TimeUnit unit) { if (executor == null) { throw new IllegalStateException("Cannot schedule tasks after shutdown"); From e3e188b4de5e20acebbc0f5894ec7902a997dff7 Mon Sep 17 00:00:00 2001 From: Sehrope Sarkuni Date: Thu, 10 Jan 2019 17:34:35 -0500 Subject: [PATCH 14/14] Rename TdsTimeoutTask to TDSTimeoutTask --- src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java | 4 ++-- .../java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java | 2 +- src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java | 4 ++-- .../jdbc/{TdsTimeoutTask.java => TDSTimeoutTask.java} | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) rename src/main/java/com/microsoft/sqlserver/jdbc/{TdsTimeoutTask.java => TDSTimeoutTask.java} (95%) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java b/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java index c066f27e48..cdb5c1f570 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java @@ -6396,7 +6396,7 @@ synchronized final boolean readPacket() throws SQLServerException { if ((command.getCancelQueryTimeoutSeconds() > 0 && command.getQueryTimeoutSeconds() > 0)) { // if a timeout is configured with this object, add it to the timeout poller int seconds = command.getCancelQueryTimeoutSeconds() + command.getQueryTimeoutSeconds(); - this.timeout = con.getSharedTimer().schedule(new TdsTimeoutTask(command, con), seconds); + this.timeout = con.getSharedTimer().schedule(new TDSTimeoutTask(command, con), seconds); } } // First, read the packet header. @@ -7546,7 +7546,7 @@ final TDSReader startResponse(boolean isAdaptive) throws SQLServerException { // the server returns the first response packet. if (queryTimeoutSeconds > 0) { SQLServerConnection conn = tdsReader != null ? tdsReader.getConnection() : null; - this.timeout = tdsWriter.getSharedTimer().schedule(new TdsTimeoutTask(this, conn), queryTimeoutSeconds); + this.timeout = tdsWriter.getSharedTimer().schedule(new TDSTimeoutTask(this, conn), queryTimeoutSeconds); } if (logger.isLoggable(Level.FINEST)) diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java index d3ca92709d..3e6243029b 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java @@ -629,7 +629,7 @@ final boolean doExecute() throws SQLServerException { int timeoutSeconds = copyOptions.getBulkCopyTimeout(); if (timeoutSeconds > 0) { connection.checkClosed(); - timeout = connection.getSharedTimer().schedule(new TdsTimeoutTask(this, connection), + timeout = connection.getSharedTimer().schedule(new TDSTimeoutTask(this, connection), timeoutSeconds); } diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java index 1236c21e13..57cc70dfa3 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/SharedTimer.java @@ -78,14 +78,14 @@ public static synchronized SharedTimer getTimer() { /** * Schedule a task to execute in the future using this SharedTimer's internal executor. */ - public ScheduledFuture schedule(TdsTimeoutTask task, long delaySeconds) { + public ScheduledFuture schedule(TDSTimeoutTask task, long delaySeconds) { return schedule(task, delaySeconds, TimeUnit.SECONDS); } /** * Schedule a task to execute in the future using this SharedTimer's internal executor. */ - public ScheduledFuture schedule(TdsTimeoutTask task, long delay, TimeUnit unit) { + public ScheduledFuture schedule(TDSTimeoutTask task, long delay, TimeUnit unit) { if (executor == null) { throw new IllegalStateException("Cannot schedule tasks after shutdown"); } diff --git a/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java b/src/main/java/com/microsoft/sqlserver/jdbc/TDSTimeoutTask.java similarity index 95% rename from src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java rename to src/main/java/com/microsoft/sqlserver/jdbc/TDSTimeoutTask.java index ee7ee1a674..062f18a32c 100644 --- a/src/main/java/com/microsoft/sqlserver/jdbc/TdsTimeoutTask.java +++ b/src/main/java/com/microsoft/sqlserver/jdbc/TDSTimeoutTask.java @@ -12,14 +12,14 @@ /** * The TDS default implementation of a timeout command */ -class TdsTimeoutTask implements Runnable { +class TDSTimeoutTask implements Runnable { private static final AtomicLong COUNTER = new AtomicLong(0); private final UUID connectionId; private final TDSCommand command; private final SQLServerConnection sqlServerConnection; - public TdsTimeoutTask(TDSCommand command, SQLServerConnection sqlServerConnection) { + public TDSTimeoutTask(TDSCommand command, SQLServerConnection sqlServerConnection) { this.connectionId = sqlServerConnection == null ? null : sqlServerConnection.getClientConIdInternal(); this.command = command; this.sqlServerConnection = sqlServerConnection;