Skip to content

Commit

Permalink
Mark iterationFinished when triggering release event.
Browse files Browse the repository at this point in the history 
When we currently trigger the iteration finished event during the
release, we don't mark the event as triggered. This means that
someone can trigger another release from within the callback,
which then tries to resend the event.

Issue: google#10758

#minor-release

PiperOrigin-RevId: 488645089
  • Loading branch information
@tonihei @microkatz
tonihei authored and microkatz committed Nov 16, 2022
1 parent 01e0db6 commit 3e5103a
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 11 deletions.
1 change: 1 addition & 0 deletions library/common/src/main/java/com/google/android/exoplayer2/util/ListenerSet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ public ListenerHolder(T listener) {
public void release(IterationFinishedEvent<T> event) {
released = true;
if (needsIterationFinishedEvent) {
needsIterationFinishedEvent = false;
event.invoke(listener, flagsBuilder.build());
}
}
Expand Down
58 changes: 47 additions & 11 deletions library/common/src/test/java/com/google/android/exoplayer2/util/ListenerSetTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public void queueEvent_withoutFlush_sendsNoEvents() {

listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2);
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

verifyNoMoreInteractions(listener);
}
Expand All @@ -66,6 +66,7 @@ public void flushEvents_sendsPreviouslyQueuedEventsToAllListeners() {
listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2);
listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.flushEvents();
ShadowLooper.idleMainLooper();

InOrder inOrder = Mockito.inOrder(listener1, listener2);
inOrder.verify(listener1).callback1();
Expand All @@ -74,6 +75,8 @@ public void flushEvents_sendsPreviouslyQueuedEventsToAllListeners() {
inOrder.verify(listener2).callback2();
inOrder.verify(listener1).callback1();
inOrder.verify(listener2).callback1();
inOrder.verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1, EVENT_ID_2));
inOrder.verify(listener2).iterationFinished(createFlagSet(EVENT_ID_1, EVENT_ID_2));
inOrder.verifyNoMoreInteractions();
}

Expand All @@ -98,6 +101,7 @@ public void callback1() {
listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2);
listenerSet.flushEvents();
ShadowLooper.idleMainLooper();

InOrder inOrder = Mockito.inOrder(listener1, listener2);
inOrder.verify(listener1).callback1();
Expand All @@ -106,6 +110,8 @@ public void callback1() {
inOrder.verify(listener2).callback2();
inOrder.verify(listener1).callback3();
inOrder.verify(listener2).callback3();
inOrder.verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1, EVENT_ID_2, EVENT_ID_3));
inOrder.verify(listener2).iterationFinished(createFlagSet(EVENT_ID_1, EVENT_ID_2, EVENT_ID_3));
inOrder.verifyNoMoreInteractions();
}

Expand All @@ -130,19 +136,19 @@ public void callback3() {
// Iteration with single flush.
listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2);
listenerSet.flushEvents();
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

// Iteration with multiple flushes.
listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2);
listenerSet.flushEvents();
listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.flushEvents();
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

// Iteration with recursive call.
listenerSet.sendEvent(EVENT_ID_3, TestListener::callback3);
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

InOrder inOrder = Mockito.inOrder(listener1, listener2);
inOrder.verify(listener1).callback2();
Expand Down Expand Up @@ -191,7 +197,7 @@ public void iterationFinished(FlagSet flags) {
listenerSet.add(listener3);

listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2);
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

InOrder inOrder = Mockito.inOrder(listener1, listener2, listener3);
inOrder.verify(listener1).callback2();
Expand All @@ -215,7 +221,7 @@ public void flushEvents_withUnsetEventFlag_doesNotThrow() {

listenerSet.queueEvent(/* eventFlag= */ C.INDEX_UNSET, TestListener::callback1);
listenerSet.flushEvents();
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

// Asserts that negative event flag (INDEX_UNSET) can be used without throwing.
}
Expand All @@ -241,7 +247,7 @@ public void callback1() {
// listener2 was added.
listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2);
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

InOrder inOrder = Mockito.inOrder(listener1, listener2);
inOrder.verify(listener1).callback1();
Expand All @@ -266,7 +272,7 @@ public void add_withQueueing_onlyReceivesUpdatesForFutureEvents() {
listenerSet.add(listener2);
listenerSet.queueEvent(EVENT_ID_2, TestListener::callback2);
listenerSet.flushEvents();
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

InOrder inOrder = Mockito.inOrder(listener1, listener2);
inOrder.verify(listener1).callback1();
Expand Down Expand Up @@ -298,7 +304,7 @@ public void callback1() {
listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.remove(listener1);
listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2);
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

verify(listener1).callback1();
verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1));
Expand All @@ -319,7 +325,7 @@ public void remove_withQueueing_stopsReceivingEventsImmediately() {
listenerSet.remove(listener1);
listenerSet.queueEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.flushEvents();
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

verify(listener2, times(2)).callback1();
verify(listener2).iterationFinished(createFlagSet(EVENT_ID_1));
Expand All @@ -346,10 +352,40 @@ public void callback1() {
// Listener2 shouldn't even get this event as it's released before the event can be invoked.
listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.sendEvent(EVENT_ID_2, TestListener::callback2);
ShadowLooper.runMainLooperToNextTask();
ShadowLooper.idleMainLooper();

verify(listener1).callback1();
verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1));
verifyNoMoreInteractions(listener1, listener2);
}

@Test
public void remove_withRecursionDuringRelease_callsAllPendingEventsAndIterationFinished() {
ListenerSet<TestListener> listenerSet =
new ListenerSet<>(Looper.myLooper(), Clock.DEFAULT, TestListener::iterationFinished);
TestListener listener2 = mock(TestListener.class);
// Listener1 removes Listener2 from within the callback triggered by release().
TestListener listener1 =
spy(
new TestListener() {
@Override
public void iterationFinished(FlagSet flags) {
listenerSet.remove(listener2);
}
});
listenerSet.add(listener1);
listenerSet.add(listener2);

// Listener2 should still get the event and iterationFinished callback because it was triggered
// before the release and the listener removal.
listenerSet.sendEvent(EVENT_ID_1, TestListener::callback1);
listenerSet.release();
ShadowLooper.idleMainLooper();

verify(listener1).callback1();
verify(listener1).iterationFinished(createFlagSet(EVENT_ID_1));
verify(listener2).callback1();
verify(listener2).iterationFinished(createFlagSet(EVENT_ID_1));
verifyNoMoreInteractions(listener1, listener2);
}

Expand Down

0 comments on commit 3e5103a

Please sign in to comment.