diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt index c39caa5781..44cf6edefc 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/push/DefaultPushHandler.kt @@ -64,7 +64,7 @@ class DefaultPushHandler( * @param pushData the data received in the push. * @param providerInfo the provider info. */ - override suspend fun handle(pushData: PushData, providerInfo: String) { + override suspend fun handle(pushData: PushData, providerInfo: String): Boolean { // Start measuring how long it takes to display a notification from when the push is received Timber.d("Calculating push-to-notification for event ${pushData.eventId}") val parent = analyticsService.startLongRunningTransaction(AnalyticsLongRunningTransaction.PushToNotification(pushData.eventId.value)) @@ -81,9 +81,10 @@ class DefaultPushHandler( } // Diagnostic Push - if (pushData.eventId == DefaultTestPush.TEST_EVENT_ID) { + return if (pushData.eventId == DefaultTestPush.TEST_EVENT_ID) { pushHistoryService.onDiagnosticPush(providerInfo) diagnosticPushHandler.handlePush() + false } else { handleInternal(pushData, providerInfo) } @@ -100,7 +101,7 @@ class DefaultPushHandler( * @param pushData Object containing message data. * @param providerInfo the provider info. */ - private suspend fun handleInternal(pushData: PushData, providerInfo: String) { + private suspend fun handleInternal(pushData: PushData, providerInfo: String): Boolean { try { if (buildMeta.lowPrivacyLoggingEnabled) { Timber.tag(loggerTag.value).d("## handleInternal() : $pushData") @@ -117,13 +118,13 @@ class DefaultPushHandler( roomId = pushData.roomId, reason = "Unable to get userId from client secret", ) - return + return false } val areNotificationsEnabled = userPushStoreFactory.getOrCreate(userId).getNotificationEnabledForDevice().first() if (!areNotificationsEnabled) { Timber.w("Push notification received when push notifications are disabled.") - return + return false } val pushRequest = PushRequest( @@ -144,8 +145,11 @@ class DefaultPushHandler( Timber.d("No pending worker for push notifications found") workManagerScheduler.submit(syncPendingNotificationsRequestFactory.create(userId)) } + + return true } catch (e: Exception) { Timber.tag(loggerTag.value).e(e, "## handleInternal() failed") + return false } } } diff --git a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/test/FakePushHandler.kt b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/test/FakePushHandler.kt index a7e476be9c..27f1af5baa 100644 --- a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/test/FakePushHandler.kt +++ b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/test/FakePushHandler.kt @@ -13,11 +13,11 @@ import io.element.android.libraries.pushproviders.api.PushHandler import io.element.android.tests.testutils.lambda.lambdaError class FakePushHandler( - private val handleResult: (PushData, String) -> Unit = { _, _ -> lambdaError() }, + private val handleResult: (PushData, String) -> Boolean = { _, _ -> lambdaError() }, private val handleInvalidResult: (String, String) -> Unit = { _, _ -> lambdaError() }, ) : PushHandler { - override suspend fun handle(pushData: PushData, providerInfo: String) { - handleResult(pushData, providerInfo) + override suspend fun handle(pushData: PushData, providerInfo: String): Boolean { + return handleResult(pushData, providerInfo) } override suspend fun handleInvalid(providerInfo: String, data: String) { diff --git a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushHandler.kt b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushHandler.kt index d3105111c5..49b578070b 100644 --- a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushHandler.kt +++ b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushHandler.kt @@ -9,11 +9,21 @@ package io.element.android.libraries.pushproviders.api interface PushHandler { + /** + * Handle a push received from the provider. + * + * @param pushData the data of the push, containing the client secret and the push content. + * @param providerInfo an identifier of the provider that sent the push, for logging and debugging purposes. + * @return `true` if the push was handled successfully and is now enqueued for processing, false otherwise. + */ suspend fun handle( pushData: PushData, providerInfo: String, - ) + ): Boolean + /** + * Handle an invalid push received from the provider. + */ suspend fun handleInvalid( providerInfo: String, data: String, diff --git a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingService.kt b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingService.kt index 6c479b92c1..67da09f1ab 100644 --- a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingService.kt +++ b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingService.kt @@ -58,11 +58,17 @@ class VectorFirebaseMessagingService : FirebaseMessagingService() { "$it: ${message.data[it]}" }, ) + pushHandlingWakeLock.unlock() } else { - pushHandler.handle( + val handled = pushHandler.handle( pushData = pushData, providerInfo = FirebaseConfig.NAME, ) + + // If we failed to handle the push, we should release the wakelock early to avoid keeping the device awake for too long. + if (!handled) { + pushHandlingWakeLock.unlock() + } } } } diff --git a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingServiceTest.kt b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingServiceTest.kt index 81bf19e666..87ca74730c 100644 --- a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingServiceTest.kt +++ b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/VectorFirebaseMessagingServiceTest.kt @@ -29,6 +29,7 @@ import kotlinx.coroutines.test.runTest import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner +import kotlin.time.Duration @RunWith(RobolectricTestRunner::class) class VectorFirebaseMessagingServiceTest { @@ -56,7 +57,7 @@ class VectorFirebaseMessagingServiceTest { @Test fun `test receiving valid data`() = runTest { - val lambda = lambdaRecorder { _, _ -> } + val lambda = lambdaRecorder { _, _ -> true } val vectorFirebaseMessagingService = createVectorFirebaseMessagingService( pushHandler = FakePushHandler(handleResult = lambda) ) @@ -78,6 +79,68 @@ class VectorFirebaseMessagingServiceTest { ) } + @Test + fun `test pushHandler returning true locks and does not unlock the wakelock so it continues running`() = runTest { + val lockLambda = lambdaRecorder { _ -> } + val unlockLambda = lambdaRecorder { } + val vectorFirebaseMessagingService = createVectorFirebaseMessagingService( + pushHandler = FakePushHandler(handleResult = { _, _ -> true }), + pushHandlingWakeLock = FakePushHandlingWakeLock( + lock = lockLambda, + unlock = unlockLambda + ) + ) + vectorFirebaseMessagingService.onMessageReceived( + message = RemoteMessage( + Bundle().apply { + putString("event_id", AN_EVENT_ID.value) + putString("room_id", A_ROOM_ID.value) + putString("cs", A_SECRET) + }, + ) + ) + + // The wakelock should be locked but not unlocked + lockLambda.assertions().isCalledOnce() + unlockLambda.assertions().isNeverCalled() + + advanceUntilIdle() + + // After handling the push, the wakelock should still not be unlocked + unlockLambda.assertions().isNeverCalled() + } + + @Test + fun `test pushHandler returning false locks and unlocks the wakelock early`() = runTest { + val lockLambda = lambdaRecorder { _ -> } + val unlockLambda = lambdaRecorder { } + val vectorFirebaseMessagingService = createVectorFirebaseMessagingService( + pushHandler = FakePushHandler(handleResult = { _, _ -> false }), + pushHandlingWakeLock = FakePushHandlingWakeLock( + lock = lockLambda, + unlock = unlockLambda + ) + ) + vectorFirebaseMessagingService.onMessageReceived( + message = RemoteMessage( + Bundle().apply { + putString("event_id", AN_EVENT_ID.value) + putString("room_id", A_ROOM_ID.value) + putString("cs", A_SECRET) + }, + ) + ) + + // The wakelock should be locked but not unlocked + lockLambda.assertions().isCalledOnce() + unlockLambda.assertions().isNeverCalled() + + advanceUntilIdle() + + // After handling the push, the wakelock should be unlocked + unlockLambda.assertions().isCalledOnce() + } + @Test fun `test new token is forwarded to the handler`() = runTest { val lambda = lambdaRecorder { } diff --git a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt index 59a2f654dd..363400ba13 100644 --- a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt +++ b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt @@ -71,11 +71,17 @@ class VectorUnifiedPushMessagingReceiver : MessagingReceiver() { providerInfo = "${UnifiedPushConfig.NAME} - $instance", data = String(message.content), ) + pushHandlingWakeLock.unlock() } else { - pushHandler.handle( + val handled = pushHandler.handle( pushData = pushData, providerInfo = "${UnifiedPushConfig.NAME} - $instance", ) + + // If we failed to handle the push, we should release the wakelock early to avoid keeping the device awake for too long. + if (!handled) { + pushHandlingWakeLock.unlock() + } } } } diff --git a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiverTest.kt b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiverTest.kt index 10de44d4ff..ef81c647b3 100644 --- a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiverTest.kt +++ b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiverTest.kt @@ -39,6 +39,7 @@ import org.unifiedpush.android.connector.FailedReason import org.unifiedpush.android.connector.data.PublicKeySet import org.unifiedpush.android.connector.data.PushEndpoint import org.unifiedpush.android.connector.data.PushMessage +import kotlin.time.Duration @RunWith(RobolectricTestRunner::class) class VectorUnifiedPushMessagingReceiverTest { @@ -76,7 +77,7 @@ class VectorUnifiedPushMessagingReceiverTest { @Test fun `onMessage valid invokes the push handler`() = runTest { val context = InstrumentationRegistry.getInstrumentation().context - val pushHandlerResult = lambdaRecorder { _, _ -> } + val pushHandlerResult = lambdaRecorder { _, _ -> true } val vectorUnifiedPushMessagingReceiver = createVectorUnifiedPushMessagingReceiver( pushHandler = FakePushHandler( handleResult = pushHandlerResult @@ -101,6 +102,60 @@ class VectorUnifiedPushMessagingReceiverTest { ) } + @Test + fun `pushHandler returning true locks the wake lock but does not unlock it so it continues to run`() = runTest { + val context = InstrumentationRegistry.getInstrumentation().context + val pushHandlerResult = lambdaRecorder { _, _ -> true } + val lockLambda = lambdaRecorder { _ -> } + val unlockLambda = lambdaRecorder { } + val vectorUnifiedPushMessagingReceiver = createVectorUnifiedPushMessagingReceiver( + pushHandler = FakePushHandler( + handleResult = pushHandlerResult + ), + pushHandlingWakeLock = FakePushHandlingWakeLock( + lock = lockLambda, + unlock = unlockLambda, + ), + ) + vectorUnifiedPushMessagingReceiver.onMessage(context, aPushMessage(), A_SECRET) + + // The wakelock should be locked but not unlocked, so it should continue to run + lockLambda.assertions().isCalledOnce() + unlockLambda.assertions().isNeverCalled() + + advanceUntilIdle() + + // After waiting for any possible timeout, the lock is still locked + unlockLambda.assertions().isNeverCalled() + } + + @Test + fun `pushHandler returning false locks and unlocks the wakelock early`() = runTest { + val context = InstrumentationRegistry.getInstrumentation().context + val pushHandlerResult = lambdaRecorder { _, _ -> false } + val lockLambda = lambdaRecorder { _ -> } + val unlockLambda = lambdaRecorder { } + val vectorUnifiedPushMessagingReceiver = createVectorUnifiedPushMessagingReceiver( + pushHandler = FakePushHandler( + handleResult = pushHandlerResult + ), + pushHandlingWakeLock = FakePushHandlingWakeLock( + lock = lockLambda, + unlock = unlockLambda, + ), + ) + vectorUnifiedPushMessagingReceiver.onMessage(context, aPushMessage(), A_SECRET) + + // The wakelock should be locked but not unlocked, so it should continue to run + lockLambda.assertions().isCalledOnce() + unlockLambda.assertions().isNeverCalled() + + advanceUntilIdle() + + // After waiting for a bit, the lock should have been unlocked since the handler returned false + unlockLambda.assertions().isCalledOnce() + } + @Test fun `onMessage invalid invokes the push handler invalid method`() = runTest { val context = InstrumentationRegistry.getInstrumentation().context