From 75a16cba5b6f072d32fbc48bb927cf2afd87f5c7 Mon Sep 17 00:00:00 2001 From: Emmeran Seehuber Date: Thu, 13 Jan 2022 18:50:27 +0100 Subject: [PATCH 1/4] First implementation of a Cleaner which uses JDK9 cleaner if possible and falls back to JDK8 finalize() if not. --- .../org/apache/fontbox/ttf/TrueTypeFont.java | 30 +++- .../fontbox/util/PdfBoxInternalCleaner.java | 169 ++++++++++++++++++ .../util/PdfBoxInternalCleanerTest.java | 54 ++++++ 3 files changed, 244 insertions(+), 9 deletions(-) create mode 100644 fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java create mode 100644 fontbox/src/test/java/org/apache/fontbox/util/PdfBoxInternalCleanerTest.java diff --git a/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java b/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java index 518106ce247..17729ef4115 100644 --- a/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java +++ b/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java @@ -33,6 +33,9 @@ import org.apache.fontbox.FontBoxFont; import org.apache.fontbox.ttf.model.GsubData; import org.apache.fontbox.util.BoundingBox; +import org.apache.fontbox.util.PdfBoxInternalCleaner; +import org.apache.fontbox.util.PdfBoxInternalCleaner.Cleanable; +import org.apache.fontbox.util.PdfBoxInternalCleaner.CleaningRunnable; /** * A TrueType font file. @@ -55,6 +58,22 @@ public class TrueTypeFont implements FontBoxFont, Closeable private final Object lockPSNames = new Object(); private final List enabledGsubFeatures = new ArrayList<>(); + private final static PdfBoxInternalCleaner CLEANER = PdfBoxInternalCleaner.create(); + private final Cleanable cleanable; + private static class TTFCleaner implements CleaningRunnable { + TTFDataStream data; + public TTFCleaner(TTFDataStream data) + { + this.data = data; + } + + @Override + public void run() throws IOException + { + data.close(); + } + } + /** * Constructor. Clients should use the TTFParser to create a new TrueTypeFont object. * @@ -63,20 +82,13 @@ public class TrueTypeFont implements FontBoxFont, Closeable TrueTypeFont(TTFDataStream fontData) { data = fontData; + cleanable = CLEANER.register(this, new TTFCleaner(data)); } @Override public void close() throws IOException { - data.close(); - } - - @Override - protected void finalize() throws Throwable - { - super.finalize(); - // PDFBOX-4963: risk of memory leaks due to SoftReference in FontCache - close(); + cleanable.clean(); } /** diff --git a/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java b/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java new file mode 100644 index 00000000000..838e27c0976 --- /dev/null +++ b/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java @@ -0,0 +1,169 @@ +package org.apache.fontbox.util; + +import java.awt.geom.Line2D; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; + +/** + * *INTERNAL* class to avoid finalizers. When running on JDK <= 8 it will still use finalizers, from JDK 9+ it will use + * java.lang.Cleaner + * + * Do not use, ALWAYS use java.lang.ref.Cleaner.Cleanable directly. + * + * Note: You have to store a reference to the Cleanable object in the class which had a finalizer. Otherwise, this won't + * work. + */ +public class PdfBoxInternalCleaner +{ + + /** + * @see java.lang.ref.Cleaner.Cleanable + */ + public interface Cleanable + { + void clean(); + } + + private interface CleanerImpl + { + Cleanable register(Object obj, CleaningRunnable action); + } + + public interface CleaningRunnable + { + void run() throws Throwable; + } + + private CleanerImpl impl; + + /** + * Simple implementation for JDK <= 8 + */ + private static class CleanerImplJDK8 implements CleanerImpl + { + + static class ObjectFinalizer implements Cleanable + { + final CleaningRunnable action; + boolean didRun; + + ObjectFinalizer(CleaningRunnable action) + { + this.action = action; + } + + @Override + protected void finalize() throws Throwable + { + clean(); + } + + @Override + public void clean() + { + try + { + synchronized (this) + { + if (didRun) + return; + didRun = true; + action.run(); + } + } + catch (Throwable t) + { + throw new RuntimeException(t); + } + } + } + + @Override + public Cleanable register(Object obj, CleaningRunnable action) + { + return new ObjectFinalizer(action); + } + } + + /** + * Uses the JDK9 Cleaner using reflection + */ + private static class CleanerImplJDK9 implements CleanerImpl + { + private final Object cleanerImpl; + private final Method register; + private final Method clean; + + CleanerImplJDK9() throws ClassNotFoundException, InvocationTargetException, + IllegalAccessException, NoSuchMethodException + { + Class cleaner = getClass().getClassLoader().loadClass("java.lang.ref.Cleaner"); + Class cleanable = getClass().getClassLoader() + .loadClass("java.lang.ref.Cleaner$Cleanable"); + Method create = cleaner.getDeclaredMethod("create"); + cleanerImpl = create.invoke(null); + register = cleaner.getDeclaredMethod("register", Object.class, Runnable.class); + clean = cleanable.getDeclaredMethod("clean"); + } + + @Override + public Cleanable register(Object obj, CleaningRunnable action) + { + try + { + Object cleanable = register.invoke(cleanerImpl, obj, (Runnable) () -> { + try + { + action.run(); + } + catch (Throwable e) + { + throw new RuntimeException(e); + } + }); + return () -> { + try + { + clean.invoke(cleanable); + } + catch (Exception e) + { + throw new RuntimeException(e); + } + }; + } + catch (Exception e) + { + throw new RuntimeException(e); + } + } + } + + private static final boolean TRY_USING_JDK9_CLEANER = true; + + private PdfBoxInternalCleaner() + { + try + { + if (TRY_USING_JDK9_CLEANER) + impl = new CleanerImplJDK9(); + else + impl = new CleanerImplJDK8(); + } + catch (Throwable throwable) + { + impl = new CleanerImplJDK8(); + } + } + + public Cleanable register(Object obj, CleaningRunnable action) + { + return impl.register(obj, action); + } + + public static PdfBoxInternalCleaner create() + { + return new PdfBoxInternalCleaner(); + } + +} diff --git a/fontbox/src/test/java/org/apache/fontbox/util/PdfBoxInternalCleanerTest.java b/fontbox/src/test/java/org/apache/fontbox/util/PdfBoxInternalCleanerTest.java new file mode 100644 index 00000000000..5fabd98e261 --- /dev/null +++ b/fontbox/src/test/java/org/apache/fontbox/util/PdfBoxInternalCleanerTest.java @@ -0,0 +1,54 @@ +package org.apache.fontbox.util; + +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import java.util.concurrent.atomic.AtomicInteger; + +import org.junit.jupiter.api.Test; + +public class PdfBoxInternalCleanerTest +{ + + private static class ToCleanObject + { + private final static PdfBoxInternalCleaner cleaner = PdfBoxInternalCleaner.create(); + private final static AtomicInteger cleanups = new AtomicInteger(0); + private final static AtomicInteger created = new AtomicInteger(0); + + public ToCleanObject() + { + created.incrementAndGet(); + cleaner.register(this, cleanups::incrementAndGet); + } + } + + @Test + public void testCleaner() + { + for (int i = 0; i < 100; i++) + { + ToCleanObject object = new ToCleanObject(); + assertNotNull(object); + } + System.gc(); + dumpStats(); + for (int i = 0; i < 100_000; i++) + { + ToCleanObject object = new ToCleanObject(); + assertNotNull(object); + } + for (int i = 0; i < 5; i++) + System.gc(); + dumpStats(); + for (int i = 0; i < 5; i++) + System.gc(); + dumpStats(); + } + + private void dumpStats() + { + System.out.println("Created " + ToCleanObject.created.get() + " Cleanups Run: " + + ToCleanObject.cleanups.get()); + } + +} From 4238f61eaeaa7fe659917ebbe58b276df5e240a4 Mon Sep 17 00:00:00 2001 From: Emmeran Seehuber Date: Sat, 15 Jan 2022 17:58:58 +0100 Subject: [PATCH 2/4] Using logging for finalizer() instances and more comments. --- .../fontbox/util/PdfBoxInternalCleaner.java | 51 +++++++++++++++---- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java b/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java index 838e27c0976..59a3c071673 100644 --- a/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java +++ b/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java @@ -1,15 +1,17 @@ package org.apache.fontbox.util; -import java.awt.geom.Line2D; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; /** * *INTERNAL* class to avoid finalizers. When running on JDK <= 8 it will still use finalizers, from JDK 9+ it will use * java.lang.Cleaner - * - * Do not use, ALWAYS use java.lang.ref.Cleaner.Cleanable directly. - * + *

+ * Do not use, PDFBox internal use only! ALWAYS use java.lang.ref.Cleaner.Cleanable directly. + *

* Note: You have to store a reference to the Cleanable object in the class which had a finalizer. Otherwise, this won't * work. */ @@ -29,32 +31,49 @@ private interface CleanerImpl Cleanable register(Object obj, CleaningRunnable action); } + /** + * @see java.lang.ref.Cleaner.Cleanable + */ public interface CleaningRunnable { + /** + * Perform the cleanup action. It is ensured that this method will run only once. + */ void run() throws Throwable; } private CleanerImpl impl; /** - * Simple implementation for JDK <= 8 + * Simple implementation for JDK <= 8 using finalizers. */ private static class CleanerImplJDK8 implements CleanerImpl { static class ObjectFinalizer implements Cleanable { + final String simpleClassName; final CleaningRunnable action; boolean didRun; - ObjectFinalizer(CleaningRunnable action) + ObjectFinalizer(String simpleClassName, CleaningRunnable action) { + this.simpleClassName = simpleClassName; this.action = action; } + static final Log LOG = LogFactory.getLog(PdfBoxInternalCleaner.class); + @Override protected void finalize() throws Throwable { + synchronized (this) + { + if (!didRun) + { + LOG.debug(simpleClassName + " not closed!"); + } + } clean(); } @@ -81,7 +100,7 @@ public void clean() @Override public Cleanable register(Object obj, CleaningRunnable action) { - return new ObjectFinalizer(action); + return new ObjectFinalizer(obj.getClass().getSimpleName(), action); } } @@ -94,8 +113,9 @@ private static class CleanerImplJDK9 implements CleanerImpl private final Method register; private final Method clean; - CleanerImplJDK9() throws ClassNotFoundException, InvocationTargetException, - IllegalAccessException, NoSuchMethodException + CleanerImplJDK9() + throws ClassNotFoundException, InvocationTargetException, IllegalAccessException, + NoSuchMethodException { Class cleaner = getClass().getClassLoader().loadClass("java.lang.ref.Cleaner"); Class cleanable = getClass().getClassLoader() @@ -156,11 +176,24 @@ private PdfBoxInternalCleaner() } } + /** + * Register a cleanup action for the given object. You have to store the returned Cleanable instance in + * the object to have this all work correctly. + * + * @param obj the object which will trigger the cleanup action after it is GCed. + * @param action the cleanup action to perform + * @return a Cleanable instance. Call cleanup() on it if possible. + */ public Cleanable register(Object obj, CleaningRunnable action) { return impl.register(obj, action); } + /** + * Create a new cleaner instance. + * + * @return a new cleaner instance + */ public static PdfBoxInternalCleaner create() { return new PdfBoxInternalCleaner(); From 67d386fd3b39795e52cd732a4a323c01d0718dac Mon Sep 17 00:00:00 2001 From: Emmeran Seehuber Date: Sat, 15 Jan 2022 18:27:08 +0100 Subject: [PATCH 3/4] Migrate the finalize() method in ScratchFileBuffer to the cleaner. --- .../apache/pdfbox/io/ScratchFileBuffer.java | 75 ++++++++++--------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/pdfbox/src/main/java/org/apache/pdfbox/io/ScratchFileBuffer.java b/pdfbox/src/main/java/org/apache/pdfbox/io/ScratchFileBuffer.java index 96eb6b0b598..f04f95dfa7d 100644 --- a/pdfbox/src/main/java/org/apache/pdfbox/io/ScratchFileBuffer.java +++ b/pdfbox/src/main/java/org/apache/pdfbox/io/ScratchFileBuffer.java @@ -16,13 +16,11 @@ */ package org.apache.pdfbox.io; +import org.apache.fontbox.util.PdfBoxInternalCleaner; + import java.io.EOFException; import java.io.IOException; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; -import org.apache.pdfbox.cos.COSStream; - /** * Implementation of {@link RandomAccess} as sequence of multiple fixed size pages handled * by {@link ScratchFile}. @@ -64,8 +62,34 @@ class ScratchFileBuffer implements RandomAccess /** number of pages held by this buffer */ private int pageCount = 0; - private static final Log LOG = LogFactory.getLog(ScratchFileBuffer.class); - + private final static PdfBoxInternalCleaner CLEANER = PdfBoxInternalCleaner.create(); + private final PdfBoxInternalCleaner.Cleanable cleanable; + private final ScratchFileBufferCleaner scratchFileBufferCleaner; + + private static class ScratchFileBufferCleaner implements PdfBoxInternalCleaner.CleaningRunnable + { + private final ScratchFile pageHandler; + private int[] pageIndexes; + private int pageCount; + public ScratchFileBufferCleaner(ScratchFile pageHandler) + { + this.pageHandler = pageHandler; + } + + private void updateFields(ScratchFileBuffer buffer) + { + pageIndexes = buffer.pageIndexes; + pageCount = buffer.pageCount; + } + + @Override + public void run() throws IOException + { + pageHandler.markPagesAsFree(pageIndexes, 0, pageCount); + } + } + + /** * Creates a new buffer using pages handled by provided {@link ScratchFile}. * @@ -77,6 +101,9 @@ class ScratchFileBuffer implements RandomAccess { pageHandler.checkClosed(); + scratchFileBufferCleaner = new ScratchFileBufferCleaner(pageHandler); + cleanable = CLEANER.register(this, scratchFileBufferCleaner); + this.pageHandler = pageHandler; pageSize = this.pageHandler.getPageSize(); @@ -131,6 +158,8 @@ private void addPage() throws IOException pageCount++; currentPage = new byte[pageSize]; positionInPage = 0; + + scratchFileBufferCleaner.updateFields(this); } /** @@ -263,7 +292,8 @@ public final void clear() throws IOException // keep only the first page, discard all other pages pageHandler.markPagesAsFree(pageIndexes, 1, pageCount - 1); pageCount = 1; - + scratchFileBufferCleaner.updateFields(this); + // change to first page if we are not already there if (currentPagePositionInPageIndexes > 0) { @@ -422,11 +452,9 @@ public int read(byte[] b, int off, int len) throws IOException @Override public void close() throws IOException { + cleanable.clean();; if (pageHandler != null) { - - pageHandler.markPagesAsFree(pageIndexes, 0, pageCount); pageHandler = null; - pageIndexes = null; currentPage = null; currentPageOffset = 0; @@ -436,33 +464,6 @@ public void close() throws IOException } } - /** - * While calling finalize is normally discouraged we will have to - * use it here as long as closing a scratch file buffer is not - * done in every case. Currently {@link COSStream} creates new - * buffers without closing the old one - which might still be - * used. - * - *

Enabling debugging one will see if there are still cases - * where the buffer is not closed.

- */ - @Override - protected void finalize() throws Throwable - { - try - { - if ((pageHandler != null) && LOG.isDebugEnabled()) - { - LOG.debug("ScratchFileBuffer not closed!"); - } - close(); - } - finally - { - super.finalize(); - } - } - @Override public RandomAccessReadView createView(long startPosition, long streamLength) throws IOException { From eb399281bdb9ed1e2935970be904f4930de126db Mon Sep 17 00:00:00 2001 From: Emmeran Seehuber Date: Sat, 15 Jan 2022 18:31:04 +0100 Subject: [PATCH 4/4] Rename PdfBoxInternalCleaner to PDFBoxInternalCleaner. --- .../main/java/org/apache/fontbox/ttf/TrueTypeFont.java | 8 ++++---- ...InternalCleaner.java => PDFBoxInternalCleaner.java} | 10 +++++----- ...CleanerTest.java => PDFBoxInternalCleanerTest.java} | 4 ++-- .../java/org/apache/pdfbox/io/ScratchFileBuffer.java | 8 ++++---- 4 files changed, 15 insertions(+), 15 deletions(-) rename fontbox/src/main/java/org/apache/fontbox/util/{PdfBoxInternalCleaner.java => PDFBoxInternalCleaner.java} (96%) rename fontbox/src/test/java/org/apache/fontbox/util/{PdfBoxInternalCleanerTest.java => PDFBoxInternalCleanerTest.java} (92%) diff --git a/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java b/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java index 17729ef4115..d2a1de3c8d3 100644 --- a/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java +++ b/fontbox/src/main/java/org/apache/fontbox/ttf/TrueTypeFont.java @@ -33,9 +33,9 @@ import org.apache.fontbox.FontBoxFont; import org.apache.fontbox.ttf.model.GsubData; import org.apache.fontbox.util.BoundingBox; -import org.apache.fontbox.util.PdfBoxInternalCleaner; -import org.apache.fontbox.util.PdfBoxInternalCleaner.Cleanable; -import org.apache.fontbox.util.PdfBoxInternalCleaner.CleaningRunnable; +import org.apache.fontbox.util.PDFBoxInternalCleaner; +import org.apache.fontbox.util.PDFBoxInternalCleaner.Cleanable; +import org.apache.fontbox.util.PDFBoxInternalCleaner.CleaningRunnable; /** * A TrueType font file. @@ -58,7 +58,7 @@ public class TrueTypeFont implements FontBoxFont, Closeable private final Object lockPSNames = new Object(); private final List enabledGsubFeatures = new ArrayList<>(); - private final static PdfBoxInternalCleaner CLEANER = PdfBoxInternalCleaner.create(); + private final static PDFBoxInternalCleaner CLEANER = PDFBoxInternalCleaner.create(); private final Cleanable cleanable; private static class TTFCleaner implements CleaningRunnable { TTFDataStream data; diff --git a/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java b/fontbox/src/main/java/org/apache/fontbox/util/PDFBoxInternalCleaner.java similarity index 96% rename from fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java rename to fontbox/src/main/java/org/apache/fontbox/util/PDFBoxInternalCleaner.java index 59a3c071673..8673d10cb46 100644 --- a/fontbox/src/main/java/org/apache/fontbox/util/PdfBoxInternalCleaner.java +++ b/fontbox/src/main/java/org/apache/fontbox/util/PDFBoxInternalCleaner.java @@ -15,7 +15,7 @@ * Note: You have to store a reference to the Cleanable object in the class which had a finalizer. Otherwise, this won't * work. */ -public class PdfBoxInternalCleaner +public class PDFBoxInternalCleaner { /** @@ -62,7 +62,7 @@ static class ObjectFinalizer implements Cleanable this.action = action; } - static final Log LOG = LogFactory.getLog(PdfBoxInternalCleaner.class); + static final Log LOG = LogFactory.getLog(PDFBoxInternalCleaner.class); @Override protected void finalize() throws Throwable @@ -161,7 +161,7 @@ public Cleanable register(Object obj, CleaningRunnable action) private static final boolean TRY_USING_JDK9_CLEANER = true; - private PdfBoxInternalCleaner() + private PDFBoxInternalCleaner() { try { @@ -194,9 +194,9 @@ public Cleanable register(Object obj, CleaningRunnable action) * * @return a new cleaner instance */ - public static PdfBoxInternalCleaner create() + public static PDFBoxInternalCleaner create() { - return new PdfBoxInternalCleaner(); + return new PDFBoxInternalCleaner(); } } diff --git a/fontbox/src/test/java/org/apache/fontbox/util/PdfBoxInternalCleanerTest.java b/fontbox/src/test/java/org/apache/fontbox/util/PDFBoxInternalCleanerTest.java similarity index 92% rename from fontbox/src/test/java/org/apache/fontbox/util/PdfBoxInternalCleanerTest.java rename to fontbox/src/test/java/org/apache/fontbox/util/PDFBoxInternalCleanerTest.java index 5fabd98e261..f6ce45a741b 100644 --- a/fontbox/src/test/java/org/apache/fontbox/util/PdfBoxInternalCleanerTest.java +++ b/fontbox/src/test/java/org/apache/fontbox/util/PDFBoxInternalCleanerTest.java @@ -6,12 +6,12 @@ import org.junit.jupiter.api.Test; -public class PdfBoxInternalCleanerTest +public class PDFBoxInternalCleanerTest { private static class ToCleanObject { - private final static PdfBoxInternalCleaner cleaner = PdfBoxInternalCleaner.create(); + private final static PDFBoxInternalCleaner cleaner = PDFBoxInternalCleaner.create(); private final static AtomicInteger cleanups = new AtomicInteger(0); private final static AtomicInteger created = new AtomicInteger(0); diff --git a/pdfbox/src/main/java/org/apache/pdfbox/io/ScratchFileBuffer.java b/pdfbox/src/main/java/org/apache/pdfbox/io/ScratchFileBuffer.java index f04f95dfa7d..37ad4ba0bb6 100644 --- a/pdfbox/src/main/java/org/apache/pdfbox/io/ScratchFileBuffer.java +++ b/pdfbox/src/main/java/org/apache/pdfbox/io/ScratchFileBuffer.java @@ -16,7 +16,7 @@ */ package org.apache.pdfbox.io; -import org.apache.fontbox.util.PdfBoxInternalCleaner; +import org.apache.fontbox.util.PDFBoxInternalCleaner; import java.io.EOFException; import java.io.IOException; @@ -62,11 +62,11 @@ class ScratchFileBuffer implements RandomAccess /** number of pages held by this buffer */ private int pageCount = 0; - private final static PdfBoxInternalCleaner CLEANER = PdfBoxInternalCleaner.create(); - private final PdfBoxInternalCleaner.Cleanable cleanable; + private final static PDFBoxInternalCleaner CLEANER = PDFBoxInternalCleaner.create(); + private final PDFBoxInternalCleaner.Cleanable cleanable; private final ScratchFileBufferCleaner scratchFileBufferCleaner; - private static class ScratchFileBufferCleaner implements PdfBoxInternalCleaner.CleaningRunnable + private static class ScratchFileBufferCleaner implements PDFBoxInternalCleaner.CleaningRunnable { private final ScratchFile pageHandler; private int[] pageIndexes;