<Swing Dev> Remove System.out.println from ImageIcon.loadImage
Volodin, Vladislav
vladislav.volodin at sap.com
Wed Jan 29 15:52:12 UTC 2020
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