johncarl81 / transfuse

:syringe: Transfuse - A Dependency Injection and Integration framework for Google Android
http://androidtransfuse.org/
Apache License 2.0
220 stars 28 forks source link

Prevent deadlock in EventManager.trigger() #173

Closed goodsoft closed 9 years ago

goodsoft commented 9 years ago

If a new event observer is registered during event execution, a deadlock occurs due to locked observersLock.

goodsoft commented 9 years ago

Mmkay, seems it's failing some tests.

johncarl81 commented 9 years ago

I'm surprised to hear about deadlock wtih the EventManager, as I have a test (albeit, nondeterministic) that focuses on just that: https://github.com/johncarl81/transfuse/blob/master/transfuse-api/src/test/java/org/androidtransfuse/event/UnregisterConcurrencyTest.java#L189

goodsoft commented 9 years ago

Here's a distilled test case for deadlock:

        eventManager.register(Event.class, new EventObserver<Event>() {
            public void trigger(Event event) {
                eventManager.register(Event.class, otherEventObserver); // deadlock!
            }
        });
        eventManager.trigger(new Event());
johncarl81 commented 9 years ago

Hmm, this should be protected against by a reentrant lock...

goodsoft commented 9 years ago

Apparently writeLock() called by register() waits for readLock() set by trigger() to be released.

goodsoft commented 9 years ago

Additionally, a writer can acquire the read lock, but not vice-versa, states ReentrantReadWriteLock class documentation

johncarl81 commented 9 years ago

We could move triggerQueue() to below the finally block. The side effect of this would be that you could unregister from an event and still receive events (see https://github.com/johncarl81/transfuse/blob/master/transfuse-api/src/test/java/org/androidtransfuse/event/UnregisterConcurrencyTest.java#L162)

goodsoft commented 9 years ago

Yeah, I have done exactly that, and that has failed exactly this test. Not sure how to circumvent this problem

johncarl81 commented 9 years ago

Recalling a conversation I had with some of the Otto devs, if it isn't a big deal that we might see events after calling unregister, we can probably get rid of that lock altogether.

johncarl81 commented 9 years ago

@goodsoft: #175