Close owned mDNS instance and remove shutdown hook on stop

JmdnsHomekitAdvertiser created its own JmDNS when given an InetAddress but
never closed it, and registered a shutdown hook on every advertise() that was
never removed. Each stopped HomekitRoot therefore left a running JmDNS
instance behind, kept reachable by its shutdown hook.

stop() now removes the shutdown hook, closes the JmDNS only if the advertiser
created it, and resets the advertising state so later changes to
discoverability, MAC or configuration index do not re-register the service.

Author: Eugen

CHANGES.md

@@ -1,5 +1,6 @@
 # HAP-Java 2.0.10
 * Add optional Mute characteristic to Doorbell service
+* Fix leak of mDNS instances and shutdown hooks when stopping `HomekitRoot`
 
 # HAP-Java 2.0.9
 * Fix Set Duration and Remaining Duration characteristics to advertise format `uint32` instead of `int` [#202](https://github.com/hap-java/HAP-Java/issues/202)

src/main/java/io/github/hapjava/server/impl/jmdns/JmdnsHomekitAdvertiser.java

@@ -17,6 +17,8 @@ public class JmdnsHomekitAdvertiser {
   private static final String SERVICE_TYPE = "_hap._tcp.local.";
 
   private final JmDNS jmdns;
+  private final boolean ownsJmdns;
+  private Thread shutdownHook;
   private boolean discoverable = true;
   private static final Logger logger = LoggerFactory.getLogger(JmdnsHomekitAdvertiser.class);
   private boolean isAdvertising = false;
@@ -30,11 +32,16 @@ public class JmdnsHomekitAdvertiser {
   private int category;
 
   public JmdnsHomekitAdvertiser(JmDNS jmdns) {
-    this.jmdns = jmdns;
+    this(jmdns, false);
   }
 
   public JmdnsHomekitAdvertiser(InetAddress localAddress) throws UnknownHostException, IOException {
-    jmdns = JmDNS.create(localAddress);
+    this(JmDNS.create(localAddress), true);
+  }
+
+  JmdnsHomekitAdvertiser(JmDNS jmdns, boolean ownsJmdns) {
+    this.jmdns = jmdns;
+    this.ownsJmdns = ownsJmdns;
   }
 
   public synchronized void advertise(
@@ -54,18 +61,33 @@ public synchronized void advertise(
 
     registerService();
 
-    Runtime.getRuntime()
-        .addShutdownHook(
-            new Thread(
-                () -> {
-                  logger.trace("Stopping advertising in response to shutdown.");
-                  jmdns.unregisterAllServices();
-                }));
+    shutdownHook =
+        new Thread(
+            () -> {
+              logger.trace("Stopping advertising in response to shutdown.");
+              jmdns.unregisterAllServices();
+            });
+    Runtime.getRuntime().addShutdownHook(shutdownHook);
     isAdvertising = true;
   }
 
   public synchronized void stop() {
     unregisterService();
+    isAdvertising = false;
+    if (shutdownHook != null) {
+      try {
+        Runtime.getRuntime().removeShutdownHook(shutdownHook);
+      } catch (IllegalStateException e) {
+        // already shutting down
+      }
+    }
+    if (ownsJmdns) {
+      try {
+        jmdns.close();
+      } catch (IOException e) {
+        logger.warn("Failed to close mDNS", e);
+      }
+    }
   }
 
   public synchronized void setDiscoverable(boolean discoverable) throws IOException {

src/test/java/io/github/hapjava/server/impl/jmdns/JmdnsHomekitAdvertiserTest.java

@@ -2,7 +2,9 @@
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.verify;
 
 import java.io.IOException;
@@ -54,6 +56,29 @@ public void testSetConfigurationIndex() throws Exception {
     assertThat(getArgumentFromUnregister().getPropertyString("c#")).isEqualTo("1");
   }
 
+  @Test
+  public void testStopClosesOwnedJmdns() throws Exception {
+    subject = new JmdnsHomekitAdvertiser(jmdns, true);
+    advertise();
+    subject.stop();
+    verify(jmdns).close();
+  }
+
+  @Test
+  public void testStopDoesNotCloseSuppliedJmdns() throws Exception {
+    advertise();
+    subject.stop();
+    verify(jmdns, never()).close();
+  }
+
+  @Test
+  public void testSetDiscoverableAfterStopDoesNotRegister() throws Exception {
+    advertise();
+    subject.stop();
+    subject.setDiscoverable(false);
+    verify(jmdns).registerService(any(ServiceInfo.class));
+  }
+
   private ServiceInfo getArgumentFromUnregister() {
     ArgumentCaptor<ServiceInfo> serviceInfoCaptor = ArgumentCaptor.forClass(ServiceInfo.class);
     verify(jmdns).unregisterService(serviceInfoCaptor.capture());