Merge pull request #8127 from vector-im/bugfix/fre/fix_countuptimer_stackoverflow

Fix StackOverFlow exception when stop action is called within the tick event

Author: bmarty

changelog.d/8127.bugfix

@@ -0,0 +1 @@
+CountUpTimer - Fix StackOverFlow exception when stop action is called within the tick event

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()
+        lastTime.set(clock.epochMillis())
+        startCounter()
     }
 
+    /**
+     * Pause the timer at the current time.
+     */
     fun pause() {
-        tickListener?.onTick(elapsedTime())
-        counterJob?.cancel()
-        counterJob = null
+        pauseAndTick()
     }
 
+    /**
+     * Resume the timer from the current time.
+     * Does nothing if the timer is already running.
+     */
     fun resume() {
-        lastTime.set(clock.epochMillis())
-        startCounter()
+        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)
     }

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<CountUpTimer.TickListener>(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<CountUpTimer.TickListener>(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<CountUpTimer.TickListener> {
+            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 of the first stop
+        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<CountUpTimer.TickListener> {
+            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 of the first pause
+        verify(exactly = 2) { tickListener.onTick(any()) } // one after reaching the first interval, a second after the pause action
+    }
 }