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-boun...@openjdk.java.net> On Behalf Of > Volodin, Vladislav > Sent: Mittwoch, 5. Februar 2020 16:59 > To: Jason Mehrens <jason_mehr...@hotmail.com> > Cc: swing-dev@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_mehr...@hotmail.com> > Sent: Mittwoch, 29. Januar 2020 17:55 > To: Volodin, Vladislav <vladislav.volo...@sap.com> > Cc: 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> > Sent: Wednesday, January 29, 2020 9:52 AM > To: Jason Mehrens > Cc: 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> > Sent: Mittwoch, 29. Januar 2020 16:35 > To: Volodin, Vladislav <vladislav.volo...@sap.com> > Cc: 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> 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- > d...@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}