Fix random test failures in TcpClientOutputTest and TcpConnectionTest

testInvalidCommand in TcpClientOutputTest used to fail randomly as two
threads working on each side of the socket connection while the socket
gets closed due to the invalid command. This is now fixed by
* don't read data at the remote socket at all, this is not required by
the test case,
* wait until all data has been consumed by the TcpClientOutput before
the connection is shut down.

testRemoteClose in TcpConnectionTest and in TcpClientOutputTest
used to fail randomly as two threads working on each side of the
socket connection while the socket gets closed. This is now fixed by
waiting until the header is fully read before the socket is closed.

To avoid lock situations in MockServerConnection the lock must be
notified when the internal buffer is decreased.

3 files changed, 60 insertions, 36 deletions

diff --git a/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/MockSocketConnection.java b/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/MockSocketConnection.java
index 8e20e24e..d82ef8f6 100644
--- a/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/MockSocketConnection.java
+++ b/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/MockSocketConnection.java
@@ -41,15 +41,15 @@ public class MockSocketConnection {
 		socketA.connect(socketB);
 	}
 
-	public Socket getSocketA() {
+	public MockSocket getSocketA() {
 		return socketA;
 	}
 
-	public Socket getSocketB() {
+	public MockSocket getSocketB() {
 		return socketB;
 	}
 
-	private class MockSocket extends Socket {
+	class MockSocket extends Socket {
 
 		private MockSocket other;
 
@@ -85,6 +85,7 @@ public class MockSocketConnection {
 								return -1;
 							}
 							final Byte b = buffer.poll();
+							buffer.notifyAll();
 							if (b != null) {
 								return 0xff & b.intValue();
 							}
@@ -98,26 +99,36 @@ public class MockSocketConnection {
 
 			@Override
 			public int available() throws IOException {
-				return buffer.size();
+				synchronized (buffer) {
+					return buffer.size();
+				}
 			}
 
 		};
 
-		public MockSocket() throws SocketException {
+		private MockSocket() throws SocketException {
 			super((SocketImpl) null);
 			closed = false;
 		}
 
-		void connect(MockSocket other) {
+		private void connect(MockSocket other) {
 			this.other = other;
 			other.other = this;
 		}
 
+		public void waitUntilInputBufferIsEmpty() throws InterruptedException {
+			synchronized (buffer) {
+				while (!closed && !buffer.isEmpty()) {
+					buffer.wait();
+				}
+			}
+		}
+
 		// socket methods with mocking behavior:
 
 		@Override
 		public OutputStream getOutputStream() throws IOException {
-			if (closed) {
+			if (isClosed()) {
 				throw new SocketException("Socket is closed");
 			}
 			return out;
@@ -125,7 +136,7 @@ public class MockSocketConnection {
 
 		@Override
 		public InputStream getInputStream() throws IOException {
-			if (closed) {
+			if (isClosed()) {
 				throw new SocketException("Socket is closed");
 			}
 			return in;
@@ -133,8 +144,8 @@ public class MockSocketConnection {
 
 		@Override
 		public void close() throws IOException {
-			closed = true;
 			synchronized (buffer) {
+				closed = true;
 				buffer.notifyAll();
 			}
 			synchronized (other.buffer) {
@@ -144,7 +155,9 @@ public class MockSocketConnection {
 
 		@Override
 		public boolean isClosed() {
-			return closed;
+			synchronized (buffer) {
+				return closed;
+			}
 		}
 
 		// unsupported socket methods:
diff --git a/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/TcpClientOutputTest.java b/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/TcpClientOutputTest.java
index 9ac8fa82..e7f18661 100644
--- a/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/TcpClientOutputTest.java
+++ b/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/TcpClientOutputTest.java
@@ -21,6 +21,7 @@ import java.net.Socket;
 import java.util.List;
 
 import org.jacoco.agent.rt.internal.ExceptionRecorder;
+import org.jacoco.agent.rt.internal.output.MockSocketConnection.MockSocket;
 import org.jacoco.core.data.ExecutionDataStore;
 import org.jacoco.core.data.SessionInfo;
 import org.jacoco.core.data.SessionInfoStore;
@@ -40,7 +41,8 @@ public class TcpClientOutputTest {
 
 	private IAgentOutput controller;
 
-	private Socket remoteSocket;
+	private MockSocket localSocket;
+	private MockSocket remoteSocket;
 
 	private RemoteControlWriter remoteWriter;
 
@@ -52,13 +54,14 @@ public class TcpClientOutputTest {
 	public void setup() throws Exception {
 		logger = new ExceptionRecorder();
 		final MockSocketConnection con = new MockSocketConnection();
+		localSocket = con.getSocketA();
 		remoteSocket = con.getSocketB();
 		remoteWriter = new RemoteControlWriter(remoteSocket.getOutputStream());
 		controller = new TcpClientOutput(logger) {
 			@Override
 			protected Socket createSocket(AgentOptions options)
 					throws IOException {
-				return con.getSocketA();
+				return localSocket;
 			}
 		};
 		data = new RuntimeData();
@@ -75,6 +78,8 @@ public class TcpClientOutputTest {
 
 	@Test
 	public void testRemoteClose() throws Exception {
+		localSocket.waitUntilInputBufferIsEmpty();
+
 		remoteSocket.close();
 		controller.shutdown();
 		logger.assertNoException();
@@ -82,16 +87,18 @@ public class TcpClientOutputTest {
 
 	@Test
 	public void testInvalidCommand() throws Exception {
+		// send invalid command to agent
 		remoteWriter.visitSessionInfo(new SessionInfo("info", 1, 2));
-		while (remoteReader.read()) {
-		}
+
+		localSocket.waitUntilInputBufferIsEmpty();
 		controller.shutdown();
 		logger.assertException(IOException.class, "No session info visitor.");
 	}
 
 	@Test
 	public void testWriteExecutionData() throws Exception {
-		data.getExecutionData(Long.valueOf(0x12345678), "Foo", 42).getProbes()[0] = true;
+		data.getExecutionData(Long.valueOf(0x12345678), "Foo", 42)
+				.getProbes()[0] = true;
 		data.setSessionId("stubid");
 
 		controller.writeExecutionData(false);
diff --git a/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/TcpConnectionTest.java b/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/TcpConnectionTest.java
index d713203d..3108a992 100644
--- a/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/TcpConnectionTest.java
+++ b/org.jacoco.agent.rt.test/src/org/jacoco/agent/rt/internal/output/TcpConnectionTest.java
@@ -66,8 +66,8 @@ public class TcpConnectionTest extends ExecutorTestBase {
 		final OutputStream remoteOut = mockConnection.getSocketB()
 				.getOutputStream();
 		new ExecutionDataWriter(remoteOut);
-		final TcpConnection con = new TcpConnection(
-				mockConnection.getSocketA(), data);
+		final TcpConnection con = new TcpConnection(mockConnection.getSocketA(),
+				data);
 		con.init();
 		remoteOut.write(123);
 		con.run();
@@ -82,8 +82,8 @@ public class TcpConnectionTest extends ExecutorTestBase {
 				.getOutputStream();
 		new ExecutionDataWriter(remoteOut);
 
-		final TcpConnection con = new TcpConnection(
-				mockConnection.getSocketA(), data);
+		final TcpConnection con = new TcpConnection(mockConnection.getSocketA(),
+				data);
 		con.init();
 
 		final Future<Void> f = executor.submit(new Callable<Void>() {
@@ -95,6 +95,7 @@ public class TcpConnectionTest extends ExecutorTestBase {
 
 		assertBlocks(f);
 
+		mockConnection.getSocketA().waitUntilInputBufferIsEmpty();
 		mockConnection.getSocketB().close();
 		f.get();
 	}
@@ -103,8 +104,8 @@ public class TcpConnectionTest extends ExecutorTestBase {
 	 * Remote endpoint is closed before even a valid header was send.
 	 */
 	public void testRemoteCloseWithoutHeader() throws Throwable {
-		final TcpConnection con = new TcpConnection(
-				mockConnection.getSocketA(), data);
+		final TcpConnection con = new TcpConnection(mockConnection.getSocketA(),
+				data);
 
 		final Future<Void> f = executor.submit(new Callable<Void>() {
 			public Void call() throws Exception {
@@ -130,8 +131,8 @@ public class TcpConnectionTest extends ExecutorTestBase {
 				.getOutputStream();
 		new ExecutionDataWriter(remoteOut);
 
-		final TcpConnection con = new TcpConnection(
-				mockConnection.getSocketA(), data);
+		final TcpConnection con = new TcpConnection(mockConnection.getSocketA(),
+				data);
 		con.init();
 
 		final Future<Void> f = executor.submit(new Callable<Void>() {
@@ -149,14 +150,15 @@ public class TcpConnectionTest extends ExecutorTestBase {
 
 	@Test
 	public void testRemoteDump() throws Exception {
-		data.getExecutionData(Long.valueOf(0x12345678), "Foo", 42).getProbes()[0] = true;
+		data.getExecutionData(Long.valueOf(0x12345678), "Foo", 42)
+				.getProbes()[0] = true;
 		data.setSessionId("stubid");
 
 		final RemoteControlWriter remoteWriter = new RemoteControlWriter(
 				mockConnection.getSocketB().getOutputStream());
 
-		final TcpConnection con = new TcpConnection(
-				mockConnection.getSocketA(), data);
+		final TcpConnection con = new TcpConnection(mockConnection.getSocketA(),
+				data);
 		con.init();
 
 		final Future<Void> f = executor.submit(new Callable<Void>() {
@@ -177,13 +179,14 @@ public class TcpConnectionTest extends ExecutorTestBase {
 
 	@Test
 	public void testLocalDump() throws Exception {
-		data.getExecutionData(Long.valueOf(0x12345678), "Foo", 42).getProbes()[0] = true;
+		data.getExecutionData(Long.valueOf(0x12345678), "Foo", 42)
+				.getProbes()[0] = true;
 		data.setSessionId("stubid");
 
 		new RemoteControlWriter(mockConnection.getSocketB().getOutputStream());
 
-		final TcpConnection con = new TcpConnection(
-				mockConnection.getSocketA(), data);
+		final TcpConnection con = new TcpConnection(mockConnection.getSocketA(),
+				data);
 		con.init();
 
 		final Future<Void> f = executor.submit(new Callable<Void>() {
@@ -204,13 +207,13 @@ public class TcpConnectionTest extends ExecutorTestBase {
 
 	@Test
 	public void testLocalDumpWithoutInit() throws Exception {
-		final TcpConnection con = new TcpConnection(
-				mockConnection.getSocketA(), data);
+		final TcpConnection con = new TcpConnection(mockConnection.getSocketA(),
+				data);
 		// Must not write any data as we're not initialized:
 		con.writeExecutionData(false);
 
-		assertEquals(0, mockConnection.getSocketB().getInputStream()
-				.available());
+		assertEquals(0,
+				mockConnection.getSocketB().getInputStream().available());
 	}
 
 	private void readAndAssertData() throws IOException {
@@ -233,13 +236,14 @@ public class TcpConnectionTest extends ExecutorTestBase {
 
 	@Test
 	public void testRemoteReset() throws Exception {
-		data.getExecutionData(Long.valueOf(123), "Foo", 1).getProbes()[0] = true;
+		data.getExecutionData(Long.valueOf(123), "Foo", 1)
+				.getProbes()[0] = true;
 
 		final RemoteControlWriter remoteWriter = new RemoteControlWriter(
 				mockConnection.getSocketB().getOutputStream());
 
-		final TcpConnection con = new TcpConnection(
-				mockConnection.getSocketA(), data);
+		final TcpConnection con = new TcpConnection(mockConnection.getSocketA(),
+				data);
 		con.init();
 
 		final Future<Void> f = executor.submit(new Callable<Void>() {