From a74c206a0c277ae7a1800ecb2c50dd3fdc171a22 Mon Sep 17 00:00:00 2001 From: cketti Date: Wed, 16 Jun 2021 14:53:50 +0200 Subject: [PATCH] Add implementation for BackendIdleRefreshManager --- .../fsck/k9/backends/AndroidAlarmManager.kt | 17 ++ .../java/com/fsck/k9/backends/KoinModule.kt | 4 +- .../fsck/k9/backends/AndroidAlarmManager.kt | 17 ++ .../java/com/fsck/k9/backends/KoinModule.kt | 4 +- backend/imap/build.gradle | 1 + .../backend/imap/BackendIdleRefreshManager.kt | 130 ++++++++++++- .../k9/backend/imap/SystemAlarmManager.kt | 7 + .../imap/BackendIdleRefreshManagerTest.kt | 182 ++++++++++++++++++ 8 files changed, 356 insertions(+), 6 deletions(-) create mode 100644 app/k9mail-jmap/src/main/java/com/fsck/k9/backends/AndroidAlarmManager.kt create mode 100644 app/k9mail/src/main/java/com/fsck/k9/backends/AndroidAlarmManager.kt create mode 100644 backend/imap/src/main/java/com/fsck/k9/backend/imap/SystemAlarmManager.kt create mode 100644 backend/imap/src/test/java/com/fsck/k9/backend/imap/BackendIdleRefreshManagerTest.kt diff --git a/app/k9mail-jmap/src/main/java/com/fsck/k9/backends/AndroidAlarmManager.kt b/app/k9mail-jmap/src/main/java/com/fsck/k9/backends/AndroidAlarmManager.kt new file mode 100644 index 000000000..6b462ae14 --- /dev/null +++ b/app/k9mail-jmap/src/main/java/com/fsck/k9/backends/AndroidAlarmManager.kt @@ -0,0 +1,17 @@ +package com.fsck.k9.backends + +import com.fsck.k9.backend.imap.SystemAlarmManager + +class AndroidAlarmManager : SystemAlarmManager { + override fun setAlarm(triggerTime: Long, callback: () -> Unit) { + TODO("implement") + } + + override fun cancelAlarm() { + TODO("implement") + } + + override fun now(): Long { + TODO("implement") + } +} diff --git a/app/k9mail-jmap/src/main/java/com/fsck/k9/backends/KoinModule.kt b/app/k9mail-jmap/src/main/java/com/fsck/k9/backends/KoinModule.kt index 92e65ab7d..bfc6da37b 100644 --- a/app/k9mail-jmap/src/main/java/com/fsck/k9/backends/KoinModule.kt +++ b/app/k9mail-jmap/src/main/java/com/fsck/k9/backends/KoinModule.kt @@ -2,6 +2,7 @@ package com.fsck.k9.backends import com.fsck.k9.backend.BackendManager import com.fsck.k9.backend.imap.BackendIdleRefreshManager +import com.fsck.k9.backend.imap.SystemAlarmManager import com.fsck.k9.backend.jmap.JmapAccountDiscovery import com.fsck.k9.mail.store.imap.IdleRefreshManager import org.koin.dsl.module @@ -26,7 +27,8 @@ val backendsModule = module { trustedSocketFactory = get() ) } - single { BackendIdleRefreshManager() } + single { AndroidAlarmManager() } + single { BackendIdleRefreshManager(alarmManager = get()) } single { Pop3BackendFactory(get(), get()) } single { WebDavBackendFactory(get(), get(), get()) } single { JmapBackendFactory(get(), get()) } diff --git a/app/k9mail/src/main/java/com/fsck/k9/backends/AndroidAlarmManager.kt b/app/k9mail/src/main/java/com/fsck/k9/backends/AndroidAlarmManager.kt new file mode 100644 index 000000000..6b462ae14 --- /dev/null +++ b/app/k9mail/src/main/java/com/fsck/k9/backends/AndroidAlarmManager.kt @@ -0,0 +1,17 @@ +package com.fsck.k9.backends + +import com.fsck.k9.backend.imap.SystemAlarmManager + +class AndroidAlarmManager : SystemAlarmManager { + override fun setAlarm(triggerTime: Long, callback: () -> Unit) { + TODO("implement") + } + + override fun cancelAlarm() { + TODO("implement") + } + + override fun now(): Long { + TODO("implement") + } +} diff --git a/app/k9mail/src/main/java/com/fsck/k9/backends/KoinModule.kt b/app/k9mail/src/main/java/com/fsck/k9/backends/KoinModule.kt index f332c4d04..2d81a5388 100644 --- a/app/k9mail/src/main/java/com/fsck/k9/backends/KoinModule.kt +++ b/app/k9mail/src/main/java/com/fsck/k9/backends/KoinModule.kt @@ -2,6 +2,7 @@ package com.fsck.k9.backends import com.fsck.k9.backend.BackendManager import com.fsck.k9.backend.imap.BackendIdleRefreshManager +import com.fsck.k9.backend.imap.SystemAlarmManager import com.fsck.k9.mail.store.imap.IdleRefreshManager import org.koin.dsl.module @@ -24,7 +25,8 @@ val backendsModule = module { trustedSocketFactory = get() ) } - single { BackendIdleRefreshManager() } + single { AndroidAlarmManager() } + single { BackendIdleRefreshManager(alarmManager = get()) } single { Pop3BackendFactory(get(), get()) } single { WebDavBackendFactory(get(), get(), get()) } } diff --git a/backend/imap/build.gradle b/backend/imap/build.gradle index d853a13e7..631d9d063 100644 --- a/backend/imap/build.gradle +++ b/backend/imap/build.gradle @@ -15,6 +15,7 @@ dependencies { testImplementation project(":mail:testing") testImplementation "junit:junit:${versions.junit}" testImplementation "org.mockito:mockito-core:${versions.mockito}" + testImplementation "com.google.truth:truth:${versions.truth}" } android { diff --git a/backend/imap/src/main/java/com/fsck/k9/backend/imap/BackendIdleRefreshManager.kt b/backend/imap/src/main/java/com/fsck/k9/backend/imap/BackendIdleRefreshManager.kt index 730dc386e..b7996a100 100644 --- a/backend/imap/src/main/java/com/fsck/k9/backend/imap/BackendIdleRefreshManager.kt +++ b/backend/imap/src/main/java/com/fsck/k9/backend/imap/BackendIdleRefreshManager.kt @@ -3,12 +3,134 @@ package com.fsck.k9.backend.imap import com.fsck.k9.mail.store.imap.IdleRefreshManager import com.fsck.k9.mail.store.imap.IdleRefreshTimer -class BackendIdleRefreshManager : IdleRefreshManager { - override fun startTimer(timeout: Long, callback: () -> Unit): IdleRefreshTimer { - TODO("implement") +private typealias Callback = () -> Unit + +private const val MIN_TIMER_DELTA = 1 * 60 * 1000L +private const val NO_TRIGGER_TIME = 0L + +/** + * Timer mechanism to refresh IMAP IDLE connections. + * + * Triggers timers early if necessary to reduce the number of times the device has to be woken up. + */ +class BackendIdleRefreshManager(private val alarmManager: SystemAlarmManager) : IdleRefreshManager { + private var timers = mutableSetOf() + private var currentTriggerTime = NO_TRIGGER_TIME + private var minTimeout = Long.MAX_VALUE + private var minTimeoutTimestamp = 0L + + @Synchronized + override fun startTimer(timeout: Long, callback: Callback): IdleRefreshTimer { + require(timeout > MIN_TIMER_DELTA) { "Timeout needs to be greater than $MIN_TIMER_DELTA ms" } + + val now = alarmManager.now() + val triggerTime = now + timeout + + updateMinTimeout(timeout, now) + setOrUpdateAlarm(triggerTime) + + return BackendIdleRefreshTimer(triggerTime, callback).also { timer -> + timers.add(timer) + } } override fun resetTimers() { - TODO("implement") + synchronized(this) { + cancelAlarm() + } + + onTimeout() + } + + private fun updateMinTimeout(timeout: Long, now: Long) { + if (minTimeoutTimestamp + minTimeout * 2 < now) { + minTimeout = Long.MAX_VALUE + } + + if (timeout <= minTimeout) { + minTimeout = timeout + minTimeoutTimestamp = now + } + } + + private fun setOrUpdateAlarm(triggerTime: Long) { + if (currentTriggerTime == NO_TRIGGER_TIME) { + setAlarm(triggerTime) + } else if (currentTriggerTime - triggerTime > MIN_TIMER_DELTA) { + adjustAlarm(triggerTime) + } + } + + private fun setAlarm(triggerTime: Long) { + currentTriggerTime = triggerTime + alarmManager.setAlarm(triggerTime, ::onTimeout) + } + + private fun adjustAlarm(triggerTime: Long) { + currentTriggerTime = triggerTime + alarmManager.cancelAlarm() + alarmManager.setAlarm(triggerTime, ::onTimeout) + } + + private fun cancelAlarm() { + currentTriggerTime = NO_TRIGGER_TIME + alarmManager.cancelAlarm() + } + + private fun onTimeout() { + val triggerTimers = synchronized(this) { + currentTriggerTime = NO_TRIGGER_TIME + + if (timers.isEmpty()) return + + val now = alarmManager.now() + val minNextTriggerTime = now + minTimeout + + val triggerTimers = timers.filter { it.triggerTime < minNextTriggerTime - MIN_TIMER_DELTA } + timers.removeAll(triggerTimers) + + timers.minOfOrNull { it.triggerTime }?.let { nextTriggerTime -> + setAlarm(nextTriggerTime) + } + + triggerTimers + } + + for (timer in triggerTimers) { + timer.onTimeout() + } + } + + @Synchronized + private fun removeTimer(timer: BackendIdleRefreshTimer) { + timers.remove(timer) + + if (timers.isEmpty()) { + cancelAlarm() + } + } + + internal inner class BackendIdleRefreshTimer( + val triggerTime: Long, + val callback: Callback + ) : IdleRefreshTimer { + override var isWaiting: Boolean = true + private set + + @Synchronized + override fun cancel() { + if (isWaiting) { + isWaiting = false + removeTimer(this) + } + } + + internal fun onTimeout() { + synchronized(this) { + isWaiting = false + } + + callback.invoke() + } } } diff --git a/backend/imap/src/main/java/com/fsck/k9/backend/imap/SystemAlarmManager.kt b/backend/imap/src/main/java/com/fsck/k9/backend/imap/SystemAlarmManager.kt new file mode 100644 index 000000000..e2133cfd0 --- /dev/null +++ b/backend/imap/src/main/java/com/fsck/k9/backend/imap/SystemAlarmManager.kt @@ -0,0 +1,7 @@ +package com.fsck.k9.backend.imap + +interface SystemAlarmManager { + fun setAlarm(triggerTime: Long, callback: () -> Unit) + fun cancelAlarm() + fun now(): Long +} diff --git a/backend/imap/src/test/java/com/fsck/k9/backend/imap/BackendIdleRefreshManagerTest.kt b/backend/imap/src/test/java/com/fsck/k9/backend/imap/BackendIdleRefreshManagerTest.kt new file mode 100644 index 000000000..cd5c4d7e7 --- /dev/null +++ b/backend/imap/src/test/java/com/fsck/k9/backend/imap/BackendIdleRefreshManagerTest.kt @@ -0,0 +1,182 @@ +package com.fsck.k9.backend.imap + +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +private const val START_TIME = 100_000_000L + +class BackendIdleRefreshManagerTest { + val alarmManager = MockSystemAlarmManager(START_TIME) + val idleRefreshManager = BackendIdleRefreshManager(alarmManager) + + @Test + fun `single timer`() { + val timeout = 15 * 60 * 1000L + val callback = RecordingCallback() + + idleRefreshManager.startTimer(timeout, callback::alarm) + alarmManager.advanceTime(timeout) + + assertThat(alarmManager.alarmTimes).isEqualTo(listOf(START_TIME + timeout)) + assertThat(callback.wasCalled).isTrue() + } + + @Test + fun `starting two timers in quick succession`() { + val timeout = 15 * 60 * 1000L + val callback1 = RecordingCallback() + val callback2 = RecordingCallback() + + idleRefreshManager.startTimer(timeout, callback1::alarm) + // Advance clock less than MIN_TIMER_DELTA + alarmManager.advanceTime(100) + idleRefreshManager.startTimer(timeout, callback2::alarm) + alarmManager.advanceTime(timeout) + + assertThat(alarmManager.alarmTimes).isEqualTo(listOf(START_TIME + timeout)) + assertThat(callback1.wasCalled).isTrue() + assertThat(callback2.wasCalled).isTrue() + } + + @Test + fun `starting second timer some time after first should trigger both at initial trigger time`() { + val timeout = 15 * 60 * 1000L + val waitTime = 10 * 60 * 1000L + val callback1 = RecordingCallback() + val callback2 = RecordingCallback() + + idleRefreshManager.startTimer(timeout, callback1::alarm) + // Advance clock by more than MIN_TIMER_DELTA but less than 'timeout' + alarmManager.advanceTime(waitTime) + + assertThat(callback1.wasCalled).isFalse() + + idleRefreshManager.startTimer(timeout, callback2::alarm) + alarmManager.advanceTime(timeout - waitTime) + + assertThat(alarmManager.alarmTimes).isEqualTo(listOf(START_TIME + timeout)) + assertThat(callback1.wasCalled).isTrue() + assertThat(callback2.wasCalled).isTrue() + } + + @Test + fun `second timer with lower timeout should reschedule alarm`() { + val timeout1 = 15 * 60 * 1000L + val timeout2 = 10 * 60 * 1000L + val callback1 = RecordingCallback() + val callback2 = RecordingCallback() + + idleRefreshManager.startTimer(timeout1, callback1::alarm) + + assertThat(alarmManager.triggerTime).isEqualTo(START_TIME + timeout1) + + idleRefreshManager.startTimer(timeout2, callback2::alarm) + alarmManager.advanceTime(timeout2) + + assertThat(alarmManager.alarmTimes).isEqualTo(listOf(START_TIME + timeout1, START_TIME + timeout2)) + assertThat(callback1.wasCalled).isTrue() + assertThat(callback2.wasCalled).isTrue() + } + + @Test + fun `do not trigger timers earlier than necessary`() { + val timeout1 = 10 * 60 * 1000L + val timeout2 = 23 * 60 * 1000L + val callback1 = RecordingCallback() + val callback2 = RecordingCallback() + val callback3 = RecordingCallback() + + idleRefreshManager.startTimer(timeout1, callback1::alarm) + idleRefreshManager.startTimer(timeout2, callback2::alarm) + + alarmManager.advanceTime(timeout1) + assertThat(callback1.wasCalled).isTrue() + assertThat(callback2.wasCalled).isFalse() + + idleRefreshManager.startTimer(timeout1, callback3::alarm) + + alarmManager.advanceTime(timeout1) + + assertThat(alarmManager.alarmTimes).isEqualTo( + listOf(START_TIME + timeout1, START_TIME + timeout2, START_TIME + timeout1 + timeout1) + ) + assertThat(callback2.wasCalled).isTrue() + assertThat(callback3.wasCalled).isTrue() + } + + @Test + fun `reset timers`() { + val timeout = 10 * 60 * 1000L + val callback = RecordingCallback() + + idleRefreshManager.startTimer(timeout, callback::alarm) + + alarmManager.advanceTime(5 * 60 * 1000L) + assertThat(callback.wasCalled).isFalse() + + idleRefreshManager.resetTimers() + + assertThat(alarmManager.triggerTime).isEqualTo(NO_TRIGGER_TIME) + assertThat(callback.wasCalled).isTrue() + } + + @Test + fun `cancel timer`() { + val timeout = 10 * 60 * 1000L + val callback = RecordingCallback() + + val timer = idleRefreshManager.startTimer(timeout, callback::alarm) + + alarmManager.advanceTime(5 * 60 * 1000L) + timer.cancel() + + assertThat(alarmManager.triggerTime).isEqualTo(NO_TRIGGER_TIME) + assertThat(callback.wasCalled).isFalse() + } +} + +class RecordingCallback { + var wasCalled = false + private set + + fun alarm() { + wasCalled = true + } +} + +typealias Callback = () -> Unit +private const val NO_TRIGGER_TIME = -1L + +class MockSystemAlarmManager(startTime: Long) : SystemAlarmManager { + var now = startTime + var triggerTime = NO_TRIGGER_TIME + var callback: Callback? = null + val alarmTimes = mutableListOf() + + override fun setAlarm(triggerTime: Long, callback: () -> Unit) { + this.triggerTime = triggerTime + this.callback = callback + alarmTimes.add(triggerTime) + } + + override fun cancelAlarm() { + this.triggerTime = NO_TRIGGER_TIME + this.callback = null + } + + override fun now(): Long = now + + fun advanceTime(delta: Long) { + now += delta + if (now >= triggerTime) { + trigger() + } + } + + private fun trigger() { + callback?.invoke().also { + triggerTime = NO_TRIGGER_TIME + callback = null + } + } +}