opendev/gerrit
Code Issues Proposed changes

Acceptance-tests: Fix 'Address already in use' bug

Browse Source 
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
This commit is contained in:
David Ostrovsky 2014-04-22 00:36:12 +02:00
parent aa2772a0ba
commit c7095c8450
4 changed files with 47 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
Documentation/dev-buck.txt
View File

@ -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

23
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/GerritServer.java
View File

@ -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();
}

22
gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java
View File

@ -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);

20
gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshDaemon.java
View File

@ -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<HostKey> 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<String> 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) {