[beam] 01/01: Merge pull request #10345: [BEAM-8943] Cleanup servers in the presence of environment failures

[mx...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

mxm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/beam.git

commit 701c4be8436a77a75a431b4c213e8fd85f17c8c3
Merge: b0c1d8c b716151
Author: Maximilian Michels <mx...@apache.org>
AuthorDate: Thu Dec 12 13:03:12 2019 +0100

    Merge pull request #10345: [BEAM-8943] Cleanup servers in the presence of environment failures

 .../control/DefaultJobBundleFactory.java           | 29 ++++++++++++----------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --cc runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/DefaultJobBundleFactory.java
index a3f6f01,8fb3e87..35146c6
--- a/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/DefaultJobBundleFactory.java
+++ b/runners/java-fn-execution/src/main/java/org/apache/beam/runners/fnexecution/control/DefaultJobBundleFactory.java
@@@ -374,17 -374,18 +374,24 @@@ public class DefaultJobBundleFactory im
        return serverInfo;
      }
  
-     @Override
-     public void close() throws Exception {
-       try (AutoCloseable envCloser = environment) {
-         // Wrap resources in try-with-resources to ensure all are cleaned up.
-       }
-       try (AutoCloseable stateServer = serverInfo.getStateServer();
+     public void close() {
++      // DO NOT ADD ANYTHING HERE WHICH MIGHT CAUSE THE BLOCK BELOW TO NOT BE EXECUTED.
++      // If we exit prematurely (e.g. due to an exception), resources won't be cleaned up properly.
++      // Please make an AutoCloseable and add it to the try statement below.
+       try (AutoCloseable envCloser = environment;
+           AutoCloseable stateServer = serverInfo.getStateServer();
            AutoCloseable dateServer = serverInfo.getDataServer();
            AutoCloseable controlServer = serverInfo.getControlServer();
            AutoCloseable loggingServer = serverInfo.getLoggingServer();
            AutoCloseable retrievalServer = serverInfo.getRetrievalServer();
-           AutoCloseable provisioningServer = serverInfo.getProvisioningServer()) {}
+           AutoCloseable provisioningServer = serverInfo.getProvisioningServer()) {
+         // Wrap resources in try-with-resources to ensure all are cleaned up.
++        // This will close _all_ of these even in the presence of exceptions.
++        // The first exception encountered will be the base exception,
++        // the next one will be added via Throwable#addSuppressed.
+       } catch (Exception e) {
+         LOG.warn("Error cleaning up servers {}", environment.getEnvironment(), e);
+       }
        // TODO: Wait for executor shutdown?
      }