Android - User is logged out randomly #5619
Comments
|
Triggered auto assignment to @marcochavezf ( |
|
I wasn't able to reproduce this issue yet, but some of the ideas to try are (after reading some of the comments of the Slack thread):
|
|
@marcochavezf Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
|
Seems an invalid
The log out happens when the
|
|
@marcochavezf Onyx would never call App/src/libs/actions/SignInRedirect.js Lines 22 to 27 in 04083b1 This is triggered on regular user log outs or cases like the one you've posted above where |
|
A very likely cause I think is See how Lines 11 to 15 in 04083b1 If we don't have it by the time Lines 267 to 268 in 04083b1 On my physical Android device I've seen delays of up to 3 sec. for retrieving a value from storage during app launch Can you trace server logs to try and confirm this - IMO we need to ensure that Onxy.get(ONYXKEYS.CREDENTIALS)
.then(({ autoGeneratedLogin, autoGeneratedPassword }) => Authenticate({
useExpensifyLogin: false,
partnerName: CONFIG.EXPENSIFY.PARTNER_NAME,
partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
autoGeneratedLogin,
autoGeneratedPassword,
authToken,
}));cc @marcaaron and @tgolen regarding There are 3 instances where we write session data to Onyx and could be overwriting something by mistake, I've traced them further and it doesn't seem likelysetSuccessfulSignInDataApp/src/libs/actions/Session.js Lines 37 to 43 in 04083b1 signInWithShortLivedTokenApp/src/libs/actions/Session.js Lines 252 to 260 in 04083b1 reauthenticateLines 278 to 283 in 04083b1 |
@kidroca you're right about this ^ but with
I think we'll need to ensure that both |
|
Looks like @johnmlee101 Experienced this issue (or a extremely similar one) in iOS Image.from.iOS.2.MOV |
Hmm, probably it's related but I'm not really sure. Today I will implement and test the proposed solution to avoid the race condition. Hopefully, that will also fix the issue in the video. |
|
Created a function to retrieve the
|
|
Unfortunately, I am going OOO for the next few days, but I'll be back next week and I would like to be involved with helping come up with a solution for this. As it stands right now, I have some concerns with the suggested solution.
I'm definitely still not on board the |
|
Yeah that change won't work reliably. The promise from The call for Lines 199 to 201 in 04083b1 And we see the request starts soon after the call. We either need to wait for Retrying would introduce a 1sec. delay due to how the network queue works enhanceParameters(queuedRequest.command, requestData)
.then(params => HttpUtils.xhr(req.command, params, req.type, req.shouldUseSecure)
.then(response => onResponse(req, response))
.catch(error => onError(req, error))vs Lines 199 to 213 in 04083b1 If for whatever reason Not sure if it's possible, but. I would prefer to throw an error here Lines 199 to 201 in 04083b1 If we rely on enhancer we should not have a fallback - falling back exposes another place where we can fail to add an auth token to a request that needs one, and endup with the user in a logged out state |
|
I'm also not certain about the authToken being the culprit - the user specific portion of the app mounts only when we have an AuthToken Line 138 in 9a69ff7 Then this component gets mounted and starts a bunch of requests: App/src/libs/Navigation/AppNavigator/AuthScreens.js Lines 132 to 135 in 2ae2fd8 But at that point we already have the token... The case where we start a request without a token is possible if we have some persisted requests that are initiated before the token is ready, but since they also have to be read from storage it's seems unlikely I still think it's the
A test that demonstrates how this should simulate an app launch where we have a logged in user, and storage being read slow enough to allow for the above to happen On slack we also talked about logging the issue to the server, so that we know what happened exactly. This should also review whether the issue occurred after a persisted request and any other circumstances |
@tgolen yeah agreed, after testing with
👍🏽 I will check how to add a unit test to demonstrate this, I think we can use a timeout like here to reproduce it in unit tests: #5619 (comment) Btw, is there a diagram of the login in the app initilization? (I can create a new one if it doesn't exist, I think it can help us to understand better the complexity of this flow). |
Thanks @kidroca I will look at My hypothesis of the Screen.Recording.2021-10-12.at.18.32.38.movIn the video I'm already logged in (I refreshed the browser just to verify everything is working as expected), then I toggle a piece of code that adds the The app returns the error Lines 199 to 210 in ddb3cbd where Lines 51 to 65 in 50beee9 |
|
I was thinking about using a flag to check if Onyx is ready (which doesn't mix async and sync operations and could be a little bit simpler) so I wanted to give it a try and test it. I created a PR (WIP) where I added the flag to check if Onyx is ready when the callback of I will be testing on an Android device/simulator and check if I can fix the |
|
@marcochavezf Whoops! This issue is 2 days overdue. Let's get this updated quick! |
I have all the ideas about how to fix them 🙃 What problem are you running into specifically? |
Sorry I have only half been following this conversation and there's a lot of context I'd need to catch up on. If I reviewed the code now I'd probably say the So, I think we should separate the concern of whether requests are "ready" to be processed from the things that might dictate that readiness. Just from looking at the code it looks like we are trying to avoid making any request before Onyx is initialized. But we are using the I would much rather do something like this... Onyx.connect({
key: 'session',
callback: (val) => {
authToken = val ? val.authToken : null,
Network.setIsReady(true);
},
});However, this kind of begs the question of whether it matters whether we are "ready" or not? Can you maybe walk us through step by step how the unexpected log out happens? And why your PR will fix it? |
@marcaaron Oh good call,
Sure, I reproduced the issue forcing the // in API.js:
let authToken;
Onyx.connect({
key: ONYXKEYS.SESSION,
callback: val => setTimeout(() => authToken = val ? val.authToken : null, 100),
});(the video attached to this comment shows how the log out happens; first I'm already logged in, then I add the So I think this scenario could happen on a handset, where the Also, I'm not sure if this is a definitive fix because we still don't know 100% the root of the issue, but what I'm proposing is to have a sanity check of the |
Ok, I think it makes sense now. The way we are testing this might be better if we delayed everything and not just the Looking at the code and video I can see how we are getting logged out via the This is a pretty deep topic, but I have a feeling the right thing to do is:
It would be good to do a historical review of why we started logging out in the |
I agree with this solution. By the time a request fails due to an auth token, we might already have read everything needed from storage, to attempt to reauthenticate This seems to be the easiest thing that can be applied to the network queue to fix the problem What I don't like about it though
I'm going to propose and idea in my next comment, we can discuss it further if it's sound to you |
|
One of the widely accepted patterns for chain based handling is piping e.g. the middleware approach where you split the logic in stages and only go to the next stage when you call A pattern like that allows us to compose stages and make them run as desired - sync or async - the next stage logic would only run when the previous stages has called I think we can make our network queue a lot more predictable and easier to understand if we structure it in a pipe fashion. This would allow for better separation of concern, and would allow for an easier decoupling mechanism - instead of registering handlers that are put on a specific place of the existing control flow, we'll be appending or prepending stages to our pipeline Here's how it would look like to solve this particular issue with the middleware approach // Sample stages based on the current queue
const sateges = [
// Connectivity check -> next()
// Persist retriable request -> next()
// Wait Storage to be ready -> next()
// Enachance Params -> next()
// Invoke Request -> next()
// Happy Handler -> finish()
// Error Handler -> decide whether we should re-run stages or finish()
];
// Promise based solution
function waitStorage(req, next) {
return Onyx.get('session')
.then(() => next(req))
.then(() => stages.remove(waitStorage)) // we no longer need this stage for future pipeline runs
}
// Non promise based solution
function waitStorage(req, res) {
return next(
new Error('Storage parameters are not yet initialized')
)
}
Onyx.connect({
key: 'session',
callback: () => stages.remove(waitStorage);
})
}
// Solution more similar to our typical handling:
function waitStorage(req, res) {
const isNetworkReady = authToken != undefined && credentials != undefined;
if (isNetworkReady) {
return next(req);
}
return next(
new Error('Storage parameters are not yet initialized')
)
}If a stage should abort pipeline execution it can return a rejected promise (throw) or call next with an error Compare our existing queue with the above suggestion we can see some of our weaknesses
Whenever we need to deal with a problem we introduce a direct change like adding an |
|
And we won't even need a function enchanceParameters(req, next) {
return Onyx.get('session')
.then((session = {})=> next({
...req,
params: {
...req.params,
token: session.token,
credentials: session.credentials,
}
}))
} |
|
That looks pretty neat @kidroca. I agree that the interval style of network queue could be improved or replaced with something better. It feels the relevant suggestion we could apply to this issue is "use I love the idea to get rid of the
I do think we are close to some improvements to the various flags here if we:
In both "ready" and "paused" case we should re-queue the requests. So we can just add the check here I think Lines 112 to 113 in 66dc079 What if we:
Thoughts? |
|
@kidroca I like that proposal when we're at the point of refactoring the entire network lib! That could be really intriguing. I think we might be there sooner rather than later :D but not sure we are there yet. |
|
I already have a 1:1 with Marc, I'm going to add a unit test to simulate the issue first and then add the isReady flag on top of that.
I agree with these points ^^ and I will check that also too. Probably the log out in the enhanceParameters was introduced to fix a bug in the past but it could also be the culprit of the random logout. |
|
One other thing that might be playing a role in this issue/bug is when the app is launched from a background process Lines 8 to 22 in f97aebc Could there be something that is triggering a request when the app is launched "behind the curtains" and it failing for some reason and logging out the user? |
@roryabraham might know the answer to that or have ideas on if it's something we can debug |
It's possible. AFAICT the only API requests that can happen in this headless context are:
|
|
One suggestion I don't think I've seen discussed in this issue... In many locations in the app, we call
This solution comes with a downside that it would probably slow our startup, but will ensure that everything we need from Onyx is loaded when we go to use it. And we're only waiting for Onyx to load once, in one location, rather than peppering our codebase with |
|
That sounds similar to the
I think it might actually speed it up
Regarding race conditions this puts us at the same spot - we ask a function whether we're done loading - if we're done do one thing if we're not do something else and come again here after 1 sec. |
|
If we do something like preload everything on App launch, we should start the Network queue (and other side effect producing code) after Right now the Network queue starts immediately Lines 232 to 233 in f97aebc If for some reason we have read persisted request but not yet the token we'll make a request without a token Lines 83 to 89 in f97aebc
But otherwise the App or the part that is making requests About side effects in scripts We might think about identifying such items and only run them on command. As a web developer I would usually be schedule such scripts to execute from
|
|
Another side effect that is set up as soon as the script is parsed App/src/libs/Navigation/AppNavigator/AuthScreens.js Lines 67 to 84 in d7a7775 I think code like this is probably running in the "behind the curtains" launch (it's probably not supposed to) |
|
@marcochavezf Whoops! This issue is 2 days overdue. Let's get this updated quick! |
|
This conversation (while interesting) feels a bit tangential and not quite in context for "Android - User is logged out randomly". Does anyone mind if we maybe funnel some of this stuff into another thread and focus on solving the current problem here without triggering a big design change for now? If I could take a stab at the problem statement I'd say something like:
|
Sounds good to me, today and tomorrow I will continue with what we agreed to tackle the logout issue: unit tests, isReady flag, and stop logging out in For the isReady flag, I like the idea that @roryabraham proposed in point 2 (create a store for auth and credentials) because 1. separates concerns, 2. the store can be used as a single source of truth in both Also, I think we could use that flag to re-queue the requests if the store (auth + credentials) is not ready, that will solve the issue that @kidroca commented where we have read persisted requests but the auth token is not ready. |
Just going to quickly restate something I said earlier:
I'm curious how the store solution addresses this? Will we be calling I am admittedly getting distracted by the suggestion to create such a store for all the various Onyx keys and then export getters. I'd prefer to discuss the problem here a bit more before establishing this pattern. |
Ah, I was thinking to use instead the if (!isStoreReady()) {
requestsToProcessOnNextRun.push(queuedRequest);
return;
}Where the requests are retried (even persisted requests) if both auth token and credentials are not ready.
Yeah, I will stick to the |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
User should not be logged out randomly.
Actual Result:
User is being logged out randomly.
Workaround:
Unknown
Platform:
Where is this issue occurring?
Version Number: 1.1.2-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
We haven't found reproducible steps yet. Seem like a random scenario. @quinthar has been able to reproduce twice.
Expensify/Expensify Issue URL:
Issue reported by: @quinthar
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1632379894048100
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: