Fixing `IllegalStateException: You should authenticate before using this` when restoring app #6710

ouchadam · 2022-08-01T14:46:04Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Always attempts to reinitialise the Session instance if we have a cached authenticated session.

Motivation and context

Fixes #6709 - crash when launching the app

Realm migration bg #6548 allowed the realm database migration to occur off the main thread but had the condition that the session must be initialised via the MainActivity, however we can't guarantee this screen will appear first as the system may attempt to restore other screens directly along with the Application at the same time

Screenshots / GIFs

*With a manual process restart after 5 seconds~

Before	After

Tests

Add the following block to HomeActivity to force the process to be restarted with the current launch intent

fun onCreate() {
  Handler(Looper.getMainLooper()).postDelayed(Runnable {
      triggerRebirth(this@HomeActivity)
  }, 10000)
}

fun triggerRebirth() {
    val intent = Intent(this, HomeActivity::class.java)
            .apply {
                addFlags(Intent.FLAG_ACTIVITY_NEW_TASK and Intent.FLAG_ACTIVITY_CLEAR_TASK)
                putExtra(Mavericks.KEY_ARG, HomeActivityArgs(
                        clearNotification = true,
                        authenticationDescription = null,
                ))
            }
    startActivity(intent)
    Runtime.getRuntime().exit(0)
}

Notice the app crashes

Tested devices

Physical
Emulator
OS version(s):

… to attempt to restore session when they're not yet created in memory

ouchadam · 2022-08-01T14:48:07Z

vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt

    }

    fun getSafeActiveSession(): Session? {
-        return activeSessionReference.get()
+        return runBlocking { getOrInitializeSession(startSync = true) }


this is the crash fix, we'll always attempt to initialise the session (including perform a migration if needed)

the hope is that most users will only perform a migration via the MainActivity flow (tapping the launcher icon/notification) which will give them a loading bar, however it's possible the ROMs/OEMs can restore the activity and application directly

ouchadam · 2022-08-01T14:51:32Z

vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt

+     * @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) {


the initialisation is thread safe and will block or wait for any in progress initialisers to complete, this means that ideally all access to ActiveSessionHolder should be off the main thread to avoid possible ANRs, however this is quite a large refactor

the most common flow will be via the MainActivity which is off the main thread

Maybe safer to use a semaphore instead of a single thread dispatcher?

will give it a try 👍

works well! 💯 9114630

ouchadam · 2022-08-01T14:52:05Z

vector/src/main/java/im/vector/app/core/pushers/VectorMessagingReceiver.kt

@@ -204,7 +198,7 @@ class VectorMessagingReceiver : MessagingReceiver() {
        }
    }

-    private fun getEventFastLane(session: Session, pushData: PushData) {
+    private suspend fun getEventFastLane(session: Session, pushData: PushData) {


reuses the suspension

ouchadam · 2022-08-01T14:54:52Z

vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt

@@ -80,18 +87,27 @@ class ActiveSessionHolder @Inject constructor(
    }

    fun hasActiveSession(): Boolean {
-        return activeSessionReference.get() != null
+        return activeSessionReference.get() != null || authenticationService.hasAuthenticatedSessions()


an active session is now a in memory instance or cached authenticated session

this should become async with the hasAuthenticatedSessions being on the IO dispatcher, however as the active session is cached in memory almost straight away in most cases I've opt'd to avoid the breaking change (will hopefully be included in the realm-kotlin changes! 🤞 )

ouchadam · 2022-08-01T14:56:30Z

vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt

                ?: throw IllegalStateException("You should authenticate before using this")
    }

+    suspend fun getOrInitializeSession(startSync: Boolean): Session? {
+        return activeSessionReference.get() ?: sessionInitializer.tryInitialize(readCurrentSession = { activeSessionReference.get() }) { session ->


we're eagerly reading from the in-memory cache to avoid a potential thread switch but it has the side effect of needing to query the in-memory value again within the initializer thread

ouchadam · 2022-08-01T15:00:15Z

vector/src/main/java/im/vector/app/core/di/ActiveSessionHolder.kt

    }

    fun getSafeActiveSession(): Session? {
-        return activeSessionReference.get()
+        return runBlocking { getOrInitializeSession(startSync = true) }


ideally getActiveSession and getSafeActiveSession would be suspending instead of using runBlocking however the change needed is much bigger than the scope of this PR

ganfra

Some small remarks otherwise LGTM

ganfra · 2022-08-01T15:05:52Z

vector/src/main/java/im/vector/app/features/start/StartAppViewModel.kt

@@ -55,11 +55,15 @@ class StartAppViewModel @AssistedInject constructor(
        handleLongProcessing()
        viewModelScope.launch(Dispatchers.IO) {


Can you take care of inject Dispatchers instead of using directly IO?

will do 👍

ganfra · 2022-08-01T15:09:30Z

vector/src/main/java/im/vector/app/core/di/SessionInitializer.kt

+     * @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) {


Maybe safer to use a semaphore instead of a single thread dispatcher?

- avoids the need for a dedicated long living thread instance

ganfra

All good, thanks!

sonarqubecloud · 2022-08-01T17:37:16Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

0.0% Coverage
0.0% Duplication

ouchadam added 4 commits August 1, 2022 13:11

creating a dedicated threadsafe Session instance initializer in order…

fedbe04

… to attempt to restore session when they're not yet created in memory

reusing the suspend scope

410a7b5

avoiding unneeded main context switch

6b754f3

adding changelog entry

dbe5b35

ouchadam added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Aug 1, 2022

ouchadam commented Aug 1, 2022

View reviewed changes

ouchadam added the Z-Crash label Aug 1, 2022

ouchadam commented Aug 1, 2022

View reviewed changes

ouchadam marked this pull request as ready for review August 1, 2022 14:57

ouchadam requested review from a team and ganfra and removed request for a team August 1, 2022 14:57

ouchadam commented Aug 1, 2022

View reviewed changes

ganfra reviewed Aug 1, 2022

View reviewed changes

ouchadam added 2 commits August 1, 2022 16:14

using injectable dispatchers instead of direct usage

3725921

replacing single context thread with semaphore

9114630

- avoids the need for a dedicated long living thread instance

ganfra approved these changes Aug 1, 2022

View reviewed changes

ouchadam merged commit cf5745e into hotfix/v1.4.31 Aug 1, 2022

ouchadam deleted the feature/adm/always-initialise-session branch August 1, 2022 16:49

bmarty mentioned this pull request Aug 19, 2022

Ensure sync thread is started #6884

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing `IllegalStateException: You should authenticate before using this` when restoring app #6710

Fixing `IllegalStateException: You should authenticate before using this` when restoring app #6710

ouchadam commented Aug 1, 2022

ouchadam Aug 1, 2022

ouchadam Aug 1, 2022 •

edited

Loading

ganfra Aug 1, 2022

ouchadam Aug 1, 2022

ouchadam Aug 1, 2022

ouchadam Aug 1, 2022

ouchadam Aug 1, 2022

ouchadam Aug 1, 2022

ouchadam Aug 1, 2022

ganfra left a comment

ganfra Aug 1, 2022

ouchadam Aug 1, 2022

ouchadam Aug 1, 2022

ganfra Aug 1, 2022

ganfra left a comment

sonarqubecloud bot commented Aug 1, 2022

		@@ -55,11 +55,15 @@ class StartAppViewModel @AssistedInject constructor(
		handleLongProcessing()
		viewModelScope.launch(Dispatchers.IO) {

Fixing IllegalStateException: You should authenticate before using this when restoring app #6710

Fixing IllegalStateException: You should authenticate before using this when restoring app #6710

Conversation

ouchadam commented Aug 1, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Choose a reason for hiding this comment

ouchadam Aug 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganfra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganfra left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Aug 1, 2022

Fixing `IllegalStateException: You should authenticate before using this` when restoring app #6710

Fixing `IllegalStateException: You should authenticate before using this` when restoring app #6710

ouchadam Aug 1, 2022 •

edited

Loading