Fix Daemon.stop() to actually stop the listener thread

ServerSocket.accept() is not interruptible: a thread busy in accept()
may not react to Thread.interrupt() and may not return from accept()
via an InterruptedException. Close the socket instead to make the
daemon's listener thread terminate.

* Close the listening socket to get the listening thread to exit
  instead of interrupting it.
* Add a stopAndWait() method that stops the listening thread and
  then waits until it has indeed finished.
* Set SO_REUSE_ADDRESS on the listening socket.

Bug: 376369
Change-Id: I9d6014103e6dcb0173daea134feb44dc52c5c69a
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/DaemonTest.java

@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2017 Thomas Wolf <thomas.wolf@paranor.ch>
+ * and other copyright owners as documented in the project's IP log.
+ *
+ * This program and the accompanying materials are made available
+ * under the terms of the Eclipse Distribution License v1.0 which
+ * accompanies this distribution, is reproduced below, and is
+ * available at http://www.eclipse.org/org/documents/edl-v10.php
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *   copyright notice, this list of conditions and the following
+ *   disclaimer in the documentation and/or other materials provided
+ *   with the distribution.
+ *
+ * - Neither the name of the Eclipse Foundation, Inc. nor the
+ *   names of its contributors may be used to endorse or promote
+ *   products derived from this software without specific prior
+ *   written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.eclipse.jgit.transport;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+import java.net.InetSocketAddress;
+
+import org.junit.Test;
+
+/**
+ * Daemon tests.
+ */
+public class DaemonTest {
+
+	@Test
+	public void testDaemonStop() throws Exception {
+		Daemon d = new Daemon();
+		d.start();
+		InetSocketAddress address = d.getAddress();
+		assertTrue("Port should be allocated", address.getPort() > 0);
+		assertTrue("Daemon should be running", d.isRunning());
+		Thread.sleep(1000); // Give it time to enter accept()
+		d.stopAndWait();
+		// Try to start a new Daemon again on the same port
+		d = new Daemon(address);
+		d.start();
+		InetSocketAddress newAddress = d.getAddress();
+		assertEquals("New daemon should run on the same port", address,
+				newAddress);
+		assertTrue("Daemon should be running", d.isRunning());
+		Thread.sleep(1000);
+		d.stopAndWait();
+	}
+
+	@Test
+	public void testDaemonRestart() throws Exception {
+		Daemon d = new Daemon();
+		d.start();
+		InetSocketAddress address = d.getAddress();
+		assertTrue("Port should be allocated", address.getPort() > 0);
+		assertTrue("Daemon should be running", d.isRunning());
+		Thread.sleep(1000);
+		d.stopAndWait();
+		// Re-start the same daemon
+		d.start();
+		InetSocketAddress newAddress = d.getAddress();
+		assertEquals("Daemon should again run on the same port", address,
+				newAddress);
+		assertTrue("Daemon should be running", d.isRunning());
+		Thread.sleep(1000);
+		d.stopAndWait();
+	}
+}

org.eclipse.jgit/src/org/eclipse/jgit/transport/Daemon.java

@@ -45,13 +45,14 @@ package org.eclipse.jgit.transport;
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.io.InterruptedIOException;
 import java.io.OutputStream;
 import java.net.InetAddress;
 import java.net.InetSocketAddress;
 import java.net.ServerSocket;
 import java.net.Socket;
 import java.net.SocketAddress;
+import java.net.SocketException;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.eclipse.jgit.errors.RepositoryNotFoundException;
 import org.eclipse.jgit.internal.JGitText;
@@ -77,9 +78,7 @@ public class Daemon {
 
 	private final ThreadGroup processors;
 
-	private boolean run;
+	private Acceptor acceptThread;
-
-	Thread acceptThread;
 
 	private int timeout;
 
@@ -281,61 +280,115 @@ public class Daemon {
 			receivePackFactory = (ReceivePackFactory<DaemonClient>) ReceivePackFactory.DISABLED;
 	}
 
-	/**
+	private class Acceptor extends Thread {
-	 * Start this daemon on a background thread.
-	 *
-	 * @throws IOException
-	 *             the server socket could not be opened.
-	 * @throws IllegalStateException
-	 *             the daemon is already running.
-	 */
-	public synchronized void start() throws IOException {
-		if (acceptThread != null)
-			throw new IllegalStateException(JGitText.get().daemonAlreadyRunning);
 
-		final ServerSocket listenSock = new ServerSocket(
+		private final ServerSocket listenSocket;
-				myAddress != null ? myAddress.getPort() : 0, BACKLOG,
+
-				myAddress != null ? myAddress.getAddress() : null);
+		private final AtomicBoolean running = new AtomicBoolean(true);
-		myAddress = (InetSocketAddress) listenSock.getLocalSocketAddress();
+
+		public Acceptor(ThreadGroup group, String name, ServerSocket socket) {
+			super(group, name);
+			this.listenSocket = socket;
+		}
 
-		run = true;
-		acceptThread = new Thread(processors, "Git-Daemon-Accept") { //$NON-NLS-1$
 		@Override
 		public void run() {
+			setUncaughtExceptionHandler((thread, throwable) -> terminate());
 			while (isRunning()) {
 				try {
-						startClient(listenSock.accept());
+					startClient(listenSocket.accept());
-					} catch (InterruptedIOException e) {
+				} catch (SocketException e) {
 					// Test again to see if we should keep accepting.
 				} catch (IOException e) {
 					break;
 				}
 			}
 
+			terminate();
+		}
+
+		private void terminate() {
 			try {
-					listenSock.close();
+				shutDown();
+			} finally {
+				clearThread();
+			}
+		}
+
+		public boolean isRunning() {
+			return running.get();
+		}
+
+		public void shutDown() {
+			running.set(false);
+			try {
+				listenSocket.close();
 			} catch (IOException err) {
 				//
-				} finally {
-					synchronized (Daemon.this) {
-						acceptThread = null;
 			}
 		}
+
 	}
-		};
+
+	/**
+	 * Start this daemon on a background thread.
+	 *
+	 * @throws IOException
+	 *             the server socket could not be opened.
+	 * @throws IllegalStateException
+	 *             the daemon is already running.
+	 */
+	public synchronized void start() throws IOException {
+		if (acceptThread != null) {
+			throw new IllegalStateException(JGitText.get().daemonAlreadyRunning);
+		}
+		ServerSocket socket = new ServerSocket();
+		socket.setReuseAddress(true);
+		if (myAddress != null) {
+			socket.bind(myAddress, BACKLOG);
+		} else {
+			socket.bind(new InetSocketAddress((InetAddress) null, 0), BACKLOG);
+		}
+		myAddress = (InetSocketAddress) socket.getLocalSocketAddress();
+
+		acceptThread = new Acceptor(processors, "Git-Daemon-Accept", socket); //$NON-NLS-1$
 		acceptThread.start();
 	}
 
+	private synchronized void clearThread() {
+		acceptThread = null;
+	}
+
 	/** @return true if this daemon is receiving connections. */
 	public synchronized boolean isRunning() {
-		return run;
+		return acceptThread != null && acceptThread.isRunning();
 	}
 
-	/** Stop this daemon. */
+	/**
+	 * Stop this daemon.
+	 */
 	public synchronized void stop() {
 		if (acceptThread != null) {
-			run = false;
+			acceptThread.shutDown();
-			acceptThread.interrupt();
+		}
+	}
+
+	/**
+	 * Stops this daemon and waits until it's acceptor thread has finished.
+	 *
+	 * @throws InterruptedException
+	 *             if waiting for the acceptor thread is interrupted
+	 *
+	 * @since 4.9
+	 */
+	public void stopAndWait() throws InterruptedException {
+		Thread acceptor = null;
+		synchronized (this) {
+			acceptor = acceptThread;
+			stop();
+		}
+		if (acceptor != null) {
+			acceptor.join();
 		}
 	}