workers: Improve verbose messages when workers are restarted. Prevent killing of broken workers in the background, when no build is currently running, because we can only alert the user about what's going on while a build is currently running.

If you're a developer, this will probably help you when debugging a new worker.

Some messages you might see with --worker_verbose specified:

INFO: Worker configuration has changed, restarting worker pool...

WARNING: Javac worker (id 6) can no longer be used, because its files have changed on disk [13ce6c10546243b6b4ea2334dd3f8a55705e4c078cedd0e746d7ad7cee082e9a -> 9731223c50bab5c1f20d60d1a4967cf2726840727b6a639b6f6425547a9b4a10].

WARNING: Javac worker (id 4) can no longer be used, because its process terminated itself or got killed.

--
MOS_MIGRATED_REVID=123093071

3 files changed, 172 insertions, 45 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
index 3c444a2577..8f997536e9 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java
@@ -79,6 +79,36 @@ final class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker
   @Override
   public boolean validateObject(WorkerKey key, PooledObject<Worker> p) {
     Worker worker = p.getObject();
-    return key.getWorkerFilesHash().equals(worker.getWorkerFilesHash()) && worker.isAlive();
+
+    boolean hashMatches = key.getWorkerFilesHash().equals(worker.getWorkerFilesHash());
+    boolean workerIsAlive = worker.isAlive();
+    boolean workerIsStillValid = hashMatches && workerIsAlive;
+
+    if (reporter != null && !workerIsStillValid) {
+      StringBuilder msg = new StringBuilder();
+      msg.append(key.getMnemonic());
+      msg.append(" worker (id ");
+      msg.append(p.getObject().getWorkerId());
+      msg.append(") can no longer be used, because");
+
+      if (!workerIsAlive) {
+        msg.append(" its process terminated itself or got killed");
+      }
+
+      if (!hashMatches) {
+        if (!workerIsAlive) {
+          msg.append(" and");
+        }
+        msg.append(" its files have changed on disk [");
+        msg.append(worker.getWorkerFilesHash());
+        msg.append(" -> ");
+        msg.append(key.getWorkerFilesHash());
+        msg.append("]");
+      }
+
+      reporter.handle(Event.warn(msg.toString()));
+    }
+
+    return workerIsStillValid;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
index ef429461a3..63bade2086 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerModule.java
@@ -29,21 +29,20 @@ import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.common.options.OptionsBase;
 
-import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
-
 import java.io.IOException;
 
 /**
  * A module that adds the WorkerActionContextProvider to the available action context providers.
  */
 public class WorkerModule extends BlazeModule {
-  private WorkerFactory workerFactory;
-  private WorkerPool workerPool;
-
   private CommandEnvironment env;
   private BuildRequest buildRequest;
   private boolean verbose;
 
+  private WorkerFactory workerFactory;
+  private WorkerPool workerPool;
+  private WorkerPoolConfig workerPoolConfig;
+
   @Override
   public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command) {
     return command.builds()
@@ -55,6 +54,13 @@ public class WorkerModule extends BlazeModule {
   public void beforeCommand(Command command, CommandEnvironment env) {
     this.env = env;
     env.getEventBus().register(this);
+  }
+
+  @Subscribe
+  public void buildStarting(BuildStartingEvent event) {
+    buildRequest = event.getRequest();
+    WorkerOptions options = buildRequest.getOptions(WorkerOptions.class);
+    verbose = options.workerVerbose;
 
     if (workerFactory == null) {
       Path logDir = env.getOutputBase().getRelative("worker-logs");
@@ -79,44 +85,50 @@ public class WorkerModule extends BlazeModule {
     }
 
     workerFactory.setReporter(env.getReporter());
+    workerFactory.setVerbose(options.workerVerbose);
+
+    WorkerPoolConfig newConfig = createWorkerPoolConfig(options);
+
+    // If the config changed compared to the last run, we have to create a new pool.
+    if (workerPoolConfig != null && !workerPoolConfig.equals(newConfig)) {
+      shutdownPool("Worker configuration has changed, restarting worker pool...");
+    }
 
     if (workerPool == null) {
-      GenericKeyedObjectPoolConfig config = new GenericKeyedObjectPoolConfig();
+      workerPoolConfig = newConfig;
+      workerPool = new WorkerPool(workerFactory, workerPoolConfig);
+    }
+  }
 
-      // It's better to re-use a worker as often as possible and keep it hot, in order to profit
-      // from JIT optimizations as much as possible.
-      config.setLifo(true);
+  private WorkerPoolConfig createWorkerPoolConfig(WorkerOptions options) {
+    WorkerPoolConfig config = new WorkerPoolConfig();
 
-      // Check for & deal with idle workers every 5 seconds.
-      config.setTimeBetweenEvictionRunsMillis(5 * 1000);
+    // It's better to re-use a worker as often as possible and keep it hot, in order to profit
+    // from JIT optimizations as much as possible.
+    config.setLifo(true);
 
-      // Always test the liveliness of worker processes.
-      config.setTestOnBorrow(true);
-      config.setTestOnCreate(true);
-      config.setTestOnReturn(true);
-      config.setTestWhileIdle(true);
+    // Keep a fixed number of workers running per key.
+    config.setMaxIdlePerKey(options.workerMaxInstances);
+    config.setMaxTotalPerKey(options.workerMaxInstances);
+    config.setMinIdlePerKey(options.workerMaxInstances);
 
-      // Don't limit the total number of worker processes, as otherwise the pool might be full of
-      // e.g. Java workers and could never accommodate another request for a different kind of
-      // worker.
-      config.setMaxTotal(-1);
+    // Don't limit the total number of worker processes, as otherwise the pool might be full of
+    // e.g. Java workers and could never accommodate another request for a different kind of
+    // worker.
+    config.setMaxTotal(-1);
 
-      workerPool = new WorkerPool(workerFactory, config);
-    }
-  }
+    // Wait for a worker to become ready when a thread needs one.
+    config.setBlockWhenExhausted(true);
 
-  @Subscribe
-  public void buildStarting(BuildStartingEvent event) {
-    Preconditions.checkNotNull(workerPool);
+    // Always test the liveliness of worker processes.
+    config.setTestOnBorrow(true);
+    config.setTestOnCreate(true);
+    config.setTestOnReturn(true);
 
-    this.buildRequest = event.getRequest();
+    // No eviction of idle workers.
+    config.setTimeBetweenEvictionRunsMillis(-1);
 
-    WorkerOptions options = buildRequest.getOptions(WorkerOptions.class);
-    workerPool.setMaxTotalPerKey(options.workerMaxInstances);
-    workerPool.setMaxIdlePerKey(options.workerMaxInstances);
-    workerPool.setMinIdlePerKey(options.workerMaxInstances);
-    workerFactory.setVerbose(options.workerVerbose);
-    this.verbose = options.workerVerbose;
+    return config;
   }
 
   @Override
@@ -136,16 +148,10 @@ public class WorkerModule extends BlazeModule {
 
   @Subscribe
   public void buildComplete(BuildCompleteEvent event) {
-    if (workerPool != null && buildRequest != null
+    if (buildRequest != null
         && buildRequest.getOptions(WorkerOptions.class) != null
         && buildRequest.getOptions(WorkerOptions.class).workerQuitAfterBuild) {
-      if (verbose) {
-        env
-            .getReporter()
-            .handle(Event.info("Build completed, shutting down worker pool..."));
-      }
-      workerPool.close();
-      workerPool = null;
+      shutdownPool("Build completed, shutting down worker pool...");
     }
   }
 
@@ -154,11 +160,18 @@ public class WorkerModule extends BlazeModule {
   // for them to finish.
   @Subscribe
   public void buildInterrupted(BuildInterruptedEvent event) {
+    shutdownPool("Build interrupted, shutting down worker pool...");
+  }
+
+  /**
+   * Shuts down the worker pool and sets {#code workerPool} to null.
+   */
+  private void shutdownPool(String reason) {
+    Preconditions.checkArgument(!reason.isEmpty());
+
     if (workerPool != null) {
       if (verbose) {
-        env
-            .getReporter()
-            .handle(Event.info("Build interrupted, shutting down worker pool..."));
+        env.getReporter().handle(Event.info(reason));
       }
       workerPool.close();
       workerPool = null;
@@ -170,5 +183,9 @@ public class WorkerModule extends BlazeModule {
     this.env = null;
     this.buildRequest = null;
     this.verbose = false;
+
+    if (this.workerFactory != null) {
+      this.workerFactory.setReporter(null);
+    }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolConfig.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolConfig.java
new file mode 100644
index 0000000000..b56e7c912b
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerPoolConfig.java
@@ -0,0 +1,80 @@
+// Copyright 2016 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.worker;
+
+import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
+
+import java.util.Objects;
+
+/**
+ * Our own configuration class for the {@code WorkerPool} that correctly implements {@code equals()}
+ * and {@code hashCode()}.
+ */
+final class WorkerPoolConfig extends GenericKeyedObjectPoolConfig {
+  @Override
+  public boolean equals(Object o) {
+    if (this == o) {
+      return true;
+    }
+    if (o == null || getClass() != o.getClass()) {
+      return false;
+    }
+    WorkerPoolConfig that = (WorkerPoolConfig) o;
+    return getBlockWhenExhausted() == that.getBlockWhenExhausted()
+        && getFairness() == that.getFairness()
+        && getJmxEnabled() == that.getJmxEnabled()
+        && getLifo() == that.getLifo()
+        && getMaxWaitMillis() == that.getMaxWaitMillis()
+        && getMinEvictableIdleTimeMillis() == that.getMinEvictableIdleTimeMillis()
+        && getNumTestsPerEvictionRun() == that.getNumTestsPerEvictionRun()
+        && getSoftMinEvictableIdleTimeMillis() == that.getSoftMinEvictableIdleTimeMillis()
+        && getTestOnBorrow() == that.getTestOnBorrow()
+        && getTestOnCreate() == that.getTestOnCreate()
+        && getTestOnReturn() == that.getTestOnReturn()
+        && getTestWhileIdle() == that.getTestWhileIdle()
+        && getTimeBetweenEvictionRunsMillis() == that.getTimeBetweenEvictionRunsMillis()
+        && getMaxIdlePerKey() == that.getMaxIdlePerKey()
+        && getMaxTotal() == that.getMaxTotal()
+        && getMaxTotalPerKey() == that.getMaxTotalPerKey()
+        && getMinIdlePerKey() == that.getMinIdlePerKey()
+        && Objects.equals(getEvictionPolicyClassName(), that.getEvictionPolicyClassName())
+        && Objects.equals(getJmxNameBase(), that.getJmxNameBase())
+        && Objects.equals(getJmxNamePrefix(), that.getJmxNamePrefix());
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(
+        getBlockWhenExhausted(),
+        getFairness(),
+        getJmxEnabled(),
+        getLifo(),
+        getMaxWaitMillis(),
+        getMinEvictableIdleTimeMillis(),
+        getNumTestsPerEvictionRun(),
+        getSoftMinEvictableIdleTimeMillis(),
+        getTestOnBorrow(),
+        getTestOnCreate(),
+        getTestOnReturn(),
+        getTestWhileIdle(),
+        getTimeBetweenEvictionRunsMillis(),
+        getMaxIdlePerKey(),
+        getMaxTotal(),
+        getMaxTotalPerKey(),
+        getMinIdlePerKey(),
+        getEvictionPolicyClassName(),
+        getJmxNameBase(),
+        getJmxNamePrefix());
+  }
+}