From 11c476346d1d7549f847c74d5e730935aa79e632 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Mon, 4 Sep 2017 11:02:37 +0200 Subject: [PATCH] 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 --- .../eclipse/jgit/transport/DaemonTest.java | 96 +++++++++++++ .../org/eclipse/jgit/transport/Daemon.java | 131 ++++++++++++------ 2 files changed, 188 insertions(+), 39 deletions(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/DaemonTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/DaemonTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/DaemonTest.java new file mode 100644 index 000000000..a5e544140 --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/DaemonTest.java @@ -0,0 +1,96 @@ +/* + * Copyright (C) 2017 Thomas Wolf + * 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(); + } +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Daemon.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/Daemon.java index 40b2c47df..896b10a4e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/Daemon.java +++ b/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; - - Thread acceptThread; + private Acceptor acceptThread; private int timeout; @@ -281,6 +280,56 @@ public class Daemon { receivePackFactory = (ReceivePackFactory) ReceivePackFactory.DISABLED; } + private class Acceptor extends Thread { + + private final ServerSocket listenSocket; + + private final AtomicBoolean running = new AtomicBoolean(true); + + public Acceptor(ThreadGroup group, String name, ServerSocket socket) { + super(group, name); + this.listenSocket = socket; + } + + @Override + public void run() { + setUncaughtExceptionHandler((thread, throwable) -> terminate()); + while (isRunning()) { + try { + startClient(listenSocket.accept()); + } catch (SocketException e) { + // Test again to see if we should keep accepting. + } catch (IOException e) { + break; + } + } + + terminate(); + } + + private void terminate() { + try { + shutDown(); + } finally { + clearThread(); + } + } + + public boolean isRunning() { + return running.get(); + } + + public void shutDown() { + running.set(false); + try { + listenSocket.close(); + } catch (IOException err) { + // + } + } + + } + /** * Start this daemon on a background thread. * @@ -290,52 +339,56 @@ public class Daemon { * the daemon is already running. */ public synchronized void start() throws IOException { - if (acceptThread != null) + 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(); - final ServerSocket listenSock = new ServerSocket( - myAddress != null ? myAddress.getPort() : 0, BACKLOG, - myAddress != null ? myAddress.getAddress() : null); - myAddress = (InetSocketAddress) listenSock.getLocalSocketAddress(); - - run = true; - acceptThread = new Thread(processors, "Git-Daemon-Accept") { //$NON-NLS-1$ - @Override - public void run() { - while (isRunning()) { - try { - startClient(listenSock.accept()); - } catch (InterruptedIOException e) { - // Test again to see if we should keep accepting. - } catch (IOException e) { - break; - } - } - - try { - listenSock.close(); - } catch (IOException err) { - // - } finally { - synchronized (Daemon.this) { - acceptThread = null; - } - } - } - }; + 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.interrupt(); + acceptThread.shutDown(); + } + } + + /** + * 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(); } }