8335231: [macos] Test java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java failed on macOS because the case didn't get the expected PrintAbortException #20027
Conversation
…el.java failed on macOS because the case didn't get the expected PrintAbortException
|
👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into |
|
@prsadhuk This change is no longer ready for integration - check the PR body for details. |
Webrevs
|
|
I am not able to reproduce in windows, and I guess neither the submitter who raised this issue as it is mentioned in JBS |
Yeah, Print-to-pdf works fine. Not sure about printer config issue. |
It seems it's timing related...For some network printers, it might take more time to setup..for example, the printer I tested Xerox Altalink it was taking time in |
| @@ -371,6 +371,8 @@ public SecondaryLoop run() { | |||
|
|
|||
| try { | |||
| printLoop(true, firstPage, lastPage); | |||
| } catch (PrinterAbortException pex) { | |||
| throw new PrinterAbortException(pex.getMessage()); | |||
There was a problem hiding this comment.
Why not re-throw the original exception ?
There was a problem hiding this comment.
Did you miss this question ?
There was a problem hiding this comment.
oh NM, this comments view doesn't show that it was fixed.
| } | ||
| }}, null); | ||
| } catch (java.lang.reflect.InvocationTargetException ite) {} | ||
| cancelDoc(); |
There was a problem hiding this comment.
The comments about deadlock refer to isCancelled() but is that not also why there was an invokeLater() ?
It is not at all clear to me. Perhaps you can explain how the deadlock would occur and why it is not a problem for calling cancelDoc() ?
There was a problem hiding this comment.
This comment was there from initial macosx port. Since RasterPrinterJob.isCancelled relies on synchronized block and since this cancelCheck method was called from native, it might have been suspected that it might block the native AppKit thread, so invokeLater was added.
But the problem was LWCToolkit.invokeLater is called with null argument,
which was not a problem in initial macosx port but JDK-8042087 got it modified in jdk9 to throw an exception if component argument is null, which is the case in CPrinterJob.m#cancelCheck
LWCToolkit.inokeAndWait is called on the Appkit thread, but if the component argument is null it uses EventQueue.invokeLater which would fail with an NPE in plugin mode. The method should be updated to throw an exception in case the provided component is null.
Coming to your question, I guess if there is a deadlock, calling cancelDoc directly will also face the same issue as it also relies on same synchronized(this) block but I could not find any deadlock issue with the printing for which this invokeLater safeguard was added...
So, Is the above comment " fail with an NPE in plugin mode" still applicable i.e do we still support plugin mode?
If not, we can restore invokeLater call in this code and use Toolkit.getToolkit if the component is null as it was previously to JDK-8042087
There was a problem hiding this comment.
I dont see any deadlock in my testing so I am using isCancelled .
How do you normally "cancel" ongoing printing in macosx? I tried clicking on "x" in "Print Centre" when the specific print job is shown in the printer spooler and I dont see any deadlock for long printing tests like PrintAllFonts etc..
There was a problem hiding this comment.
plugin mode is no longer a concern.
So it seems to me that the worry is that some other thread holds the lock on the PrinterJob, and
is waiting for a print loop to return from native code running on the AppKit thread, which then up-calls to here and then needs to synchronize on the PrinterJob.
Other cases that I see aren't from native so aren't a problem.
"Coming to your question, I guess if there is a deadlock, calling cancelDoc directly will also face the same"
Yes whereas In the existing code it is run on the AppKit thread to avoid it ...
It seems safest to restore the invokeLater.
There was a problem hiding this comment.
Ok..Restore invokeLater and handled/thrown PrinterAbortException in native..
| @@ -759,6 +761,7 @@ private boolean cancelCheck() { | |||
| // no-op, let the native side handle it | |||
| } | |||
| }}, null); | |||
| } catch (NullPointerException ex) { | |||
There was a problem hiding this comment.
I'm confused. I thought you were going to do something about the null ?
Unless you don't this code won't work.
Restoring using invokeLater isn't the same thing as restoring the bug.
Look at the code that you are calling - it won't run the Runnable if there's an NPE.
"new Component()" instead of null for example.
FWIW I do not understand why this code uses LWCToolkit.invokeLater()
Seems unnecessary.
Why don't it use EventQueue.invokeLater() directly ?
That'll solve a couple of problems - simpler code, no null check because no Component arg needed.
There was a problem hiding this comment.
Yes, actually I wanted to avoid changing the common LWCToolkit.invokeLater (as it needs to use Toolkit.getToolkit if component is null as I was not sure if it is ok to pass any adhoc component via new Component in CPrinterJob) which is also used in accessibility and InputMethod which would have affected and require testing in those areas
but yes, I didn't see having NPE will render invokeLater a no-op
Changed to use EventQueue.invokeLater..
|
I think we still have some things to look at here (1) The job still gets queued for printing - should it ? Ideally not if there's some thing we can do. Here is the NSException printed for (3) : 2024-08-02 13:34:54.360 java[50622:37988054] Java Exception Note it is not a Java Exception, it is an NSException and we log this from JNI_COCOA_EXIT after we catch it. So what's going on with (3)? - I can partly help explain that. This particular NSException happens because after the printLoop we need to make an up-call from "complete" But there's this PrinterAbortException already pending, which means we really should not be making the up-call without clearing it first, which is another issue (#4, or still part of #3 ?) After the upcall CHECK_EXCEPTION sees a pending Java Exception which it supposes is from a problem during the upcall and raises the NSException to "bail" up to the JNI_COCOA_EXIT where we catch it and log it as seen above. So how can we get that PrinterAbortException back up to Java without causing problems for subsequent upcalls? Don't forget about (1) - the job being queued. |
I guess you are testing via your standalone
I suspected it might be because of
I thought it is because of this line in I found one issue where I see that I modified the PR to do that which stops the NSException being thrown so that should solve (3) but (2) is still an enigma.. |
…d NSException log
|
I modified the PR to implement this which solves the (3) NSException log |
|
@prrace please take a look |
|
@prsadhuk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@prrace Waiting for your relook on this... |
When a printjob is cancelled midway,
PrinterAbortExceptionwas not thrown in macos. becausefirstly,
cancelCheckinvokesLWCToolkit.invokeLaterwith null as parameter causing it to fail with NPE andsecondly PrinterAbortException was consumed silently when
printLoopthrows any exceptionwhich is rectified to throw the PrinterAbortException when encountered..
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20027/head:pull/20027$ git checkout pull/20027Update a local copy of the PR:
$ git checkout pull/20027$ git pull https://git.openjdk.org/jdk.git pull/20027/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 20027View PR using the GUI difftool:
$ git pr show -t 20027Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20027.diff
Webrev
Link to Webrev Comment