Hi Martin, What an amazing effort ! I am very pleased that my (crazy) test triggered a very complicated case; I will try asap if it works with your new webrev.
Thanks for your feedback and sharing your test. > > 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. > > I could reproduce your deadlock. This deadlock scenario is a bit more > interesting than the one fixed in Webrev00. > > At some point, we may reach the following state: > > SequenceEvent.list: EV0-AppContext1, EV0-AppContext0, EV1-AppContext0 > EventQueue0: EV0-AppContext0, EV1-AppContext0 > > 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. > > 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). > > 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. > > 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. > > 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: > > 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). > > 2) We cannot loose SequencedEvent events. Of course. > > 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). > > 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. > > Removed an unnecessary "wakeup" on SequencedEvent.dispose function. > > Webrev 01: > > * http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.01/ > * > http://cr.openjdk.java.net/~mbalao/webrevs/8204142/8204142.webrev.01.zip > I had a quick look to the patch. I noticed that the list pendingEvents is not synchronized and I wonder if multiple EDT threads may be working on the same SequencedEvent: - currentSequencedEvent.pendingEvents.add(ev); - for(AWTEvent e : pendingEvents) { SunToolkit.postEvent(appContext, e); } I suppose it should never happen, so synchronization is useless ! @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. > Good idea, I will try rewriting the test. Regards, Laurent
