Consider defining additional methods to load fonts into `ITextFontResolver`

[rzam]

Due to the way OpenPDF accesses the filesystem (see this semi-related issue, specifically this, and also this), adding any font via ITextFontResolver::addFont on Windows machines causes the creation of a permanent filesystem handle that persists until the process is killed; this seems to be repeated any time any font is added (eg. 10 renderer instances with the same font added = 10 handles).

This could be somewhat alleviated by exposing an addFont overload that accepts a BaseFont object directly; that way the user could be in control of how the font is loaded (eg. via filesystem but using a singleton, via byte array, etc.)

[asolntsev]

@rzam Do you mean adding method ITextFontResolver.addFont(BaseFont font)? I agree, sounds reasonable.

But I see one nuance: I see ITextFontResolver already has two different pieces that handle BaseFont instances:

1. one for .otf, .ttf and .ttc files (lines 213-235),
2. and the other for .afm and .pfm files (lines 240-262).

which one is correct? I mean, which one should use the new method addFont(BaseFont font)?

[rzam]

From what I can see the main issue would be that there would be no way to invoke TrueTypeUtil::populateDescription without access to the original font data, which BaseFont/TrueTypeFont does not provide (as far as I can tell).

This could be fixed by providing methods to load data via byte arrays (instead of BaseFont objects) and then using the raw font data to do the stuff that was previously done by reading the font from filesystem.

Here's a proof of concept that seems to work (I can make a pull request if you'd like); the only font type I could not manage to load was PFM+PFB (mostly because I have no experience with that type of font and I have no idea how to generate one).

[asolntsev]

@rzam I've submitted PR https://github.com/flyingsaucerproject/flyingsaucer/pull/386 where I exposed part of existing addFont method to a separate public method:

public void addFont(BaseFont font, String path, @Nullable String fontFamilyNameOverride) { ... }

Probably it already solves the problem?

[rzam]

@asolntsev Thanks for the quick change! I've done a quick test and the fonts seems to be loaded correctly, but unfortunately the permanent handle is still being created on Windows machines.

After some digging around is seems to be due to the use of OpenPDF's RandomAccessFileOrArray (which internally uses MappedRandomAccessFile which creates the permanent handles) inside TrueTypeUtil::populateDescription. Maybe make the path argument optional and forgo using populateDescription when it's not provided?

[asolntsev]

@rzam I don't see any problems with RandomAccessFileOrArray in TrueTypeUtils. In both places, RandomAccessFileOrArray is created inside "try-with-resources" block - which means that it is guaranteed to be closed:

  try (RandomAccessFileOrArray rf = new RandomAccessFileOrArray(...)) {
         ...
  }  // rf.close() will be always called after this "try" block.

May the problem be in class RandomAccessFileOrArray? I see its constructor doesn't always close all InputStream instances...

[rzam]

@asolntsev It's not really a problem with OpenPDF or RandomAccessFileOrArray per-se, it's that Windows simply doesn't play nice with memory-mapping and refuses to release memory-mapped files, even if the associated channel/file/stream is closed (it's been a 20+ year long issue). As soon as FileChannel::map is invoked, the eternal handle is created.

As a simple example, this test fails on Windows but works on Linux (and probably macOS too):

// Create test file
File testFile = new File("test.txt");
Files.writeString(testFile.toPath(), "Test");

// Open file, map it, do nothing and close everything
try (FileInputStream inputStream = new FileInputStream(testFile)) {
    try (FileChannel channel = inputStream.getChannel()) {
        channel.map(FileChannel.MapMode.READ_ONLY, 0, channel.size());
    }
}

// Try to delete the file
System.out.println("File still in use: " + !testFile.delete()); // will output true on Windows

[asolntsev]

@rzam Ok, but what can we do on FlyingSaucer side?

[rzam]

@asolntsev The only way to "fix" it is by not accessing the filesystem via memory-mapping at all (which is why I initially suggested making path optional, to skip accessing the filesystem).

However, I'm noticing now that RandomAccessFileOrArray has an overloaded constructor that allows turning off memory-mapping (plainRandomAccess = true), could you maybe use that inside populateDescription? Font files are usually limited in size so it shouldn't generate too much overhead.

[asolntsev]

@rzam Ok, let's try. :)

Please check the latest 9.9.3-SNAPSHOT (https://oss.sonatype.org/content/repositories/snapshots/org/xhtmlrenderer/flying-saucer-pdf/9.9.3-SNAPSHOT/flying-saucer-pdf-9.9.3-20240916.072722-1.jar)

[rzam]

@asolntsev Confirmed to be working.

Cheers!

[asolntsev]

@rzam I've released FS 9.9.3