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-boun...@openjdk.java.net> on behalf of Jason Mehrens
<jason_mehr...@hotmail.com>
Sent: Wednesday, January 29, 2020 9:33 AM
To: Volodin, Vladislav
Cc: swing-dev@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.volo...@sap.com>
Sent: Tuesday, January 28, 2020 4:02 PM
To: Jason Mehrens
Cc: swing-dev@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_mehr...@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.volo...@sap.com>
> Sent: Tuesday, January 28, 2020 2:54 PM
> To: Jason Mehrens
> Cc: swing-dev@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_mehr...@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-boun...@openjdk.java.net> on behalf of Volodin,
>> Vladislav <vladislav.volo...@sap.com>
>> Sent: Monday, January 27, 2020 11:11 AM
>> To: Sergey Bylokhov
>> Cc: swing-dev@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.bylok...@oracle.com>
>> Sent: Dienstag, 21. Januar 2020 22:26
>> To: Volodin, Vladislav <vladislav.volo...@sap.com>
>> Cc: Jason Mehrens <jason_mehr...@hotmail.com>; swing-dev@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.bylok...@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.bylok...@oracle.com>
>>>>> Sent: Sunday, January 19, 2020 9:31 PM
>>>>> To: Volodin, Vladislav; Jason Mehrens
>>>>> Cc: swing-dev@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.
![http://doesnt.exist.anywhere/1.gif"](http://doesnt.exist.anywhere/1.gif"){kind=link}