From: tadei.bilan Date: Fri, 23 Oct 2020 11:12:06 +0000 (+0300) Subject: Use java.lang.ref.Cleaner in controller.cluster.io X-Git-Tag: v3.0.2~7 X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?p=controller.git;a=commitdiff_plain;h=e0e956ecf0a4e5085f33e67f4fb73141876f1668;ds=sidebyside Use java.lang.ref.Cleaner in controller.cluster.io 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 Signed-off-by: Robert Varga --- diff --git a/opendaylight/md-sal/sal-clustering-commons/src/main/java/org/opendaylight/controller/cluster/io/FileBackedOutputStream.java b/opendaylight/md-sal/sal-clustering-commons/src/main/java/org/opendaylight/controller/cluster/io/FileBackedOutputStream.java index 970b06f5c2..029464a82b 100644 --- a/opendaylight/md-sal/sal-clustering-commons/src/main/java/org/opendaylight/controller/cluster/io/FileBackedOutputStream.java +++ b/opendaylight/md-sal/sal-clustering-commons/src/main/java/org/opendaylight/controller/cluster/io/FileBackedOutputStream.java @@ -7,10 +7,6 @@ */ 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; @@ -19,9 +15,9 @@ import java.io.File; 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.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; @@ -39,17 +35,10 @@ public class FileBackedOutputStream extends OutputStream { 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 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; @@ -63,6 +52,9 @@ public class FileBackedOutputStream extends OutputStream { @GuardedBy("this") private File file; + @GuardedBy("this") + private Cleanable fileCleanup; + @GuardedBy("this") private ByteSource source; @@ -160,23 +152,12 @@ public class FileBackedOutputStream extends OutputStream { */ public synchronized void cleanup() { LOG.debug("In cleanup"); - closeQuietly(); - - if (file != null) { - Iterator 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") @@ -198,41 +179,43 @@ public class FileBackedOutputStream extends OutputStream { } 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(); + 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); - OutputStream transfer = null; + final OutputStream transfer; 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); } + throw e; } - - deleteFile(temp); + } catch (IOException e) { + cleanup.clean(); 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) { + LOG.debug("Deleting 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; } } - - /** - * PhantomReference that deletes the temp file when the FileBackedOutputStream is garbage collected. - */ - private static class Cleanup extends FinalizablePhantomReference { - 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); - } - } - } } diff --git a/opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/FileBackedOutputStreamTest.java b/opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/FileBackedOutputStreamTest.java index a7abf7ca97..ff5d61b41d 100644 --- a/opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/FileBackedOutputStreamTest.java +++ b/opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/FileBackedOutputStreamTest.java @@ -52,7 +52,6 @@ public class FileBackedOutputStreamTest { @Before public void setup() { deleteTempFiles(TEMP_DIR); - FileBackedOutputStream.REFERENCE_CACHE.clear(); } @After @@ -76,8 +75,6 @@ public class FileBackedOutputStreamTest { 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(); } @@ -112,12 +109,8 @@ public class FileBackedOutputStreamTest { inputStream.close(); - assertEquals("Reference cache size", 1, FileBackedOutputStream.REFERENCE_CACHE.size()); - fbos.cleanup(); - assertEquals("Reference cache size", 0, FileBackedOutputStream.REFERENCE_CACHE.size()); - assertNull("Found unexpected temp file", findTempFileName(TEMP_DIR)); } diff --git a/opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/SharedFileBackedOutputStreamTest.java b/opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/SharedFileBackedOutputStreamTest.java index 0d6f581123..71e1866f32 100644 --- a/opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/SharedFileBackedOutputStreamTest.java +++ b/opendaylight/md-sal/sal-clustering-commons/src/test/java/org/opendaylight/controller/cluster/io/SharedFileBackedOutputStreamTest.java @@ -46,7 +46,6 @@ public class SharedFileBackedOutputStreamTest { @Before public void setup() { FileBackedOutputStreamTest.deleteTempFiles(TEMP_DIR); - FileBackedOutputStream.REFERENCE_CACHE.clear(); } @After