Hmm ... I guess OpenPrinter doesn't fail often.

More of a concern is that I don't see where we (before or after this change) call ClosePrinter
in the case of success.

We certainly don't want to close it here because acc to
*https://docs.microsoft.com/en-us/windows/win32/printdocs/findfirstprinterchangenotification*
Callers of FindFirstPrinterChangeNotification must ensure that the printer handle passed into FindFirstPrinterChangeNotification remains valid until FindClosePrinterChangeNotification is called. If the printer handle is closed before the printer change notification handle, further notification
will fail to be delivered.


Are we leaking the handle ?  Looks that way to me ..

-phil



On 3/31/20, 3:21 PM, Sergey Bylokhov wrote:
Hi, Prasanta.

On 3/30/20 7:46 am, Prasanta Sadhukhan wrote:
I guess it is better if you directly pass NULL to OpenPrinter, rather than create an unnecessary variable.

That was done only for documentation purposes.

Also, I think it will be good if we do a ClosePrinter() if FindFirstPrinterChangeNotification() fails.

Nice catch! webrev is updated:
http://cr.openjdk.java.net/~serb/8241829/webrev.01


Regards
Prasanta
On 30-Mar-20 7:13 PM, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk/client.

Bug: https://bugs.openjdk.java.net/browse/JDK-8241829
Fix: http://cr.openjdk.java.net/~serb/8241829/webrev.00

In PrintServiceLookupProvider.java we always pass NULL to
the notifyFirstPrinterChange as a name of the printer, so
we can remove the code which expects a non-null value.



Reply via email to