concurrency problem (FilterList with ThreadedMatcherEditor) ?

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

concurrency problem (FilterList with ThreadedMatcherEditor) ?

hbrands
Administrator
Hey guys,

we sometimes get the following exception when using a FilterList with a ThreadedMatcherEditor:

java.lang.IllegalStateException
at ca.odell.glazedlists.FilterList.changeMatcher(FilterList.java:285)
at ca.odell.glazedlists.FilterList.changeMatcherWithLocks(FilterList.java:271)
at ca.odell.glazedlists.FilterList.access$100(FilterList.java:51)
at ca.odell.glazedlists.FilterList$PrivateMatcherEditorListener.changedMatcher(FilterList.java:445)
at ca.odell.glazedlists.matchers.AbstractMatcherEditor.fireChangedMatcher(AbstractMatcherEditor.java:115)
at ca.odell.glazedlists.matchers.ThreadedMatcherEditor$DrainMatcherEventQueueRunnable.run(ThreadedMatcherEditor.java:247)
at java.lang.Thread.run(Thread.java:619)

The stacktrace indicates, that while the ThreadMatcherEditor notifies its listeners about a MatcherEvent in its own MatcherQueueThread,
one of the listeners (the FilterList) throws an exception because the MatcherEditors of the event and the FilterList are not equal anymore.
Debug output showed that the currentEditor of the FilterList was null, because the FilterList had been disposed
(even though we write lock the FilterList when disposing).

I think the following is going on:
- we have a valid FilterList with its ThreadedMatcherEditor
- at some time the ThreadedMatcherEditor gets an event from the source MatcherEditor
- ThreadedMatcherEditor starts the MatcherQueueThread to fire a single coalesced MatcherEvent
- this thread calls the AbstractMatcherEditor.fireChangedMatcher(..) method to get the listeners of the MatcherEvent (FilterList is one of them)
- it starts iterating over the listeners, but then the thread context switches to another thread
- this other thread disposes the FilterList (this sets the currentEditor in FilterList to null)
- now the thread context switches back and AbstractMatcherEditor.fireChangedMatcher(..) continues to notify the listeners
- one of them is the (now disposed) FilterList which then runs into the above mentioned exception

It's difficult to create a test case for this scenario. But the following change to FilterList seemed to help:
We introduced a volatile boolean disposed flag in FilterList that is set to true in the dispose() method.
The private changeMatcher(..) method is changed like this to check the disposed flag:
...
if (!disposed) {
        // ensure the MatcherEvent is from OUR MatcherEditor
        if (currentEditor != matcherEditor) throw new IllegalStateException();

        switch (changeType) {
            case MatcherEditor.Event.CONSTRAINED: currentMatcher = matcher; this.constrained(); break;
            case MatcherEditor.Event.RELAXED: currentMatcher = matcher; this.relaxed(); break;
            case MatcherEditor.Event.CHANGED: currentMatcher = matcher; this.changed(); break;
            case MatcherEditor.Event.MATCH_ALL: currentMatcher = Matchers.trueMatcher(); this.matchAll(); break;
            case MatcherEditor.Event.MATCH_NONE: currentMatcher = Matchers.falseMatcher(); this.matchNone(); break;
        }
}
The idea is, that if the FilterList is already disposed, it doesn't have to do anything more.

Do you know a better way to solve this problem?
If not, should I apply this change?

Thanks,
Holger

Reply | Threaded
Open this post in threaded view
|

Re: concurrency problem (FilterList with ThreadedMatcherEditor) ?

Endre Stølsvik-8
I don't know, but please apply this change too while you're at it:

Endre.

On Sun, Aug 29, 2010 at 16:36, Holger Brands <[hidden email]> wrote:
Hey guys,

we sometimes get the following exception when using a FilterList with a ThreadedMatcherEditor:

java.lang.IllegalStateException
at ca.odell.glazedlists.FilterList.changeMatcher(FilterList.java:285)
at ca.odell.glazedlists.FilterList.changeMatcherWithLocks(FilterList.java:271)
at ca.odell.glazedlists.FilterList.access$100(FilterList.java:51)
at ca.odell.glazedlists.FilterList$PrivateMatcherEditorListener.changedMatcher(FilterList.java:445)
at ca.odell.glazedlists.matchers.AbstractMatcherEditor.fireChangedMatcher(AbstractMatcherEditor.java:115)
at ca.odell.glazedlists.matchers.ThreadedMatcherEditor$DrainMatcherEventQueueRunnable.run(ThreadedMatcherEditor.java:247)
at java.lang.Thread.run(Thread.java:619)

The stacktrace indicates, that while the ThreadMatcherEditor notifies its listeners about a MatcherEvent in its own MatcherQueueThread,
one of the listeners (the FilterList) throws an exception because the MatcherEditors of the event and the FilterList are not equal anymore.
Debug output showed that the currentEditor of the FilterList was null, because the FilterList had been disposed
(even though we write lock the FilterList when disposing).

I think the following is going on:
- we have a valid FilterList with its ThreadedMatcherEditor
- at some time the ThreadedMatcherEditor gets an event from the source MatcherEditor
- ThreadedMatcherEditor starts the MatcherQueueThread to fire a single coalesced MatcherEvent
- this thread calls the AbstractMatcherEditor.fireChangedMatcher(..) method to get the listeners of the MatcherEvent (FilterList is one of them)
- it starts iterating over the listeners, but then the thread context switches to another thread
- this other thread disposes the FilterList (this sets the currentEditor in FilterList to null)
- now the thread context switches back and AbstractMatcherEditor.fireChangedMatcher(..) continues to notify the listeners
- one of them is the (now disposed) FilterList which then runs into the above mentioned exception

It's difficult to create a test case for this scenario. But the following change to FilterList seemed to help:
We introduced a volatile boolean disposed flag in FilterList that is set to true in the dispose() method.
The private changeMatcher(..) method is changed like this to check the disposed flag:
...
if (!disposed) {
        // ensure the MatcherEvent is from OUR MatcherEditor
        if (currentEditor != matcherEditor) throw new IllegalStateException();

        switch (changeType) {
            case MatcherEditor.Event.CONSTRAINED: currentMatcher = matcher; this.constrained(); break;
            case MatcherEditor.Event.RELAXED: currentMatcher = matcher; this.relaxed(); break;
            case MatcherEditor.Event.CHANGED: currentMatcher = matcher; this.changed(); break;
            case MatcherEditor.Event.MATCH_ALL: currentMatcher = Matchers.trueMatcher(); this.matchAll(); break;
            case MatcherEditor.Event.MATCH_NONE: currentMatcher = Matchers.falseMatcher(); this.matchNone(); break;
        }
}
The idea is, that if the FilterList is already disposed, it doesn't have to do anything more.

Do you know a better way to solve this problem?
If not, should I apply this change?

Thanks,
Holger