From fedbe048ba966d586c87102866efb927d997cc23 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 1 Aug 2022 13:11:19 +0100 Subject: [PATCH 1/6] creating a dedicated threadsafe Session instance initializer in order to attempt to restore session when they're not yet created in memory --- .../vector/app/core/di/ActiveSessionHolder.kt | 30 +++++++++-- .../vector/app/core/di/ActiveSessionSetter.kt | 40 -------------- .../vector/app/core/di/SessionInitializer.kt | 53 +++++++++++++++++++ .../core/pushers/VectorMessagingReceiver.kt | 14 ++--- .../app/features/start/StartAppViewModel.kt | 12 +++-- 5 files changed, 91 insertions(+), 58 deletions(-) delete mode 100644 vector/src/main/java/im/vector/app/core/di/ActiveSessionSetter.kt create mode 100644 vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt diff --git a/vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt b/vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt index 21b4e287c6..4effc581cb 100644 --- a/vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt +++ b/vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt @@ -16,8 +16,10 @@ package im.vector.app.core.di +import android.content.Context import arrow.core.Option import im.vector.app.ActiveSessionDataSource +import im.vector.app.core.extensions.configureAndStart import im.vector.app.core.pushers.UnifiedPushHelper import im.vector.app.core.services.GuardServiceStarter import im.vector.app.features.call.webrtc.WebRtcCallManager @@ -25,6 +27,12 @@ import im.vector.app.features.crypto.keysrequest.KeyRequestHandler import im.vector.app.features.crypto.verification.IncomingVerificationRequestHandler import im.vector.app.features.notifications.PushRuleTriggerListener import im.vector.app.features.session.SessionListener +import kotlinx.coroutines.DelicateCoroutinesApi +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.newSingleThreadContext +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withContext +import org.matrix.android.sdk.api.auth.AuthenticationService import org.matrix.android.sdk.api.session.Session import timber.log.Timber import java.util.concurrent.atomic.AtomicReference @@ -41,7 +49,10 @@ class ActiveSessionHolder @Inject constructor( private val sessionListener: SessionListener, private val imageManager: ImageManager, private val unifiedPushHelper: UnifiedPushHelper, - private val guardServiceStarter: GuardServiceStarter + private val guardServiceStarter: GuardServiceStarter, + private val sessionInitializer: SessionInitializer, + private val applicationContext: Context, + private val authenticationService: AuthenticationService, ) { private var activeSessionReference: AtomicReference = AtomicReference() @@ -80,18 +91,29 @@ class ActiveSessionHolder @Inject constructor( } fun hasActiveSession(): Boolean { - return activeSessionReference.get() != null + return activeSessionReference.get() != null || authenticationService.hasAuthenticatedSessions() } fun getSafeActiveSession(): Session? { - return activeSessionReference.get() + return runBlocking { getOrInitializeSession(startSync = true) } } fun getActiveSession(): Session { - return activeSessionReference.get() + return getSafeActiveSession() ?: throw IllegalStateException("You should authenticate before using this") } + suspend fun getOrInitializeSession(startSync: Boolean): Session? { + return activeSessionReference.get() ?: sessionInitializer.tryInitialize(readCurrentSession = { activeSessionReference.get() }) { session -> + setActiveSession(session) + withContext(Dispatchers.Main) { + session.configureAndStart(applicationContext, startSyncing = startSync) + } + } + } + + fun isWaitingForSessionInitialization() = activeSessionReference.get() == null && authenticationService.hasAuthenticatedSessions() + // TODO Stop sync ? // fun switchToSession(sessionParams: SessionParams) { // val newActiveSession = authenticationService.getSession(sessionParams) diff --git a/vector/src/main/java/im/vector/app/core/di/ActiveSessionSetter.kt b/vector/src/main/java/im/vector/app/core/di/ActiveSessionSetter.kt deleted file mode 100644 index 09479a230f..0000000000 --- a/vector/src/main/java/im/vector/app/core/di/ActiveSessionSetter.kt +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright 2022 New Vector Ltd - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package im.vector.app.core.di - -import android.content.Context -import im.vector.app.core.extensions.configureAndStart -import org.matrix.android.sdk.api.auth.AuthenticationService -import javax.inject.Inject - -class ActiveSessionSetter @Inject constructor( - private val activeSessionHolder: ActiveSessionHolder, - private val authenticationService: AuthenticationService, - private val applicationContext: Context, -) { - fun shouldSetActionSession(): Boolean { - return authenticationService.hasAuthenticatedSessions() && !activeSessionHolder.hasActiveSession() - } - - fun tryToSetActiveSession(startSync: Boolean) { - if (shouldSetActionSession()) { - val lastAuthenticatedSession = authenticationService.getLastAuthenticatedSession()!! - activeSessionHolder.setActiveSession(lastAuthenticatedSession) - lastAuthenticatedSession.configureAndStart(applicationContext, startSyncing = startSync) - } - } -} diff --git a/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt b/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt new file mode 100644 index 0000000000..1413d275f2 --- /dev/null +++ b/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2022 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package im.vector.app.core.di + +import kotlinx.coroutines.DelicateCoroutinesApi +import kotlinx.coroutines.newSingleThreadContext +import kotlinx.coroutines.withContext +import org.matrix.android.sdk.api.auth.AuthenticationService +import org.matrix.android.sdk.api.session.Session +import javax.inject.Inject + +@OptIn(DelicateCoroutinesApi::class) +private val INITIALIZER_CONTEXT = newSingleThreadContext("session-initializer") + +class SessionInitializer @Inject constructor( + private val authenticationService: AuthenticationService, +) { + + /** + * A thread safe way to initialize the last authenticated Session instance. + * + * @param readCurrentSession expects an in-memory Session to be provided or null if not yet set. + * @param initializer callback to allow additional initialization on the Session, such as setting the in-memory Session instance. + * @return the initialized Session or null when no authenticated sessions are available. + */ + suspend fun tryInitialize(readCurrentSession: () -> Session?, initializer: suspend (Session) -> Unit): Session? { + return withContext(INITIALIZER_CONTEXT) { + val currentInMemorySession = readCurrentSession() + when { + currentInMemorySession != null -> currentInMemorySession + authenticationService.hasAuthenticatedSessions() -> { + val lastAuthenticatedSession = authenticationService.getLastAuthenticatedSession()!! + lastAuthenticatedSession.also { initializer(lastAuthenticatedSession) } + } + else -> null + } + } + } +} diff --git a/vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt b/vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt index be84dfeaba..9d652986e6 100644 --- a/vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt +++ b/vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt @@ -27,7 +27,6 @@ import androidx.localbroadcastmanager.content.LocalBroadcastManager import dagger.hilt.android.AndroidEntryPoint import im.vector.app.BuildConfig import im.vector.app.core.di.ActiveSessionHolder -import im.vector.app.core.di.ActiveSessionSetter import im.vector.app.core.network.WifiDetector import im.vector.app.core.pushers.model.PushData import im.vector.app.core.services.GuardServiceStarter @@ -38,6 +37,7 @@ import im.vector.app.features.settings.BackgroundSyncMode import im.vector.app.features.settings.VectorDataStore import im.vector.app.features.settings.VectorPreferences import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking @@ -60,7 +60,6 @@ class VectorMessagingReceiver : MessagingReceiver() { @Inject lateinit var notificationDrawerManager: NotificationDrawerManager @Inject lateinit var notifiableEventResolver: NotifiableEventResolver @Inject lateinit var pushersManager: PushersManager - @Inject lateinit var activeSessionSetter: ActiveSessionSetter @Inject lateinit var activeSessionHolder: ActiveSessionHolder @Inject lateinit var vectorPreferences: VectorPreferences @Inject lateinit var vectorDataStore: VectorDataStore @@ -116,7 +115,7 @@ class VectorMessagingReceiver : MessagingReceiver() { // we are in foreground, let the sync do the things? Timber.tag(loggerTag.value).d("PUSH received in a foreground state, ignore") } else { - onMessageReceivedInternal(pushData) + coroutineScope.launch(Dispatchers.IO) { onMessageReceivedInternal(pushData) } } } } @@ -170,7 +169,7 @@ class VectorMessagingReceiver : MessagingReceiver() { * * @param pushData Object containing message data. */ - private fun onMessageReceivedInternal(pushData: PushData) { + private suspend fun onMessageReceivedInternal(pushData: PushData) { try { if (BuildConfig.LOW_PRIVACY_LOG_ENABLE) { Timber.tag(loggerTag.value).d("## onMessageReceivedInternal() : $pushData") @@ -178,12 +177,7 @@ class VectorMessagingReceiver : MessagingReceiver() { Timber.tag(loggerTag.value).d("## onMessageReceivedInternal()") } - val session = activeSessionHolder.getSafeActiveSession() - ?: run { - // Active session may not exist yet, if MainActivity has not been launched - activeSessionSetter.tryToSetActiveSession(startSync = false) - activeSessionHolder.getSafeActiveSession() - } + val session = activeSessionHolder.getOrInitializeSession(startSync = false) if (session == null) { Timber.tag(loggerTag.value).w("## Can't sync from push, no current session") diff --git a/vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt b/vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt index 62a7517f5a..e4ddf647c8 100644 --- a/vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt @@ -20,7 +20,7 @@ import com.airbnb.mvrx.MavericksViewModelFactory import dagger.assisted.Assisted import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject -import im.vector.app.core.di.ActiveSessionSetter +import im.vector.app.core.di.ActiveSessionHolder import im.vector.app.core.di.MavericksAssistedViewModelFactory import im.vector.app.core.di.hiltMavericksViewModelFactory import im.vector.app.core.platform.VectorViewModel @@ -31,7 +31,7 @@ import kotlin.time.Duration.Companion.seconds class StartAppViewModel @AssistedInject constructor( @Assisted val initialState: StartAppViewState, - private val activeSessionSetter: ActiveSessionSetter, + private val sessionHolder: ActiveSessionHolder, ) : VectorViewModel(initialState) { @AssistedFactory @@ -42,7 +42,7 @@ class StartAppViewModel @AssistedInject constructor( companion object : MavericksViewModelFactory by hiltMavericksViewModelFactory() fun shouldStartApp(): Boolean { - return activeSessionSetter.shouldSetActionSession() + return sessionHolder.isWaitingForSessionInitialization() } override fun handle(action: StartAppAction) { @@ -55,11 +55,15 @@ class StartAppViewModel @AssistedInject constructor( handleLongProcessing() viewModelScope.launch(Dispatchers.IO) { // This can take time because of DB migration(s), so do it in a background task. - activeSessionSetter.tryToSetActiveSession(startSync = true) + eagerlyInitializeSession() _viewEvents.post(StartAppViewEvent.AppStarted) } } + private suspend fun eagerlyInitializeSession() { + sessionHolder.getOrInitializeSession(startSync = true) + } + private fun handleLongProcessing() { viewModelScope.launch(Dispatchers.Default) { delay(1.seconds.inWholeMilliseconds) From 410a7b525f417071016befa2a6d0ea526914dcb2 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 1 Aug 2022 13:13:00 +0100 Subject: [PATCH 2/6] reusing the suspend scope --- .../core/pushers/VectorMessagingReceiver.kt | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt b/vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt index 9d652986e6..0d6828f569 100644 --- a/vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt +++ b/vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt @@ -198,7 +198,7 @@ class VectorMessagingReceiver : MessagingReceiver() { } } - private fun getEventFastLane(session: Session, pushData: PushData) { + private suspend fun getEventFastLane(session: Session, pushData: PushData) { pushData.roomId ?: return pushData.eventId ?: return @@ -212,18 +212,16 @@ class VectorMessagingReceiver : MessagingReceiver() { return } - coroutineScope.launch { - Timber.tag(loggerTag.value).d("Fast lane: start request") - val event = tryOrNull { session.eventService().getEvent(pushData.roomId, pushData.eventId) } ?: return@launch + Timber.tag(loggerTag.value).d("Fast lane: start request") + val event = tryOrNull { session.eventService().getEvent(pushData.roomId, pushData.eventId) } ?: return - val resolvedEvent = notifiableEventResolver.resolveInMemoryEvent(session, event, canBeReplaced = true) + val resolvedEvent = notifiableEventResolver.resolveInMemoryEvent(session, event, canBeReplaced = true) - resolvedEvent - ?.also { Timber.tag(loggerTag.value).d("Fast lane: notify drawer") } - ?.let { - notificationDrawerManager.updateEvents { it.onNotifiableEventReceived(resolvedEvent) } - } - } + resolvedEvent + ?.also { Timber.tag(loggerTag.value).d("Fast lane: notify drawer") } + ?.let { + notificationDrawerManager.updateEvents { it.onNotifiableEventReceived(resolvedEvent) } + } } // check if the event was not yet received From 6b754f3e03ef71322fd745b517c15bcdcea95e29 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 1 Aug 2022 15:12:39 +0100 Subject: [PATCH 3/6] avoiding unneeded main context switch --- .../java/im/vector/app/core/di/ActiveSessionHolder.kt | 8 +------- .../main/java/im/vector/app/core/di/SessionInitializer.kt | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt b/vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt index 4effc581cb..7a1d613ab9 100644 --- a/vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt +++ b/vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt @@ -27,11 +27,7 @@ import im.vector.app.features.crypto.keysrequest.KeyRequestHandler import im.vector.app.features.crypto.verification.IncomingVerificationRequestHandler import im.vector.app.features.notifications.PushRuleTriggerListener import im.vector.app.features.session.SessionListener -import kotlinx.coroutines.DelicateCoroutinesApi -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.newSingleThreadContext import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.withContext import org.matrix.android.sdk.api.auth.AuthenticationService import org.matrix.android.sdk.api.session.Session import timber.log.Timber @@ -106,9 +102,7 @@ class ActiveSessionHolder @Inject constructor( suspend fun getOrInitializeSession(startSync: Boolean): Session? { return activeSessionReference.get() ?: sessionInitializer.tryInitialize(readCurrentSession = { activeSessionReference.get() }) { session -> setActiveSession(session) - withContext(Dispatchers.Main) { - session.configureAndStart(applicationContext, startSyncing = startSync) - } + session.configureAndStart(applicationContext, startSyncing = startSync) } } diff --git a/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt b/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt index 1413d275f2..c800ae5118 100644 --- a/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt +++ b/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt @@ -37,7 +37,7 @@ class SessionInitializer @Inject constructor( * @param initializer callback to allow additional initialization on the Session, such as setting the in-memory Session instance. * @return the initialized Session or null when no authenticated sessions are available. */ - suspend fun tryInitialize(readCurrentSession: () -> Session?, initializer: suspend (Session) -> Unit): Session? { + suspend fun tryInitialize(readCurrentSession: () -> Session?, initializer: (Session) -> Unit): Session? { return withContext(INITIALIZER_CONTEXT) { val currentInMemorySession = readCurrentSession() when { From dbe5b35ad4dc1ed3adbc6538bd620a4e140094cc Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 1 Aug 2022 15:38:51 +0100 Subject: [PATCH 4/6] adding changelog entry --- changelog.d/6709.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/6709.bugfix diff --git a/changelog.d/6709.bugfix b/changelog.d/6709.bugfix new file mode 100644 index 0000000000..359e7d9d27 --- /dev/null +++ b/changelog.d/6709.bugfix @@ -0,0 +1 @@ +Fixes crash when returning to the app after backgrounding From 3725921400631977c5fac483b18d25d10da84347 Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 1 Aug 2022 16:14:49 +0100 Subject: [PATCH 5/6] using injectable dispatchers instead of direct usage --- .../java/im/vector/app/features/start/StartAppViewModel.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt b/vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt index e4ddf647c8..4b2e7bef98 100644 --- a/vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt +++ b/vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt @@ -23,6 +23,7 @@ import dagger.assisted.AssistedInject import im.vector.app.core.di.ActiveSessionHolder import im.vector.app.core.di.MavericksAssistedViewModelFactory import im.vector.app.core.di.hiltMavericksViewModelFactory +import im.vector.app.core.dispatchers.CoroutineDispatchers import im.vector.app.core.platform.VectorViewModel import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.delay @@ -32,6 +33,7 @@ import kotlin.time.Duration.Companion.seconds class StartAppViewModel @AssistedInject constructor( @Assisted val initialState: StartAppViewState, private val sessionHolder: ActiveSessionHolder, + private val dispatchers: CoroutineDispatchers, ) : VectorViewModel(initialState) { @AssistedFactory @@ -53,7 +55,7 @@ class StartAppViewModel @AssistedInject constructor( private fun handleStartApp() { handleLongProcessing() - viewModelScope.launch(Dispatchers.IO) { + viewModelScope.launch(dispatchers.io) { // This can take time because of DB migration(s), so do it in a background task. eagerlyInitializeSession() _viewEvents.post(StartAppViewEvent.AppStarted) From 9114630bbaec9020de897d970d7da1e2e739ae8f Mon Sep 17 00:00:00 2001 From: Adam Brown Date: Mon, 1 Aug 2022 16:23:41 +0100 Subject: [PATCH 6/6] replacing single context thread with semaphore - avoids the need for a dedicated long living thread instance --- .../java/im/vector/app/core/di/SessionInitializer.kt | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt b/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt index c800ae5118..e1d4844940 100644 --- a/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt +++ b/vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt @@ -16,15 +16,13 @@ package im.vector.app.core.di -import kotlinx.coroutines.DelicateCoroutinesApi -import kotlinx.coroutines.newSingleThreadContext -import kotlinx.coroutines.withContext +import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.sync.withPermit import org.matrix.android.sdk.api.auth.AuthenticationService import org.matrix.android.sdk.api.session.Session import javax.inject.Inject -@OptIn(DelicateCoroutinesApi::class) -private val INITIALIZER_CONTEXT = newSingleThreadContext("session-initializer") +private val initializerSemaphore = Semaphore(permits = 1) class SessionInitializer @Inject constructor( private val authenticationService: AuthenticationService, @@ -38,7 +36,7 @@ class SessionInitializer @Inject constructor( * @return the initialized Session or null when no authenticated sessions are available. */ suspend fun tryInitialize(readCurrentSession: () -> Session?, initializer: (Session) -> Unit): Session? { - return withContext(INITIALIZER_CONTEXT) { + return initializerSemaphore.withPermit { val currentInMemorySession = readCurrentSession() when { currentInMemorySession != null -> currentInMemorySession