Use java.lang.ref.Cleaner in controller.cluster.io 86/93286/2
authortadei.bilan <tadei.bilan@pantheon.tech>
Fri, 23 Oct 2020 11:12:06 +0000 (14:12 +0300)
committerRobert Varga <robert.varga@pantheon.tech>
Mon, 16 Nov 2020 16:39:50 +0000 (17:39 +0100)
Improve temporary file clean up by using a Cleaner to dispatch
cleaning tasks. Since it gives us a Cleanable, we can dispense with
a tracking map and removal from it -- Cleanable makes sure it is
called exactly once.

JIRA: CONTROLLER-1911
Change-Id: I5fb715102912359cf002129d25a7433199826982
Signed-off-by: tadei.bilan <tadei.bilan@pantheon.tech>
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
opendaylight/md-sal/sal-clustering-commons/src/main/java/org/opendaylight/controller/cluster/io/FileBackedOutputStream.java
opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/FileBackedOutputStreamTest.java
opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/SharedFileBackedOutputStreamTest.java

index 970b06f..029464a 100644 (file)
@@ -7,10 +7,6 @@
  */
 package org.opendaylight.controller.cluster.io;
 
  */
 package org.opendaylight.controller.cluster.io;
 
-import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.FinalizablePhantomReference;
-import com.google.common.base.FinalizableReferenceQueue;
-import com.google.common.collect.Sets;
 import com.google.common.io.ByteSource;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.ByteArrayInputStream;
 import com.google.common.io.ByteSource;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.io.ByteArrayInputStream;
@@ -19,9 +15,9 @@ import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.lang.ref.Cleaner;
+import java.lang.ref.Cleaner.Cleanable;
 import java.nio.file.Files;
 import java.nio.file.Files;
-import java.util.Iterator;
-import java.util.Set;
 import org.checkerframework.checker.lock.qual.GuardedBy;
 import org.checkerframework.checker.lock.qual.Holding;
 import org.eclipse.jdt.annotation.NonNull;
 import org.checkerframework.checker.lock.qual.GuardedBy;
 import org.checkerframework.checker.lock.qual.Holding;
 import org.eclipse.jdt.annotation.NonNull;
@@ -39,17 +35,10 @@ public class FileBackedOutputStream extends OutputStream {
     private static final Logger LOG = LoggerFactory.getLogger(FileBackedOutputStream.class);
 
     /**
     private static final Logger LOG = LoggerFactory.getLogger(FileBackedOutputStream.class);
 
     /**
-     * This stores the Cleanup PhantomReference instances statically. This is necessary because PhantomReferences
-     * need a hard reference so they're not garbage collected. Once finalized, the Cleanup PhantomReference removes
-     * itself from this map and thus becomes eligible for garbage collection.
+     * A Cleaner instance responsible for deleting any files which may be lost due to us not being cleaning up
+     * temporary files.
      */
      */
-    @VisibleForTesting
-    static final Set<Cleanup> REFERENCE_CACHE = Sets.newConcurrentHashSet();
-
-    /**
-     * Used as the ReferenceQueue for the Cleanup PhantomReferences.
-     */
-    private static final FinalizableReferenceQueue REFERENCE_QUEUE = new FinalizableReferenceQueue();
+    private static final Cleaner FILE_CLEANER = Cleaner.create();
 
     private final int fileThreshold;
     private final String fileDirectory;
 
     private final int fileThreshold;
     private final String fileDirectory;
@@ -63,6 +52,9 @@ public class FileBackedOutputStream extends OutputStream {
     @GuardedBy("this")
     private File file;
 
     @GuardedBy("this")
     private File file;
 
+    @GuardedBy("this")
+    private Cleanable fileCleanup;
+
     @GuardedBy("this")
     private ByteSource source;
 
     @GuardedBy("this")
     private ByteSource source;
 
@@ -160,23 +152,12 @@ public class FileBackedOutputStream extends OutputStream {
      */
     public synchronized void cleanup() {
         LOG.debug("In cleanup");
      */
     public synchronized void cleanup() {
         LOG.debug("In cleanup");
-
         closeQuietly();
         closeQuietly();
-
-        if (file != null) {
-            Iterator<Cleanup> iter = REFERENCE_CACHE.iterator();
-            while (iter.hasNext()) {
-                if (file.equals(iter.next().file)) {
-                    iter.remove();
-                    break;
-                }
-            }
-
-            LOG.debug("cleanup - deleting temp file {}", file);
-
-            deleteFile(file);
-            file = null;
+        if (fileCleanup != null) {
+            fileCleanup.clean();
         }
         }
+        // Already deleted above
+        file = null;
     }
 
     @Holding("this")
     }
 
     @Holding("this")
@@ -198,41 +179,43 @@ public class FileBackedOutputStream extends OutputStream {
         }
 
         if (file == null && memory.getCount() + len > fileThreshold) {
         }
 
         if (file == null && memory.getCount() + len > fileThreshold) {
-            File temp = File.createTempFile("FileBackedOutputStream", null,
+            final File temp = File.createTempFile("FileBackedOutputStream", null,
                     fileDirectory == null ? null : new File(fileDirectory));
             temp.deleteOnExit();
                     fileDirectory == null ? null : new File(fileDirectory));
             temp.deleteOnExit();
+            final Cleaner.Cleanable cleanup = FILE_CLEANER.register(this, () -> deleteFile(temp));
 
             LOG.debug("Byte count {} has exceeded threshold {} - switching to file: {}", memory.getCount() + len,
                     fileThreshold, temp);
 
 
             LOG.debug("Byte count {} has exceeded threshold {} - switching to file: {}", memory.getCount() + len,
                     fileThreshold, temp);
 
-            OutputStream transfer = null;
+            final OutputStream transfer;
             try {
                 transfer = Files.newOutputStream(temp.toPath());
             try {
                 transfer = Files.newOutputStream(temp.toPath());
-                transfer.write(memory.getBuffer(), 0, memory.getCount());
-                transfer.flush();
-
-                // We've successfully transferred the data; switch to writing to file
-                out = transfer;
-                file = temp;
-                memory = null;
-
-                new Cleanup(this, file);
-            } catch (IOException e) {
-                if (transfer != null) {
+                try {
+                    transfer.write(memory.getBuffer(), 0, memory.getCount());
+                    transfer.flush();
+                } catch (IOException e) {
                     try {
                         transfer.close();
                     } catch (IOException ex) {
                         LOG.debug("Error closing temp file {}", temp, ex);
                     }
                     try {
                         transfer.close();
                     } catch (IOException ex) {
                         LOG.debug("Error closing temp file {}", temp, ex);
                     }
+                    throw e;
                 }
                 }
-
-                deleteFile(temp);
+            } catch (IOException e) {
+                cleanup.clean();
                 throw e;
             }
                 throw e;
             }
+
+            // We've successfully transferred the data; switch to writing to file
+            out = transfer;
+            file = temp;
+            fileCleanup = cleanup;
+            memory = null;
         }
     }
 
     private static void deleteFile(final File file) {
         }
     }
 
     private static void deleteFile(final File file) {
+        LOG.debug("Deleting temp file {}", file);
         if (!file.delete()) {
             LOG.warn("Could not delete temp file {}", file);
         }
         if (!file.delete()) {
             LOG.warn("Could not delete temp file {}", file);
         }
@@ -250,30 +233,4 @@ public class FileBackedOutputStream extends OutputStream {
             return count;
         }
     }
             return count;
         }
     }
-
-    /**
-     * PhantomReference that deletes the temp file when the FileBackedOutputStream is garbage collected.
-     */
-    private static class Cleanup extends FinalizablePhantomReference<FileBackedOutputStream> {
-        private final File file;
-
-        Cleanup(final FileBackedOutputStream referent, final File file) {
-            super(referent, REFERENCE_QUEUE);
-            this.file = file;
-
-            REFERENCE_CACHE.add(this);
-
-            LOG.debug("Added Cleanup for temp file {}", file);
-        }
-
-        @Override
-        public void finalizeReferent() {
-            LOG.debug("In finalizeReferent");
-
-            if (REFERENCE_CACHE.remove(this)) {
-                LOG.debug("finalizeReferent - deleting temp file {}", file);
-                deleteFile(file);
-            }
-        }
-    }
 }
 }
index a7abf7c..ff5d61b 100644 (file)
@@ -52,7 +52,6 @@ public class FileBackedOutputStreamTest {
     @Before
     public void setup() {
         deleteTempFiles(TEMP_DIR);
     @Before
     public void setup() {
         deleteTempFiles(TEMP_DIR);
-        FileBackedOutputStream.REFERENCE_CACHE.clear();
     }
 
     @After
     }
 
     @After
@@ -76,8 +75,6 @@ public class FileBackedOutputStreamTest {
             assertArrayEquals("Read bytes", bytes, fbos.asByteSource().read());
             assertArrayEquals("Read bytes", bytes, fbos.asByteSource().read());
 
             assertArrayEquals("Read bytes", bytes, fbos.asByteSource().read());
             assertArrayEquals("Read bytes", bytes, fbos.asByteSource().read());
 
-            assertEquals("Reference cache size", 0, FileBackedOutputStream.REFERENCE_CACHE.size());
-
             fbos.cleanup();
         }
 
             fbos.cleanup();
         }
 
@@ -112,12 +109,8 @@ public class FileBackedOutputStreamTest {
 
             inputStream.close();
 
 
             inputStream.close();
 
-            assertEquals("Reference cache size", 1, FileBackedOutputStream.REFERENCE_CACHE.size());
-
             fbos.cleanup();
 
             fbos.cleanup();
 
-            assertEquals("Reference cache size", 0, FileBackedOutputStream.REFERENCE_CACHE.size());
-
             assertNull("Found unexpected temp file", findTempFileName(TEMP_DIR));
         }
 
             assertNull("Found unexpected temp file", findTempFileName(TEMP_DIR));
         }
 
index 0d6f581..71e1866 100644 (file)
@@ -46,7 +46,6 @@ public class SharedFileBackedOutputStreamTest {
     @Before
     public void setup() {
         FileBackedOutputStreamTest.deleteTempFiles(TEMP_DIR);
     @Before
     public void setup() {
         FileBackedOutputStreamTest.deleteTempFiles(TEMP_DIR);
-        FileBackedOutputStream.REFERENCE_CACHE.clear();
     }
 
     @After
     }
 
     @After

©2013 OpenDaylight, A Linux Foundation Collaborative Project. All Rights Reserved.
OpenDaylight is a registered trademark of The OpenDaylight Project, Inc.
Linux Foundation and OpenDaylight are registered trademarks of the Linux Foundation.
Linux is a registered trademark of Linus Torvalds.