[HOLD for payment 2024-11-20] Investigate infinite credentials Onyx loading state when migrating SignInPage to useOnyx

@blazejkustra

When migrating SignInPage.tsx to useOnyx for testing purposes, @blazejkustra noticed that the screen becomes blank when logging out the user. Refreshing the page solves the problem.

It seems that the credentials data returned by useOnyx is always on loading state, so if we put the isLoadingOnyxValue condition to only render the page when all Onyx data is available the page will never render.

Screen.Recording.2024-09-06.at.13.42.35-compressed.mov

We must investigate why this is happening as it could be a bug in Onyx internals.

Update: Still investigating it, it seems a problem around the cache / Onyx.clear()

Update: I got back to this issue today and I was able to replicate through an new unit test, will investigate and work in the solution now.

Update: Still looking for a solution to solve the racing condition around the connection / cache / Onyx.clear()

Update: Working on and testing a possible solution I found today.

I found another example for this when working on #49103, here is a diff for you to test it:

diff --git a/src/libs/Navigation/AppNavigator/AuthScreens.tsx b/src/libs/Navigation/AppNavigator/AuthScreens.tsx
index ccc8290a93c..a4d78926558 100644
--- a/src/libs/Navigation/AppNavigator/AuthScreens.tsx
+++ b/src/libs/Navigation/AppNavigator/AuthScreens.tsx
@@ -1,10 +1,11 @@
 import React, {memo, useEffect, useMemo, useRef, useState} from 'react';
 import {View} from 'react-native';
 import type {OnyxEntry} from 'react-native-onyx';
-import Onyx, {withOnyx} from 'react-native-onyx';
+import Onyx, {useOnyx, withOnyx} from 'react-native-onyx';
 import type {ValueOf} from 'type-fest';
 import ActiveGuidesEventListener from '@components/ActiveGuidesEventListener';
 import ComposeProviders from '@components/ComposeProviders';
+import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
 import OptionsListContextProvider from '@components/OptionListContextProvider';
 import {SearchContextProvider} from '@components/Search/SearchContext';
 import SearchRouter from '@components/Search/SearchRouter/SearchRouter';
@@ -51,6 +52,7 @@ import SCREENS from '@src/SCREENS';
 import type * as OnyxTypes from '@src/types/onyx';
 import type {SelectedTimezone, Timezone} from '@src/types/onyx/PersonalDetails';
 import {isEmptyObject} from '@src/types/utils/EmptyObject';
+import isLoadingOnyxValue from '@src/types/utils/isLoadingOnyxValue';
 import type ReactComponentModule from '@src/types/utils/ReactComponentModule';
 import beforeRemoveReportOpenedFromSearchRHP from './beforeRemoveReportOpenedFromSearchRHP';
 import CENTRAL_PANE_SCREENS from './CENTRAL_PANE_SCREENS';
@@ -588,14 +590,27 @@ AuthScreens.displayName = 'AuthScreens';
 
 const AuthScreensMemoized = memo(AuthScreens, () => true);
 
-export default withOnyx<AuthScreensProps, AuthScreensProps>({
-    session: {
-        key: ONYXKEYS.SESSION,
-    },
-    lastOpenedPublicRoomID: {
-        key: ONYXKEYS.LAST_OPENED_PUBLIC_ROOM_ID,
-    },
-    initialLastUpdateIDAppliedToClient: {
-        key: ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT,
-    },
-})(AuthScreensMemoized);
+export default function AuthScreensWithOnyx() {
+    const [session, sessionStatus] = useOnyx(ONYXKEYS.SESSION);
+    const [lastOpenedPublicRoomID, lastOpenedPublicRoomIDStatus] = useOnyx(ONYXKEYS.LAST_OPENED_PUBLIC_ROOM_ID);
+    const [initialLastUpdateIDAppliedToClient, initialLastUpdateIDAppliedToClientStatus] = useOnyx(ONYXKEYS.ONYX_UPDATES_LAST_UPDATE_ID_APPLIED_TO_CLIENT);
+
+    console.log(
+        `sessionStatus, lastOpenedPublicRoomIDStatus, initialLastUpdateIDAppliedToClientStatus = `,
+        sessionStatus,
+        lastOpenedPublicRoomIDStatus,
+        initialLastUpdateIDAppliedToClientStatus,
+    );
+
+    if (isLoadingOnyxValue(sessionStatus, lastOpenedPublicRoomIDStatus, initialLastUpdateIDAppliedToClientStatus)) {
+        return <FullScreenLoadingIndicator />;
+    }
+
+    return (
+        <AuthScreensMemoized
+            session={session}
+            lastOpenedPublicRoomID={lastOpenedPublicRoomID}
+            initialLastUpdateIDAppliedToClient={initialLastUpdateIDAppliedToClient}
+        />
+    );
+}

The outcome is that there is an indefinite loading because one of the keys is in constant loading state:

Screen.Recording.2024-10-01.at.09.17.33.mov

@blazejkustra

Thanks for this report @blazejkustra , I will look into it tomorrow because I'm not sure if my fix will cover this and maybe the problem is a broader one than expected.

@fabioh8010

@fabioh8010 Any updates?

@blazejkustra

Hi @blazejkustra, sorry for the late here.

After cracking my head over these issues for some time, here's my findings:

Case 1

Problem

Racing conditions between an Onyx.clear() execution and useOnyx subscription causes useOnyx to be always in loading state and never fully resolve its first connection.

This happens because we clear the cache during Onyx.clear() execution and if we are connecting to some key with useOnyx at the same time, there is no cache where the hook can look into (OnyxCache.hasCacheForKey(key) will be false) and it get stuck in loading state.

From what I've seen this problem doesn't happen for withOnyx basically because the racing condition doesn't occur (maybe because it's a component and it's slightly slower during initialisation?).

Solution

No definitive solutions yet. I first tried to use OnyxCache.captureTask() in Onyx.clear() to "register" this ongoing task and later I would make the subscription / useOnyx processes wait until clearing is done to proceed with the process, but I'm not sure if it's a good solution.

EDIT: I came with a similar but simpler solution in the Draft PR.

Case 2

Problem

The cache clearing performed by Onyx.clear() combined with the connection reusing logic we currently have causes an useOnyx connection to a key that was connected before to be always loading state and never fully resolve its first connection.

Specifically, if we are reusing the connection of a key that is still exists in the connection manager after an Onyx.clear() execution, the next useOnyx to connect to that same key gets stuck in the loading state as the cache doesn't really exist anymore, and we didn't populate it again because we are reusing the connection.

When logging out a lot of Onyx connections will be removed from the connection manager as expected, but some of them persists because they don't belong to the React's lifecycle e.g. any Onyx.connect in our lib files, helping create the issue described above.

This problem doesn't happen for withOnyx because the HOC doesn't reuse connections and create new ones upon every subscription, thus populating the cache again for that key and allowing the HOC to work as expected.

Solution

I first tried to "signal" the connection manager after Onyx.clear() that it should refresh its own connections (by temporally subscribing to the key and updating the cached value) when a subscriber was going to reuse them again, but it led to many bugs and overall it looked like a bad / unpredictable solution.

My second and current approach is to introduce the concept of "Session ID" to the connection manager. This Session ID will be used when generating the Connection IDs with generateConnectionID() and we would generate another Session ID after an Onyx.clear() call. In this way, if a subscriber tries to connect to a Onyx key that persisted after Onyx.clear() it would result in a separate connection instead of reusing the connection from the "previous session".

Not sure if it's the best approach but it solves this case for now. Draft PR of the solution: Expensify/react-native-onyx#588

I will continue exploring for other solutions and also explore if we can solve this at the cache's side. I will reach out Margelo's team eventually too as they have more context about the cache system and can suggest better solutions.

Update: I've designed a similar but simpler solution for Case 1, tomorrow will open the PR and ask for review

Update: Asked for more eyes here. I will proceed into opening the PR to Exfy review next week and add screenshots / videos

PR was opened to review

Reviewing label has been removed, please complete the "BugZero Checklist".

@fabioh8010

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.60-3 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

Bump Onyx to v2.0.78 #51942

If no regressions arise, payment will be issued on 2024-11-20. 🎊

For reference, here are some details about the assignees on this issue:

@fabioh8010 does not require payment (Contractor)

Skipping the payment summary for this issue since all the assignees are employees or vendors. If this is incorrect, please manually add the payment summary SO.

@c3024

Seems maybe @c3024 requires payment here before we can close? I'm gonna add the Bug label here.

@bfitzexpensify

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@c3024

Offer sent @c3024

And now paid out - let's close this out.

Navigation Menu

Repository navigation

[HOLD for payment 2024-11-20] Investigate infinite credentials Onyx loading state when migrating SignInPage to useOnyx #48725

Case 1

Problem

Solution

Case 2

Problem

Solution

9 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[HOLD for payment 2024-11-20] Investigate infinite credentials Onyx loading state when migrating SignInPage to useOnyx #48725

Description

Activity

fabioh8010 commented on Sep 11, 2024

fabioh8010 commented on Sep 18, 2024

fabioh8010 commented on Sep 20, 2024

fabioh8010 commented on Sep 26, 2024

blazejkustra commented on Oct 1, 2024

fabioh8010 commented on Oct 1, 2024

blazejkustra commented on Oct 3, 2024

fabioh8010 commented on Oct 8, 2024

Case 1

Problem

Solution

Case 2

Problem

Solution

fabioh8010 commented on Oct 14, 2024

fabioh8010 commented on Oct 18, 2024

fabioh8010 commented on Oct 22, 2024

9 remaining items

melvin-bot commented on Nov 13, 2024

melvin-bot commented on Nov 13, 2024

melvin-bot commented on Nov 20, 2024

marcaaron commented on Nov 21, 2024

melvin-bot commented on Nov 21, 2024

bfitzexpensify commented on Nov 22, 2024

bfitzexpensify commented on Nov 22, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions