From 68712513b369d17151663df136769a8fdee0b2c5 Mon Sep 17 00:00:00 2001 From: Florian Renaud Date: Tue, 14 Feb 2023 12:16:24 +0100 Subject: [PATCH] Fix StackOverFlow exception when stop action is called within the tick event --- .../lib/core/utils/timer/CountUpTimer.kt | 52 ++++++++++++----- .../lib/core/utils/timer/CountUpTimerTest.kt | 58 +++++++++++++++++++ 2 files changed, 97 insertions(+), 13 deletions(-) diff --git a/library/core-utils/src/main/java/im/vector/lib/core/utils/timer/CountUpTimer.kt b/library/core-utils/src/main/java/im/vector/lib/core/utils/timer/CountUpTimer.kt index 3ed63a407b..ea1d3d36e2 100644 --- a/library/core-utils/src/main/java/im/vector/lib/core/utils/timer/CountUpTimer.kt +++ b/library/core-utils/src/main/java/im/vector/lib/core/utils/timer/CountUpTimer.kt @@ -35,6 +35,7 @@ class CountUpTimer( private val elapsedTime: AtomicLong = AtomicLong(0) private fun startCounter() { + counterJob?.cancel() counterJob = coroutineScope.launch { while (true) { delay(intervalInMs - elapsedTime() % intervalInMs) @@ -54,29 +55,54 @@ class CountUpTimer( } } + /** + * Start a new timer with the initial given time, if any. + * If the timer is already started, it will be restarted. + */ fun start(initialTime: Long = 0L) { elapsedTime.set(initialTime) - resume() - } - - fun pause() { - tickListener?.onTick(elapsedTime()) - counterJob?.cancel() - counterJob = null - } - - fun resume() { lastTime.set(clock.epochMillis()) startCounter() } + /** + * Pause the timer at the current time. + */ + fun pause() { + pauseAndTick() + } + + /** + * Resume the timer from the current time. + * Does nothing if the timer is already running. + */ + fun resume() { + if (counterJob?.isActive != true) { + lastTime.set(clock.epochMillis()) + startCounter() + } + } + + /** + * Stop and reset the timer. + */ fun stop() { - tickListener?.onTick(elapsedTime()) - counterJob?.cancel() - counterJob = null + pauseAndTick() elapsedTime.set(0L) } + private fun pauseAndTick() { + if (counterJob?.isActive == true) { + // get the elapsed time before cancelling the timer + val elapsedTime = elapsedTime() + // cancel the timer before ticking + counterJob?.cancel() + counterJob = null + // tick with the computed elapsed time + tickListener?.onTick(elapsedTime) + } + } + fun interface TickListener { fun onTick(milliseconds: Long) } diff --git a/library/core-utils/src/test/java/im/vector/lib/core/utils/timer/CountUpTimerTest.kt b/library/core-utils/src/test/java/im/vector/lib/core/utils/timer/CountUpTimerTest.kt index 83f11900b1..547eaa817d 100644 --- a/library/core-utils/src/test/java/im/vector/lib/core/utils/timer/CountUpTimerTest.kt +++ b/library/core-utils/src/test/java/im/vector/lib/core/utils/timer/CountUpTimerTest.kt @@ -19,6 +19,8 @@ package im.vector.lib.core.utils.timer import im.vector.lib.core.utils.test.fakes.FakeClock import io.mockk.every import io.mockk.mockk +import io.mockk.spyk +import io.mockk.verify import io.mockk.verifySequence import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceTimeBy @@ -36,6 +38,7 @@ internal class CountUpTimerTest { @Test fun `when pausing and resuming the timer, the timer ticks the right values at the right moments`() = runTest { + // Given every { fakeClock.epochMillis() } answers { currentTime } val tickListener = mockk(relaxed = true) val timer = CountUpTimer( @@ -44,6 +47,7 @@ internal class CountUpTimerTest { intervalInMs = AN_INTERVAL, ).also { it.tickListener = tickListener } + // When timer.start() advanceTimeBy(AN_INTERVAL / 2) // no tick timer.pause() // tick @@ -52,6 +56,7 @@ internal class CountUpTimerTest { advanceTimeBy(AN_INTERVAL * 4) // tick * 4 timer.stop() // tick + // Then verifySequence { tickListener.onTick(AN_INTERVAL / 2) tickListener.onTick(AN_INTERVAL) @@ -64,6 +69,7 @@ internal class CountUpTimerTest { @Test fun `given an initial time, the timer ticks the right values at the right moments`() = runTest { + // Given every { fakeClock.epochMillis() } answers { currentTime } val tickListener = mockk(relaxed = true) val timer = CountUpTimer( @@ -72,6 +78,7 @@ internal class CountUpTimerTest { intervalInMs = AN_INTERVAL, ).also { it.tickListener = tickListener } + // When timer.start(AN_INITIAL_TIME) advanceTimeBy(AN_INTERVAL) // tick timer.pause() // tick @@ -80,6 +87,7 @@ internal class CountUpTimerTest { advanceTimeBy(AN_INTERVAL * 4) // tick * 4 timer.stop() // tick + // Then val offset = AN_INITIAL_TIME % AN_INTERVAL verifySequence { tickListener.onTick(AN_INITIAL_TIME + AN_INTERVAL - offset) @@ -91,4 +99,54 @@ internal class CountUpTimerTest { tickListener.onTick(AN_INITIAL_TIME + AN_INTERVAL * 5) } } + + @Test + fun `when stopping the timer on tick, the stop action is called twice and the timer ticks twice`() = runTest { + // Given + every { fakeClock.epochMillis() } answers { currentTime } + val timer = spyk( + CountUpTimer( + coroutineScope = this, + clock = fakeClock, + intervalInMs = AN_INTERVAL, + ) + ) + val tickListener = mockk { + every { onTick(any()) } answers { timer.stop() } + } + timer.tickListener = tickListener + + // When + timer.start() + advanceTimeBy(AN_INTERVAL * 10) + + // Then + verify(exactly = 2) { timer.stop() } // one call at the first tick, a second time because of the tick on the previous stop action + verify(exactly = 2) { tickListener.onTick(any()) } // one after reaching the first interval, a second after the stop action + } + + @Test + fun `when pausing the timer on tick, the pause action is called twice and the timer ticks twice`() = runTest { + // Given + every { fakeClock.epochMillis() } answers { currentTime } + val timer = spyk( + CountUpTimer( + coroutineScope = this, + clock = fakeClock, + intervalInMs = AN_INTERVAL, + ) + ) + val tickListener = mockk { + every { onTick(any()) } answers { timer.pause() } + } + timer.tickListener = tickListener + + // When + timer.start() + advanceTimeBy(AN_INTERVAL * 10) + + // Then + verify(exactly = 2) { timer.pause() } // one call at the first tick, a second time because of the tick on the previous pause action + verify(exactly = 2) { tickListener.onTick(any()) } // one after reaching the first interval, a second after the pause action + } }