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.

Sign up for GitHub

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

Jump to bottom

Executor service instead of thread #162

Merged
merged 9 commits into from
Mar 10, 2017
Merged

Executor service instead of thread #162

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d5f10d5
replace Thread with ExecutorService
xiangyushawn Feb 27, 2017
72d596b
if ExecutorService is shutdown already, reset it
xiangyushawn Feb 27, 2017
2979464
use futures and set threads as Daemon
xiangyushawn Feb 28, 2017
585c0b5
thread can be interrupted
xiangyushawn Mar 1, 2017
4e46838
group TimeoutTimer threads
xiangyushawn Mar 1, 2017
ff2d811
added Connection Pool test cases
xiangyushawn Mar 4, 2017
ba54360
close datasource, test setQueryTimeout(), count number of threads
xiangyushawn Mar 7, 2017
1780179
add java docs for tests
xiangyushawn Mar 10, 2017
a9d41cc
add class level java doc
xiangyushawn Mar 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,15 +114,27 @@
<artifactId>junit-jupiter-engine</artifactId>
<version>5.0.0-M3</version>
<scope>test</scope>
</dependency>
</dependency>
<dependency>
<groupId>com.zaxxer</groupId>
<artifactId>HikariCP</artifactId>
<version>2.6.0</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-dbcp2 </artifactId>
<version>2.1.1</version>
<scope>test</scope>
</dependency>
<!--
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<version>4.12.0-M3</version>
<scope>test</scope>
</dependency>
-->
-->
</dependencies>

<profiles>
Expand Down
28 changes: 22 additions & 6 deletions src/main/java/com/microsoft/sqlserver/jdbc/IOBuffer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@
import java.util.Set;
import java.util.SimpleTimeZone;
import java.util.TimeZone;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.SynchronousQueue;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
Expand Down Expand Up @@ -6822,9 +6826,23 @@ final void TryProcessFeatureExtAck(boolean featureExtAckReceived) throws SQLServ
* a reason like "timed out".
*/
final class TimeoutTimer implements Runnable {
private static final String threadGroupName = "mssql-jdbc-TimeoutTimer";
private final int timeoutSeconds;
private final TDSCommand command;
private Thread timerThread;
private volatile Future<?> task;

private static final ExecutorService executor = Executors.newCachedThreadPool(new ThreadFactory() {
private final ThreadGroup tg = new ThreadGroup(threadGroupName);
private final String threadNamePrefix = tg.getName() + "-";
private final AtomicInteger threadNumber = new AtomicInteger(0);
@Override
public Thread newThread(Runnable r) {
Thread t = new Thread(tg, r, threadNamePrefix + threadNumber.incrementAndGet());
t.setDaemon(true);
return t;
}
});

private volatile boolean canceled = false;

TimeoutTimer(int timeoutSeconds,
Expand All @@ -6837,17 +6855,15 @@ final class TimeoutTimer implements Runnable {
}

final void start() {
timerThread = new Thread(this);
timerThread.setDaemon(true);
timerThread.start();
task = executor.submit(this);
}

final void stop() {
task.cancel(true);
canceled = true;
timerThread.interrupt();
}

public void run() {
public void run() {
int secondsRemaining = timeoutSeconds;
try {
// Poll every second while time is left on the timer.
Expand Down
54 changes: 54 additions & 0 deletions src/test/java/com/microsoft/sqlserver/jdbc/connection/PoolingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,16 @@

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.UUID;

import javax.sql.DataSource;
import javax.sql.PooledConnection;

import org.apache.commons.dbcp2.BasicDataSource;
import org.junit.jupiter.api.Test;
import org.junit.platform.runner.JUnitPlatform;
import org.junit.runner.RunWith;
Expand All @@ -30,6 +34,8 @@
import com.microsoft.sqlserver.testframework.DBConnection;
import com.microsoft.sqlserver.testframework.DBTable;
import com.microsoft.sqlserver.testframework.util.RandomUtil;
import com.zaxxer.hikari.HikariConfig;
import com.zaxxer.hikari.HikariDataSource;

@RunWith(JUnitPlatform.class)
public class PoolingTest extends AbstractTest {
Expand Down Expand Up @@ -139,4 +145,52 @@ public void testConnectionPoolClientConnectionId() throws SQLException {

assertEquals(Id1, Id2, "ClientConnection Ids from pool are not the same.");
}

@Test
public void testHikariCP() throws SQLException {
HikariConfig config = new HikariConfig();
config.setJdbcUrl(connectionString);
HikariDataSource ds = new HikariDataSource(config);

connect(ds);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing performance with 'trivial queries', so add a loop:

		try {
			int ttStartCount = countTimeoutThreads();
			for (int i = 0; i < 100; i++) {
				connect(ds);
			}
			int ttEndCount = countTimeoutThreads();
			if (ttEndCount - ttStartCount > 2) {
				throw new AssertionError("Too many timeout threads started.");
			}
		} finally {
			// always close the datasource
			ds.close();
		}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fwi : Thanks for suggestions. We are trying out Java MicroBenchmark Harness (JMH) test cases for performance testing. We are figuring out how & when we can open performance test cases. At the same time we want our Travis-CI & Appveyour builds should not be long running.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad choice of words on my part: this is actually a functional test. Remember when we changed a little bit of code to prevent creating lots of threads when many queries are executed rapidly? How do you know there is not another mistake like that still in the code? This test simply measures the amount of threads before and after executing many queries rapidly. Measuring is knowing.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note: the datasource backed by the pool implementation does need to be closed when the test is finished. If not, connections remain open and the pool implementation itself starts threads as well that are shutdown when the datasource is closed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final note: verify ttEndCount is at least 1 to ensure the test is working (if ttEndCount is zero, the test did not do its intended job).

Copy link
Contributor Author

@xiangyushawn xiangyushawn Mar 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of doing if (ttEndCount - ttStartCount > 2) { throw exception}, I guess only verify ttEndCount is at least 1 is sufficient. In the for loop, if we increase the termination (e.g. to 100000), it may create more than 2 timeout threads.

}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example for thread-counts (untested):

	public static int countTimeoutThreads() {

		ThreadInfo[] tinfos = ManagementFactory.getThreadMXBean().getThreadInfo(ManagementFactory.getThreadMXBean().getAllThreadIds(), 0);
		// IOBuffer.TimeoutTimer.threadGroupName should be publicly accessible ...
		final String toname = "mssql-jdbc-TimeoutTimer";
		int count = 0;
		for (ThreadInfo ti : tinfos) {
			if (ti.getThreadName().startsWith(toname) && !ti.getThreadState().equals(java.lang.Thread.State.TERMINATED)) {
				count++;
			}
		}
		return count;
	}

@Test
public void testApacheDBCP() throws SQLException {
BasicDataSource ds = new BasicDataSource();
ds.setUrl(connectionString);

connect(ds);
}

private static void connect(DataSource ds) throws SQLException {
Connection con = null;
PreparedStatement pst = null;
ResultSet rs = null;

try {
con = ds.getConnection();
con.isValid(5);

pst = con.prepareStatement("SELECT SUSER_SNAME()");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For testing timeout: pst.setQueryTimeout(5);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hello @fwi , could you please inform me what the benefits are by testing setQueryTimeout() instead of isValid()? from my understanding, both create a new thread for the timeout timer

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently "isValid" ends up using the timeout timer, but this is an implementation choice (other JDBC drivers do it differently for speed and this JDBC driver might one day too) and might change in the future. Also "isValid" is already called by the connection pool before returing a connection from the pool (see for example "validationTimeout" in HikariCP).

Besides that, the original issue was about "queries with a time-out", so why not test that explicitly and make sure to use a query with a time-out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that makes sense, I will change it.

rs = pst.executeQuery();

while (rs.next()) {
rs.getString(1);
}
}
finally {
if (rs != null) {
rs.close();
}

if (pst != null) {
pst.close();
}

if (con != null) {
con.close();
}
}
}
}