Merge pull request #214 from hap-java/fix/close-owned-jmdns

Close owned mDNS instance and remove shutdown hook on stop

Author: yfre

CHANGES.md

@@ -1,6 +1,7 @@
 # HAP-Java 2.0.10
 * Add constructors with custom min/max to the window covering tilt angle characteristics [#162](https://github.com/hap-java/HAP-Java/issues/162)
 * 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());