iOS - App crashes after turning off the internet connection

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Expected Result:

User status is shown offline and can explore the app

Actual Result:

App crashes when turning off the internet connection

Action Performed:

1. Lunch the app and login
2. Disable internet connection on the device

Workaround:

Unknown

Platform:

Web
iOS ✔️
Android
Desktop App
Mobile Web

Version Number: 1.0.66-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Bug5105198_Crash.mp4

ios crash.txt

Expensify/Expensify Issue URL:

View all open jobs on Upwork

[MelvinBot]

Triggered auto assignment to @CortneyOfstad (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[OSBotify]

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

1. Identify the pull request that introduced this issue and revert it.
2. Find someone who can quickly fix the issue.
3. Fix the issue yourself.

[MelvinBot]

Triggered auto assignment to @TomatoToaster (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[TomatoToaster]

I've seen this and I'm trying to run the staging branch locally to fix this. Running into some issues with XCode and ECash so I'm resolving that right now. Haven't been able to recreate locally yet.

[trjExpensify]

👋 Any luck, @TomatoToaster?

[TomatoToaster]

😞 No not really. I'm still not able to recreate this locally, and that would tell me most easily why this is happening. I'm trying to figure out from everything else I can use including the crash logs here.

In the meantime I had looked through the PRs we had merged for this and I can't find anything that would cause this issue we're seeing. I can confirm this should still be a blocker though because this definitely isn't happening on the latest version of the app or the previous staging version.

[TomatoToaster]

😭 I finally got it working locally. Ok I'm trying to figure out a fix for this.

[TomatoToaster]

Ok so now I'm fairly certain it's this PR that's created this new problem: #3423

More specifically it's the changes in Onyx PR here: https://github.com/Expensify/react-native-onyx/pull/81/files

Sorry I had a bit of mixup, that is the most recent PR but it's not exactly the one that caused this specific regression. It's a part of the larger diff here: https://github.com/Expensify/react-native-onyx/compare/d0a5d12c08f2f029e3038fb3516a93a9403e9746..52c254fbc4b96224d2ac087e57ef6efc4210ccc1

From the crash logs it seems like this is is being called in an endless loop:
https://github.com/Expensify/react-native-onyx/blob/52c254fbc4b96224d2ac087e57ef6efc4210ccc1/lib/Onyx.js#L520

So I don't think it makes sense to revert that PR, since that is actually fixing another breaking feature. I think what that PR fixes takes priority over not being able to use offline mode.

I'm going to see if I can figure out a fix that won't fail to clear both of these deployblockers.

[parasharrajat]

@TomatoToaster You should mark this as a regression on that PR as everyone is waiting for issues related to that PR.

[TomatoToaster]

So to clarify a bit more on what's happening, when the app goes offline we repeat these lines endlessly, causing stack to grow too huge too quickly and crash:

https://github.com/Expensify/Expensify.cash/blob/533a0230986b3724397e1152f7ce70a2edb2569a/src/libs/Network.js#L42-
https://github.com/Expensify/react-native-onyx/blob/52c254fbc4b96224d2ac087e57ef6efc4210ccc1/lib/Onyx.js#L520
https://github.com/Expensify/react-native-onyx/blob/52c254fbc4b96224d2ac087e57ef6efc4210ccc1/lib/Onyx.js#L290
https://github.com/Expensify/react-native-onyx/blob/52c254fbc4b96224d2ac087e57ef6efc4210ccc1/lib/Onyx.js#L293

I've tried changing various things to fix it but haven't gotten a good fix yet.

[parasharrajat]

Thanks for pointing it out @TomatoToaster. Matter of fact, I am working on a PR #2703 that changes this logic. You can pull in that PR to confirm if that fixes this problem. I will take a look at this issue and try to add a fix there on the PR.

[TomatoToaster]

Your PR does manage to resolve this problem (I think). However, I wasn't able to verify it actually lets us recover from offline mode and send the queued actions when wifi is reactivated. I also think there also might be a simpler solution that fixes this.

Either way, I think we don't need to keep this as a staging deployblocker. Since we're not going to revert all the Onyx changes, we can leave this as a daily regression and still keep working on a solution (or the PR @parasharrajat tagged above might get merged before then and this issue might dissapear). iOS offline mode will be broken for the meantime, but that's pretty workaroundable for most uses of the app.

[kidroca]

@TomatoToaster thanks for the links

Here's what I've found out:

https://github.com/Expensify/Expensify.cash/blob/533a0230986b3724397e1152f7ce70a2edb2569a/src/libs/Network.js#L32-L45

We have a listener subscribed to ONYXKEYS.NETWORK_REQUEST_QUEUE that also uses Onyx.set to update the same key, which in turn is triggering the listener again

The check here does not work well

https://github.com/Expensify/Expensify.cash/blob/533a0230986b3724397e1152f7ce70a2edb2569a/src/libs/Network.js#L35-L37

The code that follows updates Onyx and this triggers the callback again
https://github.com/Expensify/Expensify.cash/blob/533a0230986b3724397e1152f7ce70a2edb2569a/src/libs/Network.js#L42

The next value for peristedRequests is [] so the expression !persistedRequests does not skip, code continues and calls Onyx.set again and this triggers the callback again and again... forever

The check can be updated to

if (didLoadPersistedRequests || _.size(persistedRequests) === 0) {
  return;
}

_.size would return 0 for undefined - _.size(undefined) // 0

Alternatively you can call Onyx.remove(ONYXKEYS.NETWORK_REQUEST_QUEUE) as it seem that was an expected case e.g. not having !persistedRequests

---

I think this was an existing issue, it just became exposed as now Onyx.set works much faster because it optimistically calls subscribers with the new value

[kidroca]

Here's a debug video of the loop:

Expensify.cash.-.Google.Chrome.2021-06-10.15-23-56.mp4

[TomatoToaster]

I think this was an existing issue, it just became exposed as now Onyx.set works much faster because it optimistically calls subscribers with the new value

I think you're spot on about this, @kidroca. I came to a similar conclusion by following the loops and already have a PR for it: #3511

It's a testament to how much faster Onyx is that we're seeing regressions like this 😄 .

[kidroca]

Now that I look at it again, we might have a problem with the new Onyx.set - this was not an existing problem.
Before Onyx.set used to call keysChanged in the .then block of a Promise, even if the write happens instantly promises are resolved on the next tick and didLoadPersistedRequests would always be set by that time

Now Onyx.set is calling keysChnaged right away, which means that code "stops" at the line Onyx.set(...) and proceeds after all the sync code in Onyx.set is over, which means after keysChanged is called

This can be a problem because keysChanged might have to inform a lot of connections and the UI thread will be blocked during this time - this can be related to the short freezes we get when a chat is selected

Instead we should probably update Onyx.set to schedule the call to keysChanged for the next tick by doing something like

setImmediate(() => keysChanged(key, value)) or Promise.resolve().then(() => keysChanged(key, value)))
This would still feel instant to the user, but would not cause problems like the above

cc @marcaaron

[TomatoToaster]

I'm thinking it would actually be better to avoid situations like the above by explicitly avoiding extra loops. We have a weird scenario here (or is this actually common in Expesnsify.cash?) where we're calling the Onyx.set within the Onyx.connect's callback that's causing the infinite loop. Even if keysChanged happened in a promise, it might get called an extra time more than it's needed right?

I think the right solution here is to quit early for empty persistedRequests. Adding the promise there is equivalent to just swapping the order of didLoadPersistedRequests = true and Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, []). All of these will solve the problem here, but the latter two solutions runs the loop an extra time for no reason.

I think wrapping keysChanged in a promise in Onyx.set() is not a great idea because then we might introduce more race conditions in our code. Code where we're expecting it to go one extra tick feels like it would be harder to maintain.

[kidroca]

I think the right solution here is to quit early for empty persistedRequests

Yes, the fix is good it should stay whatever is decided

I think wrapping keysChanged in a promise in Onyx.set() is not a great idea because then we might introduce more race conditions in our code. Code where we're expecting it to go one extra tick feels like it would be harder to maintain.

So far everything worked without expectation of how long it would take for set to finally call keysChaged and I think it should stay that way - components don't subscribe to set or merge they subscribe to receive updates at some point

My concern is only that now set is blocking whatever happens on screen as it has a lot of synchronous code - I might be wrong, but we can investigate if this is the reason (well one of them) for the little hiccup when you select a chat

E.g. if you have a button component that calls Onyx.set in it's press handler it might block the UI for a bit if keysChanged have to iterate a lot of subscribers

[kidroca]

Another options would be to use the InteractionsManager and run the keysChanged after interactions are complete - this way hopefully are not blocking UI

[marcaaron]

Just catching up here, but nice detective work everyone! I hear where you are coming from @TomatoToaster, but I think in general we should first try to preserve the original behavior we had when it's something low level like this and could potentially affect many things.

Since the entire app has been built on the assumption that subscribers are notified async I think we should maybe continue this way for now and move the keysChanged() call to another tick like @kidroca is suggesting.

[marcaaron]

I think the right solution here is to quit early for empty persistedRequests. Adding the promise there is equivalent to just swapping the order of didLoadPersistedRequests = true and Onyx.set(ONYXKEYS.NETWORK_REQUEST_QUEUE, []). All of these will solve the problem here, but the latter two solutions runs the loop an extra time for no reason.

There's no real problem with the original code - if we can assume things are asynchronous. The order doesn't make any difference in that case. But maybe it's a little confusing if you're not familiar with how Onyx will work.

Also, separately, I think there is maybe a strong case to discuss a future where Onyx isn't an asynchronous system now that we have the local caching stuff. But probably requires a larger conversation.

[kidroca]

There's no real problem with the original code - if we can assume things are asynchronous

I think the code should still be updated, since persistedRequests is an array this check should account for it being empty

https://github.com/Expensify/Expensify.cash/blob/533a0230986b3724397e1152f7ce70a2edb2569a/src/libs/Network.js#L35-L37

Also why not disconnect from Onyx and release this connection - nothing would ever set didLoadPersistedRequests to false

[marcaaron]

sure, it could be improved (like many things) - but those issues are not causing an infinite loop 🙃