<Swing Dev> Remove System.out.println from ImageIcon.loadImage
Langer, Christoph
christoph.langer at sap.com
Wed Feb 5 22:49:59 UTC 2020
Hi Vlad,
I can help you generate a webrev and upload it to cr.o.j.n. Please ping me tomorrow.
Another question to all: Shall I reopen https://bugs.openjdk.java.net/browse/JDK-6421373 for this?
Thanks
Christoph
> -----Original Message-----
> From: swing-dev <swing-dev-bounces at openjdk.java.net> On Behalf Of
> Volodin, Vladislav
> Sent: Mittwoch, 5. Februar 2020 16:59
> To: Jason Mehrens <jason_mehrens at hotmail.com>
> Cc: swing-dev at openjdk.java.net
> Subject: [CAUTION] Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
>
> Hi Jason,
>
> thank you for your advice. I have changed my code, now it simulates the
> behavior of the interrupted thread. Can you please check my patch?
> I don't have the "bug" ticket, so in my test case "@bug JDK-123456" should
> be adjusted.
>
> I will appreciate if you and somebody else can review my patch and submit it
> to JDK.
>
> Thanks,
> Vlad
>
> -----Original Message-----
> From: Jason Mehrens <jason_mehrens at hotmail.com>
> Sent: Mittwoch, 29. Januar 2020 17:55
> To: Volodin, Vladislav <vladislav.volodin at sap.com>
> Cc: swing-dev at openjdk.java.net
> Subject: Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
>
> 1. Agreed.
> 2. I was just pulling from the jdk8 source (because I'm lazy) to express the
> idea. Feel free to adjust.
> 3. Reasserting in the finally ensure we are not forcefully setting the
> interrupted status on the current thread and calling 'statusID' and
> 'removeImage'. It also ensures that the interrupt is set even if an
> unexpected exception is thrown. Reasserting at the end of 'e1' and never in
> 'e2' should work too.
>
> The main issue that I'm trying to convey to you is that your test is incomplete
> in that it does check that the interrupt was swallowed. Swallowing interrupts
> is bad practice. Reasserting in e2 only means that we swallow interrupts
> from e1.
>
> Change your test to this and retest:
> ===
> public static void main(String[] args) throws Exception {
> Toolkit ignoreToolkit = Toolkit.getDefaultToolkit();
>
> Thread.currentThread().interrupt();
> ImageIcon iconInterrupt = new ImageIcon(EMPTY_GIF);
> if (iconInterrupt.getImageLoadStatus() != MediaTracker.COMPLETE) {
> throw new RuntimeException("Couldn't load GIF from bytes after
> interruption");
> }
>
> if (!Thread.currentThread().isInterrupted()) {
> throw new RuntimeException("Interrupt was swallowed");
> }
> }
> }
> ===
> ________________________________________
> From: Volodin, Vladislav <vladislav.volodin at sap.com>
> Sent: Wednesday, January 29, 2020 9:52 AM
> To: Jason Mehrens
> Cc: swing-dev at openjdk.java.net
> Subject: RE: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
>
> Hi Jason,
>
> I have few questions:
>
> 1. The second assignment is probably redundant:
>
> } catch (InterruptedException e1) {
> wasInterrupted = true; // line #2, see comment below
> try {
> mTracker.waitForID(id, 0);
> } catch (InterruptedException e2) {
> loadStatus = MediaTracker.ABORTED;
> wasInterrupted = true; // <-- this is redundant, because of the line #2
> }
> } finally {
>
> 2. I think the first call of "addImage" is not necessary:
>
> boolean wasInterrupted = false;
> mTracker.addImage(image, id); // maybe I should remove this, because of
> another comment down below.
> try {
> loadStatus = 0;
> mTracker.addImage(image, id); // because of that
>
> May I also ask you a question?
> What is the purpose of interrupting the current thread in the finally block,
> instead of doing it in the second catch block (where e2 is created)? I assume
> that since we were able to handle the exception properly, and plus the
> entire block is in the synchronization area, in theory we have only one
> importer at the time.
>
> Kind regards,
> Vlad
>
> -----Original Message-----
> From: Jason Mehrens <jason_mehrens at hotmail.com>
> Sent: Mittwoch, 29. Januar 2020 16:35
> To: Volodin, Vladislav <vladislav.volodin at sap.com>
> Cc: swing-dev at openjdk.java.net
> Subject: Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
>
> Bug in that last version:
> Thread.currentThread().isInterrupted(); ->
> Thread.currentThread().interrupt();
> ===
> protected void loadImage(Image image) {
> MediaTracker mTracker = getTracker();
> synchronized (mTracker) {
> int id = getNextID();
>
> boolean wasInterrupted = false;
> mTracker.addImage(image, id);
> try {
> loadStatus = 0;
> mTracker.addImage(image, id);
> mTracker.waitForID(id, 0);
> } catch (InterruptedException e1) {
> wasInterrupted = true;
> try {
> mTracker.waitForID(id, 0);
> } catch (InterruptedException e2) {
> loadStatus = MediaTracker.ABORTED;
> wasInterrupted = true;
> }
> } finally {
> if (loadStatus == 0) {
> loadStatus = mTracker.statusID(id, false);
> }
> mTracker.removeImage(image, id);
> if (wasInterrupted) {
> Thread.currentThread().interrupt();
> }
> }
> }
> }
> ===
>
> ________________________________________
> From: swing-dev <swing-dev-bounces at openjdk.java.net> on behalf of
> Jason Mehrens <jason_mehrens at hotmail.com>
> Sent: Wednesday, January 29, 2020 9:33 AM
> To: Volodin, Vladislav
> Cc: swing-dev at openjdk.java.net
> Subject: Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
>
> A shorter version is just to reassert at the end of catch e1. It is safer to just
> reassert as the last statement in the finally.
>
> ===
> protected void loadImage(Image image) {
> MediaTracker mTracker = getTracker();
> synchronized (mTracker) {
> int id = getNextID();
>
> boolean wasInterrupted = false;
> mTracker.addImage(image, id);
> try {
> loadStatus = 0;
> mTracker.addImage(image, id);
> mTracker.waitForID(id, 0);
> } catch (InterruptedException e1) {
> wasInterrupted = true;
> try {
> mTracker.waitForID(id, 0);
> } catch (InterruptedException e2) {
> loadStatus = MediaTracker.ABORTED;
> }
> } finally {
> if (loadStatus == 0) {
> loadStatus = mTracker.statusID(id, false);
> }
> mTracker.removeImage(image, id);
> if (wasInterrupted) {
> Thread.currentThread().isInterrupted();
> }
> }
> }
> }
> ===
>
> Jason
> ________________________________________
> From: Volodin, Vladislav <vladislav.volodin at sap.com>
> Sent: Tuesday, January 28, 2020 4:02 PM
> To: Jason Mehrens
> Cc: swing-dev at openjdk.java.net
> Subject: Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
>
> This is a valid point, because I wasn’t sure that when the thread is
> interrupted, I handled the first exception, and my thought was that all other
> ‘wait’ calls (maybe in other threads) should get the same exception as well.
>
> And since I handled this exceptional case, I am restoring the interrupted
> status in case if I don’t know how to handle this exception the second time.
>
> } catch (InterruptedException e1) {
> try {
> mTracker.waitForID(id, 0);
> } catch (InterruptedException e2) {
> loadStatus = MediaTracker.ABORTED;
> Thread.currentThread().interrupt();
> }
>
> If I restore the interrupted status BEFORE waitForID call, then this thread will
> immoderately fail. Should I restore it AFTER, e.g.
>
> } catch (InterruptedException e1) {
> try {
> mTracker.waitForID(id, 0);
> Thread.currentThread().interrupt(); // <— Here?
> } catch (InterruptedException e2) {
> loadStatus = MediaTracker.ABORTED;
> Thread.currentThread().interrupt();
> }
>
> Thanks,
> Vlad
>
> Sent from myPad
>
> > On 28. Jan 2020, at 22:27, Jason Mehrens <jason_mehrens at hotmail.com>
> wrote:
> >
> > I see. Well I would have to do some more digging and testing to make
> myself more knowledgeable on MediaTracker.
> >
> > Then the only bug in your current code is that you are not reasserting
> interrupted status of the current thread in the case e1 is raised and e2 is not.
> It is usually best to reassert the interrupted state at the end of the method.
> >
> > Jason
> >
> > ________________________________________
> > From: Volodin, Vladislav <vladislav.volodin at sap.com>
> > Sent: Tuesday, January 28, 2020 2:54 PM
> > To: Jason Mehrens
> > Cc: swing-dev at openjdk.java.net
> > Subject: Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
> >
> > Hi Jason,
> >
> > I am not sure about the loop, because I don’t know if we can trust results
> from mTracker.statusID at the moment when an exceptional case occurs. For
> example, I was experimenting with the code and found out that the
> MediaTracker can spawn a thread to load the image, or might use the current
> thread, and the ABORTED state is never returned (I don’t even know how to
> raise this state). That is why I don’t even know if your loop will ever stop.
> >
> > In my solution, I give the second chance only, and in case if the execution
> was interrupted the second time, I explicitly assign the ABORTED state to
> loadStatus. If the user wants to load this image once again, he can create
> ImageIcon again (or even do this in a loop), or reinitialize it with a single
> method (I don’t remember its name).
> >
> > What do you think?
> >
> > Kind regards,
> > Vlad
> >
> > Sent from myPad
> >
> >> On 28. Jan 2020, at 21:34, Jason Mehrens <jason_mehrens at hotmail.com>
> wrote:
> >>
> >> Vlad,
> >>
> >> I assume you would want to wait in a loop and reassert the interrupt at
> the end.
> >>
> >> ===
> >> protected void loadImage(Image image) {
> >> MediaTracker mTracker = getTracker();
> >> synchronized(mTracker) {
> >> int id = getNextID();
> >>
> >> mTracker.addImage(image, id);
> >> boolean wasInterrupted = false;
> >> try {
> >> do {
> >> try {
> >> mTracker.waitForID(id, 0);
> >> } catch (InterruptedException e) {
> >> wasInterrupted = true;
> >> }
> >> loadStatus = mTracker.statusID(id, false);
> >> } while (loadStatus == MediaTracker.LOADING);
> >> mTracker.removeImage(image, id);
> >>
> >> width = image.getWidth(imageObserver);
> >> height = image.getHeight(imageObserver);
> >> } finally {
> >> if (wasInterrupted) {
> >> Thread.currentThread().interrupt();
> >> }
> >> }
> >> }
> >> }
> >> ===
> >>
> >> Jason
> >> ________________________________________
> >> From: swing-dev <swing-dev-bounces at openjdk.java.net> on behalf of
> Volodin, Vladislav <vladislav.volodin at sap.com>
> >> Sent: Monday, January 27, 2020 11:11 AM
> >> To: Sergey Bylokhov
> >> Cc: swing-dev at openjdk.java.net
> >> Subject: Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
> >>
> >> Hello Sergey, and others,
> >>
> >> here is the patch (made with "git diff") in the attachment. I have read that
> sometimes the attachments are lost. So here is my version with few
> comments regarding my code. I decided to use your approach with a small
> improvement. There is an excerpt of my patch.
> >>
> >> The idea is pretty simple:
> >> 1. I create an empty gif 1x1;
> >> 2. Initialize DefaultToolkit (I plan to interrupt the main thread, and if I do
> not initialize the toolkit in advance, my test will fail at the beginning
> >> 3. Interrupt the main thread
> >> 4. Try to load the icon:
> >>
> >> import javax.swing.*;
> >> import java.awt.*;
> >>
> >> public class imageIconInterrupted {
> >> static byte[] EMPTY_GIF = new byte[]{
> >> (byte) 0x47, (byte) 0x49, (byte) 0x46, (byte) 0x38, (byte) 0x39, (byte)
> 0x61, (byte) 0x01, (byte) 0x00,
> >> (byte) 0x01, (byte) 0x00, (byte) 0x00, (byte) 0x00, (byte) 0x00, (byte)
> 0x21, (byte) 0xF9, (byte) 0x04,
> >> (byte) 0x01, (byte) 0x00, (byte) 0x00, (byte) 0x00, (byte) 0x00, (byte)
> 0x2C, (byte) 0x00, (byte) 0x00,
> >> (byte) 0x00, (byte) 0x00, (byte) 0x01, (byte) 0x00, (byte) 0x01, (byte)
> 0x00, (byte) 0x00, (byte) 0x02
> >> };
> >>
> >> public static void main(String[] args) throws Exception {
> >> Toolkit ignoreToolkit = Toolkit.getDefaultToolkit();
> >>
> >> Thread.currentThread().interrupt();
> >> ImageIcon iconInterrupt = new ImageIcon(EMPTY_GIF);
> >> if (iconInterrupt.getImageLoadStatus() != MediaTracker.COMPLETE) {
> >> throw new RuntimeException("Couldn't load GIF from bytes after
> interruption");
> >> }
> >> }
> >> }
> >>
> >> The code of loadImage I have changed as follows:
> >> 1. I clear loadStatus for "finally" part;
> >> 2. Check the interruption according to your comments;
> >> 3. Change the status to ABORTED, if the interruption happened again (this
> part I cannot test, because I cannot properly interrupt the thread whenever I
> want. Multithreading is quite fragile there :( )
> >> 4. update the status "interrupted" again;
> >> 5. and then - finally block.
> >>
> >> protected void loadImage(Image image) {
> >> ...
> >> try {
> >> loadStatus = 0;
> >> mTracker.addImage(image, id);
> >> mTracker.waitForID(id, 0);
> >> } catch (InterruptedException e1) {
> >> try {
> >> mTracker.waitForID(id, 0);
> >> } catch (InterruptedException e2) {
> >> loadStatus = MediaTracker.ABORTED;
> >> Thread.currentThread().interrupt();
> >> }
> >> } finally {
> >> if (loadStatus == 0) {
> >> loadStatus = mTracker.statusID(id, false);
> >> }
> >> mTracker.removeImage(image, id);
> >> }
> >>
> >> P.S. if you think that my patch sounds fine, I will find a sponsor for the bug
> report and the patch preparation, so you can review it later.
> >> After the second attempt, my EMPTY_GIF was loaded successfully. The
> ABORTED part it seems that I don't know if it has ever worked. My patch (in
> the attachment) checks also the state ERROR for the URL that doesn't exist.
> Something like:
> >>
> >> ImageIcon iconNotExist = new ImageIcon(new
> URL("http://doesnt.exist.anywhere/1.gif")); // I am not sure if I spelled this
> URL grammatically correct
> >> if (iconNotExist.getImageLoadStatus() != MediaTracker.ERRORED) {
> >> throw new RuntimeException("Got unexpected status from non-
> existing GIF");
> >> }
> >>
> >> Thanks to everyone.
> >>
> >> Kind regards,
> >> Vlad
> >>
> >> -----Original Message-----
> >> From: Sergey Bylokhov <Sergey.Bylokhov at oracle.com>
> >> Sent: Dienstag, 21. Januar 2020 22:26
> >> To: Volodin, Vladislav <vladislav.volodin at sap.com>
> >> Cc: Jason Mehrens <jason_mehrens at hotmail.com>; swing-
> dev at openjdk.java.net
> >> Subject: Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
> >>
> >>>> On 1/21/20 1:14 pm, Volodin, Vladislav wrote:
> >>> Hi all,
> >>>
> >>> If I am not mistaken, this method is called from the constructor and
> other methods. How long should we try loading the icon using the
> unmanaged (by any thread pool, but I am not sure about this statement)
> thread?
> >>>
> >>> We don’t even have the flag “interrupted”. So technically this icon has to
> be loaded.
> >>
> >> I think it is necessary to save the "interrupted" state in the catch
> >> block and try to call waitForID() again. It will be necessary to set
> >> "interrupted" flag for the Thread after that(when the waitForID will
> >> return without exception).
> >>
> >>
> >>> Sent from myFone
> >>>
> >>>>> On 21. Jan 2020, at 21:55, Sergey Bylokhov
> <Sergey.Bylokhov at oracle.com> wrote:
> >>>>
> >>>> On 1/21/20 12:26 pm, Jason Mehrens wrote:
> >>>>> +1 for Sergey suggestion.
> >>>>
> >>>> Or probably we need to try to load the image again because of the spec
> of the method state this:
> >>>> "Loads the image, returning only when the image is loaded."
> >>>>
> >>>>> ________________________________________
> >>>>> From: Sergey Bylokhov <Sergey.Bylokhov at oracle.com>
> >>>>> Sent: Sunday, January 19, 2020 9:31 PM
> >>>>> To: Volodin, Vladislav; Jason Mehrens
> >>>>> Cc: swing-dev at openjdk.java.net
> >>>>> Subject: Re: <Swing Dev> Remove System.out.println from
> ImageIcon.loadImage
> >>>>> I guess there are no objections, probably the best way to fix
> >>>>> it is to drop the System.out.println and set interrupted flag.
> >>>>> On 1/16/20 7:05 am, Volodin, Vladislav wrote:
> >>>>>> If people in this distribution list agree, I can start working on a simple
> fix and either remove the logging (System.out.println), or replace it with
> PlatformLogger. I guess the second solution sounds better, but I don't know
> what is the better way to write a test-case for it.
> >>>>> --
> >>>>> Best regards, Sergey.
> >>>>
> >>>>
> >>>> --
> >>>> Best regards, Sergey.
> >>
> >>
> >> --
> >> Best regards, Sergey.
More information about the swing-dev
mailing list