diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java index 6d3a56f40ed..3e59092c74f 100644 --- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java +++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmSnapshot.java @@ -29,6 +29,7 @@ import java.util.stream.Collectors; import org.apache.hadoop.hdds.client.RatisReplicationConfig; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; +import org.apache.hadoop.hdds.utils.LeakDetector; import org.apache.hadoop.ozone.OzoneAcl; import org.apache.hadoop.ozone.audit.AuditLogger; import org.apache.hadoop.ozone.audit.AuditLoggerType; @@ -48,6 +49,7 @@ import org.apache.hadoop.ozone.security.acl.OzoneObj; import org.apache.hadoop.ozone.security.acl.OzoneObjInfo; import org.apache.hadoop.util.Time; +import org.apache.ratis.util.UncheckedAutoCloseable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -72,6 +74,8 @@ public class OmSnapshot implements IOmMetadataReader, Closeable { private static final AuditLogger AUDIT = new AuditLogger( AuditLoggerType.OMLOGGER); + private static final LeakDetector LEAK_DETECTOR = new LeakDetector(OmSnapshot.class.getName()); + private final OmMetadataReader omMetadataReader; private final String volumeName; private final String bucketName; @@ -80,6 +84,8 @@ public class OmSnapshot implements IOmMetadataReader, Closeable { // To access snapshot checkpoint DB metadata private final OMMetadataManager omMetadataManager; private final KeyManager keyManager; + /** Warns if this snapshot is garbage collected without being closed. Assigned in the constructor. */ + private final UncheckedAutoCloseable leakTracker; public OmSnapshot(KeyManager keyManager, PrefixManager prefixManager, @@ -100,6 +106,8 @@ public OmSnapshot(KeyManager keyManager, this.snapshotID = snapshotID; this.keyManager = keyManager; this.omMetadataManager = keyManager.getMetadataManager(); + this.leakTracker = LEAK_DETECTOR.track(this, + newLeakReporter(omMetadataManager.getStore().toString(), snapshotName)); } @Override @@ -343,21 +351,21 @@ public UUID getSnapshotID() { @Override public void close() throws IOException { - // Close DB - omMetadataManager.getStore().close(); + try { + // Close DB + omMetadataManager.getStore().close(); + } finally { + // Closed properly: stop tracking so the leak reporter does not fire at GC. + leakTracker.close(); + } } - @Override - protected void finalize() throws Throwable { - // Verify that the DB handle has been closed, log warning otherwise - // https://softwareengineering.stackexchange.com/a/288724 - if (!omMetadataManager.getStore().isClosed()) { - LOG.warn("{} is not closed properly. snapshotName: {}", - // Print hash code for debugging - omMetadataManager.getStore().toString(), - snapshotName); - } - super.finalize(); + /** + * @return a leak reporter that captures only the store description and the snapshot name, + * never the {@link OmSnapshot} itself. + */ + private static Runnable newLeakReporter(String storeDescription, String snapshotName) { + return () -> LOG.warn("{} is not closed properly. snapshotName: {}", storeDescription, snapshotName); } @VisibleForTesting diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java new file mode 100644 index 00000000000..5162b33ccc6 --- /dev/null +++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/TestOmSnapshotLeakDetection.java @@ -0,0 +1,87 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.hadoop.ozone.om; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.UUID; +import org.apache.hadoop.hdds.utils.db.DBStore; +import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer; +import org.apache.ozone.test.GenericTestUtils.LogCapturer; +import org.junit.jupiter.api.Test; + +/** + * Test {@link OmSnapshot}'s leak detection: the snapshot registers with a shared LeakDetector + * and warns if it is garbage collected without being closed. + */ +class TestOmSnapshotLeakDetection { + + /** + * Drive an actual {@link OmSnapshot} instance through GC without closing it and verify the leak + * is detected. Collaborators (including the {@link DBStore}) are mocked, so no metadata store is + * opened; this exercises the constructor's LeakDetector registration and the GC-triggered report. + */ + @Test + void leakDetectedForUnclosedSnapshot() throws Exception { + try (LogCapturer logs = LogCapturer.captureLogs(OmSnapshot.class)) { + OmSnapshot snapshot = newSnapshotWithMockedStore(); + assertThat(snapshot).isNotNull(); + + // Drop the only strong reference; the reporter captures no reference back to the snapshot, + // so it becomes collectible. The report runs asynchronously on the LeakDetector thread. + snapshot = null; + for (int i = 0; i < 50 && !logs.getOutput().contains("is not closed properly"); i++) { + System.gc(); + Thread.sleep(100); + } + assertThat(logs.getOutput()).contains("is not closed properly. snapshotName: snap-1"); + } + } + + /** Closing the snapshot stops the leak tracker, so a GC afterwards must not report a leak. */ + @Test + void closedSnapshotDoesNotReportLeak() throws Exception { + try (LogCapturer logs = LogCapturer.captureLogs(OmSnapshot.class)) { + OmSnapshot snapshot = newSnapshotWithMockedStore(); + snapshot.close(); + + snapshot = null; + System.gc(); + Thread.sleep(100); + assertThat(logs.getOutput()).doesNotContain("is not closed properly"); + } + } + + private static OmSnapshot newSnapshotWithMockedStore() { + DBStore store = mock(DBStore.class); + OMMetadataManager metadataManager = mock(OMMetadataManager.class); + when(metadataManager.getStore()).thenReturn(store); + KeyManager keyManager = mock(KeyManager.class); + when(keyManager.getMetadataManager()).thenReturn(metadataManager); + OzoneManager ozoneManager = mock(OzoneManager.class); + IAccessAuthorizer authorizer = mock(IAccessAuthorizer.class); + when(ozoneManager.getAccessAuthorizer()).thenReturn(authorizer); + when(authorizer.isNative()).thenReturn(false); + PrefixManager prefixManager = mock(PrefixManager.class); + + return new OmSnapshot(keyManager, prefixManager, ozoneManager, + "vol", "bucket", "snap-1", UUID.randomUUID()); + } +}