release graphics if an exception will be thrown in PDFPrintable.print method

[valerybokov]

No description provided.

[THausherr]

I thought this looked good but I asked copilot because I can use it for free. Your PR disposes the wrong object if the exception is too early. Here's the important parts (AI is too verbose):

====
At the end of the try-block, graphics2D will usually still refer to the image graphics (good)… unless an exception happens before graphics2D = image.createGraphics() is executed, in which case graphics2D still refers to the printer graphics and the finally would dispose a graphics object it probably does not own.

Change the code to dispose only the image graphics created by image.createGraphics():

introduce Graphics2D imageGraphics = null;
set it right after creation
finally { if (imageGraphics != null) imageGraphics.dispose(); }

=====

Here's the full review:

=====

PR #440 is conceptually aiming at the right thing (avoid leaking/holding onto graphics resources when print() exits via an exception), but the implementation as shown in the patch has a bug and likely doesn’t do what the author intended.
What the PR changes

It hoists Graphics2D graphics2D and Graphics2D printerGraphics out of the try block and adds a finally intended to dispose graphics if something goes wrong.

Patch excerpt (from the PR “files” view):
Diff

•    Graphics2D printerGraphics = null;
  

•    Graphics2D graphics2D = null;
  

...

•        Graphics2D graphics2D = (Graphics2D)graphics;
  

•        graphics2D = (Graphics2D)graphics;
  

...

•        Graphics2D printerGraphics = null;
  

...

•            graphics2D.dispose();
  

...

•    finally
  

•    {
  

•        if (printerGraphics != null)
  

•        {
  

•            graphics2D.dispose();
  

•        }
  

•    }
  

Main issue: finally disposes the wrong object

In the rasterization branch (existing code), the flow is:

printerGraphics = graphics2D; (this is the original printer-provided graphics)
graphics2D = image.createGraphics(); (this is the BufferedImage graphics that must be disposed)

So if rasterization is enabled, the thing you must dispose is the image graphics, i.e. the graphics2D after reassignment.

But in the PR’s finally, the condition is if (printerGraphics != null) (meaning rasterization happened), and then it calls graphics2D.dispose().

At the end of the try-block, graphics2D will usually still refer to the image graphics (good)… unless an exception happens before graphics2D = image.createGraphics() is executed, in which case graphics2D still refers to the printer graphics and the finally would dispose a graphics object it probably does not own.

Also: the PR removed the existing graphics2D.dispose() in the normal (non-exception) rasterization flow, but the finally only disposes when printerGraphics != null; so disposal would still happen on success. That part is fine if it’s always the correct graphics instance—but as explained, it’s not guaranteed.
Secondary issue: likely typo / intent mismatch

If the PR title is “release graphics if an exception will be thrown…”, the safe pattern is usually:

Never dispose the incoming Graphics/Graphics2D graphics argument (caller owns it)
Dispose only graphics objects you created (the image.createGraphics() result)

That suggests the PR should track something like Graphics2D imageGraphics = null; and if (imageGraphics != null) imageGraphics.dispose(); in finally.
Recommendation (what I would request changes on)

Change the code to dispose only the image graphics created by image.createGraphics():
    introduce Graphics2D imageGraphics = null;
    set it right after creation
    finally { if (imageGraphics != null) imageGraphics.dispose(); }
Keep printerGraphics for drawing the final image, but do not dispose it.
Add a small regression test if the project has printing tests (or at least a unit-level test around resource disposal if feasible). If not feasible, at least add a comment clarifying ownership: “Do not dispose the printer graphics; only dispose graphics created for rasterization.”

Bottom line

Good idea, but as written, I would not approve: the finally block can dispose the wrong graphics object and violates typical ownership rules around Printable#print(Graphics, ...).

[valerybokov]

As I understand it, the main argument of Claude's is that the wrong graphics object can be disposed if an exception occurs before calling "image.createGraphics()." This is potentially possible. That is, the code is written in such a way that if another programmer changes it in the future, such an error could occur.

I checked my proposed code for such an error, if it is an IOException. You can comment out lines 202 and 318-321. Then you will see that the exception never occurs before the "image.createGraphics()" command. Therefore, disposing the wrong object is only possible if an entirely different exception (OOM?) is thrown.

I modified the code to eliminate the potential error of disposing the wrong Graphics2D object.

[valerybokov]

Here are some more things that look like bugs:

1. This is strange: drawRect uses the wrong Graphics object (line 305)

graphics.drawRect(0, 0, (int)cropBox.getWidth(), (int)cropBox.getHeight());

This calls graphics (the original parameter), not graphics2D. All the configuration just set on graphics2D (transform,
clip, scale, color, stroke) is ignored, so the page border is drawn at the wrong place with default color/stroke, and
the just-set transform is never used. Should be graphics2D.drawRect(...).

2. State leak on the printer Graphics2D. The method mutates graphics2D (translate at line 255, translate at line 264, setBackground, setTransform, setClip, scale, setColor, setStroke) but never saves/restores the original state. print() may be called multiple times for the same Graphics; the next page inherits the mangled transform/clip/stroke. Standard pattern is:

AffineTransform orig = graphics2D.getTransform();
 try { ... } finally { graphics2D.setTransform(orig); }

3. Potential zero/negative-size BufferedImage (lines 279–281)
   If imageableWidth * dpiScale / scale truncates to 0 (tiny imageable area, or scale very large), new BufferedImage(0,
   0, ...) throws IllegalArgumentException. Edge case, but not guarded.

[THausherr]

Thank you, these are very good observations. Please create 3 different tickets if you want, but AFTER this one is done, and not at the same time. The correction here looks good to me but lets see what copilot says.

[THausherr]

copilot (GPT 5.2) agrees and made a separate (unrelated) suggestion for clarity that I will commit separately.