From d5a0990e075abff8c1046df87554a3753736577c Mon Sep 17 00:00:00 2001
From: Julien Viet <julien@julienviet.com>
Date: Mon, 3 Feb 2020 14:25:33 +0100
Subject: [PATCH] VerticleManager should not use the number of deployed
 instances to close an IsolatedClassLoader. Instead it should care of the
 associated Deployment life cycle

---
 .../java/io/vertx/core/impl/Deployment.java   |  2 ++
 .../io/vertx/core/impl/DeploymentManager.java | 32 ++++++++++++++++++-
 .../io/vertx/core/impl/VerticleManager.java   | 27 +++++++---------
 3 files changed, 45 insertions(+), 16 deletions(-)

diff --git a/src/main/java/io/vertx/core/impl/Deployment.java b/src/main/java/io/vertx/core/impl/Deployment.java
index 299eb75ec..11f0dc63b 100644
--- a/src/main/java/io/vertx/core/impl/Deployment.java
+++ b/src/main/java/io/vertx/core/impl/Deployment.java
@@ -54,5 +54,7 @@ public interface Deployment {
 
   Set<Verticle> getVerticles();
 
+  void undeployHandler(Handler<Void> handler);
+
   boolean isChild();
 }
diff --git a/src/main/java/io/vertx/core/impl/DeploymentManager.java b/src/main/java/io/vertx/core/impl/DeploymentManager.java
index 8bcd1b703..fb0df3d30 100644
--- a/src/main/java/io/vertx/core/impl/DeploymentManager.java
+++ b/src/main/java/io/vertx/core/impl/DeploymentManager.java
@@ -261,6 +261,7 @@ public class DeploymentManager {
     private final List<VerticleHolder> verticles = new CopyOnWriteArrayList<>();
     private final Set<Deployment> children = new ConcurrentHashSet<>();
     private final DeploymentOptions options;
+    private Handler<Void> undeployHandler;
     private int status = ST_DEPLOYED;
     private volatile boolean child;
 
@@ -280,8 +281,18 @@ public class DeploymentManager {
       if (status == ST_DEPLOYED) {
         status = ST_UNDEPLOYING;
         doUndeployChildren(callingContext).setHandler(childrenResult -> {
+          Handler<Void> handler;
           synchronized (DeploymentImpl.this) {
             status = ST_UNDEPLOYED;
+            handler = undeployHandler;
+            undeployHandler = null;
+          }
+          if (handler != null) {
+            try {
+              handler.handle(null);
+            } catch (Exception e) {
+              context.reportException(e);
+            }
           }
           if (childrenResult.failed()) {
             reportFailure(cause, callingContext, completionHandler);
@@ -364,7 +375,15 @@ public class DeploymentManager {
         }
         Promise<Void> resolvingPromise = undeployingContext.promise();
         CompositeFuture.all(undeployFutures).<Void>mapEmpty().setHandler(resolvingPromise);
-        return resolvingPromise.future();
+        Future<Void> fut = resolvingPromise.future();
+        Handler<Void> handler = undeployHandler;
+        if (handler != null) {
+          undeployHandler = null;
+          fut.onComplete(ar -> {
+            handler.handle(null);
+          });
+        }
+        return fut;
       }
     }
 
@@ -416,6 +435,17 @@ public class DeploymentManager {
       return verts;
     }
 
+    @Override
+    public void undeployHandler(Handler<Void> handler) {
+      synchronized (this) {
+        if (status != ST_UNDEPLOYED) {
+          undeployHandler = handler;
+          return;
+        }
+      }
+      handler.handle(null);
+    }
+
     @Override
     public boolean isChild() {
       return child;
diff --git a/src/main/java/io/vertx/core/impl/VerticleManager.java b/src/main/java/io/vertx/core/impl/VerticleManager.java
index e5027df88..fa4d96dd2 100644
--- a/src/main/java/io/vertx/core/impl/VerticleManager.java
+++ b/src/main/java/io/vertx/core/impl/VerticleManager.java
@@ -206,24 +206,21 @@ public class VerticleManager {
             Future<Deployment> fut = p.future().compose(callable -> deploymentManager.doDeploy(options, v -> identifier, parentContext, callingContext, cl, callable));
             String group = options.getIsolationGroup();
             if (group != null) {
-              fut.setHandler(ar -> {
+              fut.onComplete(ar -> {
                 if (ar.succeeded()) {
                   Deployment result = ar.result();
-                  result.getContexts().forEach(ctx -> {
-                    ctx.addCloseHook(hook -> {
-                      synchronized (VerticleManager.this) {
-                        IsolatingClassLoader icl = classloaders.get(group);
-                        if (--icl.refCount == 0) {
-                          classloaders.remove(group);
-                          try {
-                            icl.close();
-                          } catch (IOException e) {
-                            // log.debug("Issue when closing isolation group loader", e);
-                          }
+                  result.undeployHandler(v -> {
+                    synchronized (VerticleManager.this) {
+                      IsolatingClassLoader icl = classloaders.get(group);
+                      if (--icl.refCount == 0) {
+                        classloaders.remove(group);
+                        try {
+                          icl.close();
+                        } catch (IOException e) {
+                          // log.debug("Issue when closing isolation group loader", e);
                         }
                       }
-                      hook.complete();
-                    });
+                    }
                   });
                 } else {
                   // ??? not tested
@@ -291,7 +288,7 @@ public class VerticleManager {
             options.getIsolatedClasses());
           classloaders.put(isolationGroup, icl);
         }
-        icl.refCount += options.getInstances();
+        icl.refCount++;
         cl = icl;
       }
     }