From c7095c8450965600a8ceed9f1c88589c5fc84c0f Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 22 Apr 2014 00:36:12 +0200 Subject: [PATCH] Acceptance-tests: Fix 'Address already in use' bug This change fixes ephemeral port assignment for HTTP and SSH daemons. Current implementation is broken: it assigns a random port in GerritServer, closes the socket and effectively freeing it for the kernel to recycle to another thread, and store that port in gerrit.config file for reuse. Some time later in HTTP and SSH daemons the port number get read from the config file and the ports are bound. The problem is, that during this "think time" another test got the same port assigned and bind attempt is failing with: Caused by: java.net.BindException: Address already in use To rectify it, set port to 0 in gerrit.config for both HTTP and SSH daemons and let the daemons to do the ephemeral port assignments. In SSH and HTTP daemons this special case is recognized and the assigned ports are written back to gerrit.config to pass to the test harness side. Change-Id: Ib4971b200d6dc86268674398fe590137ae35b6f3 --- Documentation/dev-buck.txt | 27 ++++--------------- .../gerrit/acceptance/GerritServer.java | 23 +++------------- .../gerrit/pgm/http/jetty/JettyServer.java | 22 +++++++++++++-- .../com/google/gerrit/sshd/SshDaemon.java | 20 ++++++++++++-- 4 files changed, 47 insertions(+), 45 deletions(-) diff --git a/Documentation/dev-buck.txt b/Documentation/dev-buck.txt index c2c2a2dbc8..c6faac50d5 100644 --- a/Documentation/dev-buck.txt +++ b/Documentation/dev-buck.txt @@ -469,34 +469,17 @@ heap size: EOF ---- -== Sporadic failures of unit tests +== Rerun unit tests -Due to implementation of ephemeral port assignment for HTTP and SSH daemons, -(the reason is explained in depth in this thread: -https://groups.google.com/forum/#!topic/repo-discuss/db14RJ59ubs), -unit tests can randomly fail with "BindException: Address already in use". -In this case, failed tests need to be repeated: - ----- -FAIL 32,0s 7 Passed 1 Failed com.google.gerrit.acceptance.rest.group.AddRemoveGroupMembersIT -FAILURE includeRemoveGroup: Cannot bind to localhost:40955 - at com.google.gerrit.sshd.SshDaemon.start(SshDaemon.java:281) - at com.google.gerrit.lifecycle.LifecycleManager.start(LifecycleManager.java:74) - at com.google.gerrit.pgm.Daemon.start(Daemon.java:288) - at com.google.gerrit.acceptance.GerritServer.start(GerritServer.java:81) -[...] -Caused by: java.net.BindException: Address already in use - at sun.nio.ch.Net.bind0(Native Method) ----- - -Because the test execution results are cached by Buck, they must be removed -before retrying: +If for some reasons tests, that were already run must be repeated, unit test +cache must be removed fist. That's because the test execution results are +cached by Buck: ---- $ rm -rf buck-out/bin/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/group/.AddRemoveGroupMembersIT/ ---- -After clearing the cache and repeating of the failed tests, they are successful: +After clearing the cache test can be rerun again: ---- $ buck test //gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/group:AddRemoveGroupMembersIT diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java index a8ce229b18..cbf2685726 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java @@ -36,7 +36,6 @@ import java.io.IOException; import java.lang.reflect.Field; import java.net.InetAddress; import java.net.InetSocketAddress; -import java.net.ServerSocket; import java.net.URI; import java.net.UnknownHostException; import java.util.concurrent.BrokenBarrierException; @@ -123,23 +122,18 @@ public class GerritServer { private static void mergeTestConfig(Config cfg) throws IOException { - InetSocketAddress http = newPort(); - InetSocketAddress sshd = newPort(); - String url = "http://" + format(http) + "/"; - + String forceEphemeralPort = String.format("%s:0", + getLocalHost().getHostName()); + String url = "http://" + forceEphemeralPort + "/"; cfg.setString("gerrit", null, "canonicalWebUrl", url); cfg.setString("httpd", null, "listenUrl", url); - cfg.setString("sshd", null, "listenAddress", format(sshd)); + cfg.setString("sshd", null, "listenAddress", forceEphemeralPort); cfg.setString("cache", null, "directory", null); cfg.setBoolean("sendemail", null, "enable", false); cfg.setInt("cache", "projects", "checkFrequency", 0); cfg.setInt("plugins", null, "checkFrequency", 0); } - private static String format(InetSocketAddress s) { - return String.format("%s:%d", s.getAddress().getHostAddress(), s.getPort()); - } - private static Injector createTestInjector(Daemon daemon) throws Exception { Injector sysInjector = get(daemon, "sysInjector"); Module module = new FactoryModule() { @@ -160,15 +154,6 @@ public class GerritServer { return (T) f.get(obj); } - private static final InetSocketAddress newPort() throws IOException { - ServerSocket s = new ServerSocket(0, 0, getLocalHost()); - try { - return (InetSocketAddress) s.getLocalSocketAddress(); - } finally { - s.close(); - } - } - private static InetAddress getLocalHost() throws UnknownHostException { return InetAddress.getLoopbackAddress(); } diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java index 6ba54c9727..067eeab35e 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java @@ -19,6 +19,7 @@ import static java.util.concurrent.TimeUnit.SECONDS; import com.google.common.base.Charsets; import com.google.common.base.Objects; +import com.google.common.base.Strings; import com.google.common.escape.Escaper; import com.google.common.html.HtmlEscapers; import com.google.common.io.ByteStreams; @@ -103,16 +104,33 @@ public class JettyServer { static class Lifecycle implements LifecycleListener { private final JettyServer server; + private final Config cfg; @Inject - Lifecycle(final JettyServer server) { + Lifecycle(final JettyServer server, @GerritServerConfig final Config cfg) { this.server = server; + this.cfg = cfg; } @Override public void start() { try { + String origUrl = cfg.getString("httpd", null, "listenUrl"); + boolean rewrite = !Strings.isNullOrEmpty(origUrl) + && origUrl.endsWith(":0/"); server.httpd.start(); + if (rewrite) { + Connector con = server.httpd.getConnectors()[0]; + if (con instanceof ServerConnector) { + @SuppressWarnings("resource") + ServerConnector serverCon = (ServerConnector)con; + String host = serverCon.getHost(); + int port = serverCon.getLocalPort(); + String url = String.format("http://%s:%d", host, port); + cfg.setString("gerrit", null, "canonicalWebUrl", url); + cfg.setString("httpd", null, "listenUrl", url); + } + } } catch (Exception e) { throw new IllegalStateException("Cannot start HTTP daemon", e); } @@ -259,7 +277,7 @@ public class JettyServer { } else { final URI r = u.parseServerAuthority(); c.setHost(r.getHost()); - c.setPort(0 < r.getPort() ? r.getPort() : defaultPort); + c.setPort(0 <= r.getPort() ? r.getPort() : defaultPort); } } catch (URISyntaxException e) { throw new IllegalArgumentException("Invalid httpd.listenurl " + u, e); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java index 5338c166b4..f5079207cd 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java @@ -18,6 +18,8 @@ import static com.google.gerrit.server.ssh.SshAddressesModule.IANA_SSH_PORT; import static java.util.concurrent.TimeUnit.MILLISECONDS; import static java.util.concurrent.TimeUnit.SECONDS; +import com.google.common.base.Strings; +import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.gerrit.common.Version; import com.google.gerrit.extensions.events.LifecycleListener; @@ -51,11 +53,11 @@ import org.apache.sshd.common.cipher.AES128CTR; import org.apache.sshd.common.cipher.AES192CBC; import org.apache.sshd.common.cipher.AES256CBC; import org.apache.sshd.common.cipher.AES256CTR; +import org.apache.sshd.common.cipher.ARCFOUR128; +import org.apache.sshd.common.cipher.ARCFOUR256; import org.apache.sshd.common.cipher.BlowfishCBC; import org.apache.sshd.common.cipher.CipherNone; import org.apache.sshd.common.cipher.TripleDESCBC; -import org.apache.sshd.common.cipher.ARCFOUR128; -import org.apache.sshd.common.cipher.ARCFOUR256; import org.apache.sshd.common.compression.CompressionNone; import org.apache.sshd.common.file.FileSystemFactory; import org.apache.sshd.common.file.FileSystemView; @@ -100,6 +102,7 @@ import org.slf4j.LoggerFactory; import java.io.File; import java.io.IOException; import java.net.InetAddress; +import java.net.InetSocketAddress; import java.net.SocketAddress; import java.net.UnknownHostException; import java.security.InvalidKeyException; @@ -144,6 +147,7 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { private final boolean keepAlive; private final List hostKeys; private volatile IoAcceptor acceptor; + private final Config cfg; @Inject SshDaemon(final CommandFactory commandFactory, final NoShell noShell, @@ -155,6 +159,7 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { @SshAdvertisedAddresses final List advertised) { setPort(IANA_SSH_PORT /* never used */); + this.cfg = cfg; this.listen = listen; this.advertised = advertised; keepAlive = cfg.getBoolean("sshd", "tcpkeepalive", true); @@ -276,7 +281,14 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { acceptor = createAcceptor(); try { + String listenAddress = cfg.getString("sshd", null, "listenAddress"); + boolean rewrite = !Strings.isNullOrEmpty(listenAddress) + && listenAddress.endsWith(":0"); acceptor.bind(listen); + if (rewrite) { + SocketAddress bound = Iterables.getOnlyElement(acceptor.getBoundAddresses()); + cfg.setString("sshd", null, "listenAddress", format((InetSocketAddress)bound)); + } } catch (IOException e) { throw new IllegalStateException("Cannot bind to " + addressList(), e); } @@ -286,6 +298,10 @@ public class SshDaemon extends SshServer implements SshInfo, LifecycleListener { } } + private static String format(InetSocketAddress s) { + return String.format("%s:%d", s.getAddress().getHostAddress(), s.getPort()); + } + @Override public synchronized void stop() { if (acceptor != null) {