<div dir="ltr"><div>Hi Martin,</div><div><br></div><div>What an amazing effort !</div><div>I am very pleased that my (crazy) test triggered a very complicated case; I will try asap if it works with your new webrev.</div><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr"><div>Thanks for your feedback and sharing your test.</div><div><br></div><div>I've taken your suggestion of making the filter class final. In regards to having a single instance of the filter class, it would have been a good idea but now we have some additional requirements you'll see below.</div><div><br></div><div>I could reproduce your deadlock. This deadlock scenario is a bit more interesting than the one fixed in Webrev00.</div><div><br></div><div>At some point, we may reach the following state:</div><div><br></div><div>SequenceEvent.list: EV0-AppContext1, EV0-AppContext0, EV1-AppContext0</div><div>EventQueue0: EV0-AppContext0, EV1-AppContext0</div><div><br></div><div>EDT0 now takes EV0-AppContext0 out of EventQueue0. When dispatching this event, it notices that it's not the first one in SequenceEvent.list and proceeds to dispatch a new event from EventQueue0 while waiting. It gets EV1-AppContext0 and again, it cannot continue because EV0-AppContext1 is still the first one in SequenceEvent.list. EDT0 now waits for either a SentEvent (sent by EDT1) or for a new SequencedEvent in EventQueue0.</div><div><br></div><div>Let's say that EDT1 dispatches EV0-AppContext1 and sends a SentEvent to EDT0. EDT0 wakes up (enabled at Webrev00) and tries to dispatch EV1-AppContext0. However, EV1-AppContext0 is not the first one in SequenceEvent.list so it goes to sleep again (waiting for a SentEvent or a new SequencedEvent to come).</div><div><br></div><div>However, EDT0 is the only one that can unlock this by dispatching EV0-AppContext0. This is not possible because EV0-AppContext0-dispatch call is down the stack. There is a stack frame on top of it: the one for EV1-AppContext0-dispatch. It's an inversion of the order.</div><div><br></div><div>We don't want EDT0 to take a new SequencedEvent out of EventQueue0 (that is: creating a new dispatch frame on the call stack) if all events in SequenceEvent.list previous to the currently SequencedEvent event being dispatched are from a different AppContext.</div><div><br></div><div>My first approach to this was: check SequenceEvent.list before going to sleep and see if we need to filter SequencedEvent events or not. However, this does not work for 2 reasons:</div><div><br></div><div> 1) Events in SequencedEvent.list which are not being dispatched have a null AppContext. The only way of knowing if they belong to a specific AppContext is by iterating the EventeQueue queue and checking object identity. No APIs for this. On the other hand, assigning an AppContext when creating SequencedEvent events may break things (i.e.: creating events on a TG and posting them to a different one).</div><div><br></div><div> 2) We cannot loose SequencedEvent events. Of course.</div><div><br></div><div>What I propose to do is capturing out-of-order SequencedEvents events in the filter and releasing them once we dispatch the frame. In other words, try to dispatch only if the event that arrives is from a previous position in SequenceEvent.list (so we won't block).</div><div><br></div><div>In addition to these changes, I propose to dispatch any non-SequencedEvent event while waiting. There is no need to block other events, and the GUI becomes unresponsive under a stress case such as the test otherwise.</div><div><br></div><div>Removed an unnecessary "wakeup" on SequencedEvent.dispose function.</div><div><br></div><div>Webrev 01:</div><div><br></div><div> * <a href="http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.01/" target="_blank">http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.01/</a></div><div> * <a href="http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.01.zip" target="_blank">http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.01.zip</a></div></div></div></blockquote><div><br></div><div>I had a quick look to the patch. <br></div><div>I noticed that the list <span class="gmail-new">pendingEvents is not synchronized and I wonder if multiple EDT threads may be working on the same SequencedEvent:</span></div><div><span class="gmail-new">- </span><span class="gmail-new">currentSequencedEvent.pendingEvents.add(ev);</span><span class="gmail-new"></span></div><div><span class="gmail-new">- </span><span class="gmail-new">for(AWTEvent e : pendingEvents) { </span><span class="gmail-new">SunToolkit.postEvent(appContext, e); }</span></div><div><span class="gmail-new"><br></span></div><div><span class="gmail-new">I suppose it should never happen, so synchronization is useless !</span></div><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr"><div>@Laurent: is it possible to tweak your test a bit to remove internal API usage and include an assertion? I don't know if you have tried that before, but it would be very nice so we can include your test in the patch. One additional comment regarding the test: the 10 ms lapse between events injection it is too fast for dispatching to keep up with -at least in my environment-; the queue tends to grow in the long run. 100 ms made it stable. I don't expect this test to run for too much though.</div></div></div></blockquote><div><br></div><div>Good idea, I will try rewriting the test.</div><div><br></div><div>Regards,</div><div>Laurent</div></div></div>