Only compute hostname once per server lifetime

hlopko · philwo · commit 8bdd409f4900 · 2017-09-14T18:48:51.000+02:00
NetUtil.getShortHostName can take seconds on mac and on windows (like, 20!), since it performs reverse dns lookup. We already cached hostname for the BazelWorkspaceStatusModule, let's cache it for entire bazel server. Also make sure that users of the method understand it's cached. Fixes bazelbuild#3586. RELNOTES: None. PiperOrigin-RevId: 168691615
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
@@ -282,7 +282,6 @@ public Artifact getStableStatus() {
   }
 
   private class BazelStatusActionFactory implements WorkspaceStatusAction.Factory {
-    private String hostname;
 
     @Override
     public Map<String, String> createDummyWorkspaceStatus() {
@@ -312,11 +311,7 @@ public WorkspaceStatusAction createWorkspaceStatusAction(
      * changes during bazel server lifetime, bazel will not see the change.
      */
     private String getHostname() {
-      if (hostname == null) {
-        hostname = NetUtil.findShortHostName();
-      }
-
-      return hostname;
+      return NetUtil.getCachedShortHostName();
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -90,7 +90,7 @@ public LocalSpawnRunner(
     this.execRoot = execRoot;
     this.processWrapper = getProcessWrapper(execRoot, localOs).getPathString();
     this.localExecutionOptions = Preconditions.checkNotNull(localExecutionOptions);
-    this.hostName = NetUtil.findShortHostName();
+    this.hostName = NetUtil.getCachedShortHostName();
     this.resourceManager = resourceManager;
     this.useProcessWrapper = useProcessWrapper;
     this.productName = productName;
diff --git a/src/main/java/com/google/devtools/build/lib/util/NetUtil.java b/src/main/java/com/google/devtools/build/lib/util/NetUtil.java
@@ -21,14 +21,31 @@
  */
 public final class NetUtil {
 
+  private static String hostname;
+
   private NetUtil() {
   }
 
+  /**
+   * Returns the *cached* short hostname (computed at most once per the lifetime of a server). Can
+   * take seconds to complete when the cache is cold.
+   */
+  public static String getCachedShortHostName() {
+    if (hostname == null) {
+      synchronized (NetUtil.class) {
+        if (hostname == null) {
+          hostname = computeShortHostName();
+        }
+      }
+    }
+    return computeShortHostName();
+  }
+
   /**
    * Returns the short hostname or <code>unknown</code> if the host name could not be determined.
-   * Performs reverse DNS lookup and can take seconds to complete!
+   * Performs reverse DNS lookup and can take seconds to complete.
    */
-  public static String findShortHostName() {
+  private static String computeShortHostName() {
     try {
       return InetAddress.getLocalHost().getHostName();
     } catch (UnknownHostException e) {
diff --git a/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunnerTest.java
@@ -258,7 +258,7 @@ public void vanillaZeroExit() throws Exception {
     assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS);
     assertThat(result.exitCode()).isEqualTo(0);
     assertThat(result.setupSuccess()).isTrue();
-    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.findShortHostName());
+    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.getCachedShortHostName());
 
     assertThat(captor.getValue().getArgv())
         .containsExactlyElementsIn(
@@ -304,7 +304,7 @@ public void noProcessWrapper() throws Exception {
     assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS);
     assertThat(result.exitCode()).isEqualTo(0);
     assertThat(result.setupSuccess()).isTrue();
-    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.findShortHostName());
+    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.getCachedShortHostName());
 
     assertThat(captor.getValue().getArgv())
         .containsExactlyElementsIn(ImmutableList.of("/bin/echo", "Hi!"));
@@ -339,7 +339,7 @@ public void nonZeroExit() throws Exception {
     assertThat(result.status()).isEqualTo(SpawnResult.Status.SUCCESS);
     assertThat(result.exitCode()).isEqualTo(3);
     assertThat(result.setupSuccess()).isTrue();
-    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.findShortHostName());
+    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.getCachedShortHostName());
 
     assertThat(captor.getValue().getArgv())
         .containsExactlyElementsIn(
@@ -377,7 +377,7 @@ public void processStartupThrows() throws Exception {
     assertThat(result.exitCode()).isEqualTo(-1);
     assertThat(result.setupSuccess()).isFalse();
     assertThat(result.getWallTimeMillis()).isEqualTo(0);
-    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.findShortHostName());
+    assertThat(result.getExecutorHostName()).isEqualTo(NetUtil.getCachedShortHostName());
 
     assertThat(FileSystemUtils.readContent(fs.getPath("/out/stderr"), StandardCharsets.UTF_8))
         .isEqualTo("Action failed to execute: java.io.IOException: I'm sorry, Dave\n");
@@ -399,7 +399,7 @@ public void disallowLocalExecution() throws Exception {
     assertThat(reply.exitCode()).isEqualTo(-1);
     assertThat(reply.setupSuccess()).isFalse();
     assertThat(reply.getWallTimeMillis()).isEqualTo(0);
-    assertThat(reply.getExecutorHostName()).isEqualTo(NetUtil.findShortHostName());
+    assertThat(reply.getExecutorHostName()).isEqualTo(NetUtil.getCachedShortHostName());
 
     // TODO(ulfjack): Maybe we should only lock after checking?
     assertThat(policy.lockOutputFilesCalled).isTrue();

Original file line number	Diff line number	Diff line change
`@@ -282,7 +282,6 @@ public Artifact getStableStatus() {`
`282`	`282`	`}`
`283`	`283`
`284`	`284`	`private class BazelStatusActionFactory implements WorkspaceStatusAction.Factory {`
`285`		`- private String hostname;`
`286`	`285`
`287`	`286`	`@Override`
`288`	`287`	`public Map<String, String> createDummyWorkspaceStatus() {`
`@@ -312,11 +311,7 @@ public WorkspaceStatusAction createWorkspaceStatusAction(`
`312`	`311`	`* changes during bazel server lifetime, bazel will not see the change.`
`313`	`312`	`*/`
`314`	`313`	`private String getHostname() {`
`315`		`- if (hostname == null) {`
`316`		`- hostname = NetUtil.findShortHostName();`
`317`		`- }`
`318`		`-`
`319`		`- return hostname;`
	`314`	`+ return NetUtil.getCachedShortHostName();`
`320`	`315`	`}`
`321`	`316`	`}`
`322`	`317`