Phil,
Point 6, "The worst thing you can do with InterruptedException is swallow it --
catch it and neither rethrow it nor reassert the thread's interrupted status."
-Brian Goetz
https://www.ibm.com/developerworks/library/j-jtp05236/index.html
The swallowing of interrupts interferes with implementing cooperative
cancelable tasks. For modal dialogs like the JOptionPane, interrupting the
EDT from the application thread can be used to cancel modal blocking. The
JOptionPane for INFO, ERROR, WARNING icons shown are provided by the ImageIcon
class which means the ImageIcon can if the timing is just right swallow the
interrupt that is important for the JOptionPane to see as a signal not wait.
Jason
________________________________________
From: Philip Race <philip.r...@oracle.com>
Sent: Sunday, March 8, 2020 5:56 PM
To: Volodin, Vladislav
Cc: Jason Mehrens; swing-dev@openjdk.java.net
Subject: Re: <Swing Dev> Remove System.out.println from ImageIcon.loadImage
1) That bug is still closed. Did you want it re-opened ? I've just done so.
BTW I don't know why, but it was closed less than 3 months ago
by an engineer from the hotspot team (!?) I have NO idea why
since there is no comment.
2) If you are starting a review thread for a bug, the email subject line
should have the bug id and its synopsis thus :
RFR: 6421373: ImageIcon does not reassert interrupt status.
Although depending on what we end up doing the synopsis may
need to change (see 6 + 7 below).
3) The test has very long lines. Please curtail them to 80 chars.
4) What Christoph said.
5) I know where it was coming from, but I wish the docs said
"Returns when the image is loaded, or loading has failed.
Call getImageLoadStatus()" to check this."
Also a whole bunch of other questions arise like what
should happen if you call setImage() twice for different images.
If the first image succeeded, but the second failed, then what ?
6) I am not completely sold on reasserting the interrupted status
since it is a change of behaviour and you will need an approved CSR
to push this. Not sure if you need to update the javadoc as well ...
7) Now .. if all you really wanted was to get rid of the print statement
this is a lot of effort with some compatibility impact.
-phil.
On 2/22/20, 2:38 AM, Volodin, Vladislav wrote:
Hello Jason and everyone else,
here is my webrev that my colleague published
http://cr.openjdk.java.net/~clanger/webrevs/6421373.0/<http://cr.openjdk.java.net/%7Eclanger/webrevs/6421373.0/>
Can you please review it and let me know if I should change anything? Currently
I am OOO, but I will do the changes in March.
Kind regards,
Vlad
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Jason Mehrens
<jason_mehr...@hotmail.com><mailto:jason_mehr...@hotmail.com>
Sent: Tuesday, February 11, 2020 9:13:41 PM
To: Volodin, Vladislav
<vladislav.volo...@sap.com><mailto:vladislav.volo...@sap.com>
Cc: swing-dev@openjdk.java.net<mailto:swing-dev@openjdk.java.net>
<swing-dev@openjdk.java.net><mailto:swing-dev@openjdk.java.net>
Subject: Re: <Swing Dev> Remove System.out.println from ImageIcon.loadImage
Vlad,
Yea that looks good.
Jason
________________________________________
From: Volodin, Vladislav
<vladislav.volo...@sap.com><mailto:vladislav.volo...@sap.com>
Sent: Wednesday, February 5, 2020 9:59 AM
To: Jason Mehrens
Cc: swing-dev@openjdk.java.net<mailto:swing-dev@openjdk.java.net>
Subject: 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_mehr...@hotmail.com><mailto:jason_mehr...@hotmail.com>
Sent: Mittwoch, 29. Januar 2020 17:55
To: Volodin, Vladislav
<vladislav.volo...@sap.com><mailto:vladislav.volo...@sap.com>
Cc: swing-dev@openjdk.java.net<mailto:swing-dev@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.volo...@sap.com><mailto:vladislav.volo...@sap.com>
Sent: Wednesday, January 29, 2020 9:52 AM
To: Jason Mehrens
Cc: swing-dev@openjdk.java.net<mailto:swing-dev@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_mehr...@hotmail.com><mailto:jason_mehr...@hotmail.com>
Sent: Mittwoch, 29. Januar 2020 16:35
To: Volodin, Vladislav
<vladislav.volo...@sap.com><mailto:vladislav.volo...@sap.com>
Cc: swing-dev@openjdk.java.net<mailto:swing-dev@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-boun...@openjdk.java.net><mailto:swing-dev-boun...@openjdk.java.net> on behalf
of Jason Mehrens <jason_mehr...@hotmail.com><mailto:jason_mehr...@hotmail.com>
Sent: Wednesday, January 29, 2020 9:33 AM
To: Volodin, Vladislav
Cc: swing-dev@openjdk.java.net<mailto: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><mailto:vladislav.volo...@sap.com>
Sent: Tuesday, January 28, 2020 4:02 PM
To: Jason Mehrens
Cc: swing-dev@openjdk.java.net<mailto: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><mailto: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><mailto:vladislav.volo...@sap.com>
Sent: Tuesday, January 28, 2020 2:54 PM
To: Jason Mehrens
Cc: swing-dev@openjdk.java.net<mailto: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><mailto: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><mailto:swing-dev-boun...@openjdk.java.net> on behalf
of Volodin, Vladislav <vladislav.volo...@sap.com><mailto:vladislav.volo...@sap.com>
Sent: Monday, January 27, 2020 11:11 AM
To: Sergey Bylokhov
Cc: swing-dev@openjdk.java.net<mailto: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><mailto:sergey.bylok...@oracle.com>
Sent: Dienstag, 21. Januar 2020 22:26
To: Volodin, Vladislav
<vladislav.volo...@sap.com><mailto:vladislav.volo...@sap.com>
Cc: Jason Mehrens <jason_mehr...@hotmail.com><mailto:jason_mehr...@hotmail.com>;
swing-dev@openjdk.java.net<mailto: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><mailto: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><mailto:sergey.bylok...@oracle.com>
Sent: Sunday, January 19, 2020 9:31 PM
To: Volodin, Vladislav; Jason Mehrens
Cc: swing-dev@openjdk.java.net<mailto: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}