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..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,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..8673d10cb46 --- /dev/null +++ b/fontbox/src/main/java/org/apache/fontbox/util/PDFBoxInternalCleaner.java @@ -0,0 +1,202 @@ +package org.apache.fontbox.util; + +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, 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. + */ +public class PDFBoxInternalCleaner +{ + + /** + * @see java.lang.ref.Cleaner.Cleanable + */ + public interface Cleanable + { + void clean(); + } + + 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 using finalizers. + */ + private static class CleanerImplJDK8 implements CleanerImpl + { + + static class ObjectFinalizer implements Cleanable + { + final String simpleClassName; + final CleaningRunnable action; + boolean didRun; + + 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(); + } + + @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(obj.getClass().getSimpleName(), 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(); + } + } + + /** + * 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(); + } + +} 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..f6ce45a741b --- /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()); + } + +} 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..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,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 {