Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Data missing from storage after app restart #195

Closed
andrei-tofan opened this issue Dec 21, 2021 · 131 comments · Fixed by #231
Closed

[Bug] Data missing from storage after app restart #195

andrei-tofan opened this issue Dec 21, 2021 · 131 comments · Fixed by #231

Comments

@andrei-tofan
Copy link

andrei-tofan commented Dec 21, 2021

Describe the bug
We received in the past few months many complaints about users being logged out from the app. After adding logging and investigating the incidents we found out that at random the getStringAsync returns undefined after the app is restarted.
If we restart the app again the getStringAsync returns the expected value.
By restart I mean the app is closed by use with swipe up or is closed by to os and the user starts it again at a later time.

To Reproduce

Steps to reproduce the behavior:

  1. Initialize the storage with the following
this.storage = new MMKVStorage.Loader()
  .withServiceName('com.test.app1.TestV1')
  .withInstanceID('TestV1')
  .withEncryption()
  .initialize();
  1. Call this.storage.setStringAsync('token', tokenValue); on login to store the secret token
  2. Call this.storage.getStringAsync('token'); on each app start, if the call returns a value it means the user is logged in
  3. On some restarts at random calling this.storage.getStringAsync('token'); returns undefined after the value is set.

Expected behavior
After calling setStringAsync, all the calls to getStringAsync should return the value set.

Platform Information:

  • OS: iOS
  • React Native Version: 0.65.1
  • Library Version v0.6.6

Additional context
We are also curious if other people experience the same issue?

@ammarahm-ed
Copy link
Owner

ammarahm-ed commented Dec 29, 2021

Are you calling setStringAsync immediately after getStringAsync?

  1. You need to add await for async functions.
  2. If setStringAsync works, it should return true, otherwise it should returned undefined.
  3. Check if mmkv is initialized properly by calling console.log(global.setStringMMKV). It should return [Function ...] if everything is working as expected.

Can you post here the exact stub of the function that you are using to call setStringAsync and getStringAsync?

@haikov
Copy link

haikov commented Jan 6, 2022

We are also curious if other people experience the same issue?

We're also experiencing something similar, in very rare cases, but so far haven't been able to reproduce it ourselves, only got it as a feedback.

We're using this library for persisting redux data, and every time affected users open the app, the storage seems to be clear and not persisted, which results in users logged out.

What we also noticed, reinstalling the app helps and so far is the only way for the user to recover. We started looking into it, but didn't find anything yet.

@andrei-tofan
Copy link
Author

andrei-tofan commented Jan 6, 2022

Hi @ammarahm-ed,

Thanks for the response.

Are you calling setStringAsync immediately after getStringAsync?

No, we setStringAsync on login and getStringAsync when the app is started

  1. You need to add await for async functions.

We do that

  1. If setStringAsync works, it should return true, otherwise it should returned undefined.
  2. Check if mmkv is initialized properly by calling console.log(global.setStringMMKV). It should return [Function ...] if everything is working as expected.

Can you post here the exact stub of the function that you are using to call setStringAsync and getStringAsync?

Will create a demo app.

@ammarahm-ed
Copy link
Owner

We can fix this if we know what is causing the problem. Either storage is not initialized or it is not decrypted properly. Are you using an encrypted storage instance?

@andrei-tofan
Copy link
Author

Many thanks!

Are you using an encrypted storage instance?

Yes

@haikov
Copy link

haikov commented Jan 10, 2022

Are you using an encrypted storage instance?

Same for us. We're using it as a custom storage for redux persist as new MMKVStorage.Loader().withEncryption().initialize()

@ammarahm-ed
Copy link
Owner

@andrei-tofan @haikov It happens only to iOS users? How many users have been affected until now?

@haikov
Copy link

haikov commented Jan 11, 2022

It happens only to iOS users?

Yes, only iOS.

How many users have been affected until now?

Hard to tell precisely, as we don't get any crash reports for it, but it's either coming from customer support, or from app reviews. So far, we have seen a few. And also once experienced ourselves, but didn't manage to debug anything because re-installing the app fixed the problem.

@andrei-tofan
Copy link
Author

andrei-tofan commented Jan 11, 2022

It happens only to iOS users?

Yes, only iOS.

How many users have been affected until now?
Based on customer support I will estimate something around 5-10% / month, but I think some customers simply don't report the issue.

I've created a sample project with how we use react-native-mmkv-storage.
Let me know if we are doing something wrong
https://github.com/lineinspector/react-native-mmkv-storage-demo

Note that the issue can't be reproduced consistently.

@ammarahm-ed
Copy link
Owner

ammarahm-ed commented Jan 11, 2022

@andrei-tofan Have you been able to reproduce the issue at least once? Your implementation looks okay. No issue there.

I have an idea of what might be causing this. Will investigate.

@andrei-tofan
Copy link
Author

Thx! @ammarahm-ed

With the demo app not yet, but I'm running it on my phone and will update you once it happens.

@JoniVR
Copy link
Contributor

JoniVR commented Jan 22, 2022

I can confirm that we have faced this issue too. Over the past few months, I've had several users report "getting logged out randomly when launching the app". Also, one particular user said that when they restarted the app without logging in manually again, all was fine.

I've added more logging and from my testing, it does look like it's an issue related to reading persisted state. Couldn't figure out the reason at first either. At first I thought it was perhaps a redux-persist bug or refresh token failure bug, but after extensive logging that doesn't seem to be the issue.

I also can't reproduce it myself, but I've had about 8 reports on it out of about 1K users.
Most of the reports are using the iPhone SE (think 2nd gen) but that could just be a coincidence.
Also seems to be iOS only. We're using the encrypted storage with redux-persist. Hope this helps at least a bit.

@ammarahm-ed
Copy link
Owner

Hey @JoniVR thanks for adding this valuable information. It seems that sometimes mmkv is not registered. I will work a way to reproduce this then work on a solution. 8 times in 100 means if app is restarted about 100 times, maybe we can get 1 problematic launch.

@JoniVR
Copy link
Contributor

JoniVR commented Jan 22, 2022

Hi @ammarahm-ed!

Thanks for the quick reply. I'd also like to add to that that these specific users seem to be getting the issue more frequently than 8/100, perhaps even 2/10 times. Could also be more widespread but only these users reporting it.

Earlier reports were also saying it happend around when they received a push notification (though it also happens without one and even when you receive a push notification and just open the app regularly).
Perhaps it's a configuration specific thing? (thinking low memory, low storage,...)

My guess was also around registration failing or even a timing issue where the storage isn't created in time for redux-persist (though not sure how plausible that is).

If there's anything I can do to help, let me know. We've set up a specific group of Testflight testers who are facing this issue and have asked them to report the issue each time it occurs (reporting in this case means sending logs). I'd be happy to test possible patches or add some logging if we need it.

@ammarahm-ed
Copy link
Owner

@JoniVR I will post here a list of things to Log. That will help in debugging.

@ammarahm-ed
Copy link
Owner

@JoniVR The very basic thing you should log:

console.log("storage initialized: ", global.getStringMMKV);

This should return [Function ...] always. The most probable reason for this being undefined could be MMKV not registering at runtime. Add this and next time a user reports. Check to logs to see if storage was initialized or not.

@JoniVR
Copy link
Contributor

JoniVR commented Jan 24, 2022

I've currently logged it like this in the latest test build (inside index.js):

if (typeof global.getStringMMKV !== 'function') {
  crashlytics().log(
    `global.getStringMMKV value: ${global.getStringMMKV}, see: https://github.com/ammarahm-ed/react-native-mmkv-storage/issues/195`,
  );
  crashlytics().recordError(
    new Error('typeof global.getStringMMKV is not a function'),
  );
} else {
  crashlytics().log(`MMKV storage initialized: ${global.getStringMMKV}`);
}

In case anyone else wants to use it.

Now, let's hope a user runs into it again, should generate an automatic crashlytics report. If not, a manual report will confirm through logging that the function works as intended but the issue might be elsewhere.

@HT7-develop
Copy link

Currently a project i'm working on has been having this issue also ( but for the android version only, ios is working fine ).
As mentioned above by @ammarahm-ed i tried logging :
Which returns undefined (both have been used in App.js)
console.log("storage initialized: ", global.getStringMMKV);
and also returns undefined
await MMKV.setStringAsync("test", "some txt"); , const test = await MMKV.getStringAsync("isLoggedIn"); console.log('signin.js test: ', test);

Any additional info needed?

Platform Information:
OS: Android
react-native: 0.63.4
react-native-mmkv-storage: ^0.5.8,

@ammarahm-ed
Copy link
Owner

@HT7-develop You are getting this in the release build or while debugging?

@HT7-develop
Copy link

@ammarahm-ed thanks for the quick reply, I get it in the release build and also when debugging.
(ios seems fine for both)

@ammarahm-ed
Copy link
Owner

@HT7-develop Does it happen always for you in debug? Is it always undefined?

@HT7-develop
Copy link

Storage has been working for the project for months without a problem, so both debug and release were working without problems.
About 2 months ago we noticed some issue with getting the user credentials, and when using the console log you provided (and any other we try to retrieve) is indeed always undefined.

@ammarahm-ed
Copy link
Owner

@HT7-develop How often does it happen while debugging and in release app? How many occurrences?

@HT7-develop
Copy link

It happens 100% of the time when logging in to the app for Android and the same for all the times I myself tried to retrieve something from the storage.

@ammarahm-ed
Copy link
Owner

@HT7-develop Are you sure you have configured everything correctly? You might want to recheck library configuration. You can also try to run the example app in the project which works normally. Also update the library to 0.6.10 and see if that helps?

@HT7-develop
Copy link

@ammarahm-ed , yep i'll try the example project and compare both projects for differences. and post an update here.

@ammarahm-ed
Copy link
Owner

We might have a potential fix to this issue. I will work on it asap.

@ammarahm-ed
Copy link
Owner

ammarahm-ed commented Jan 26, 2022

Hello, I have worked on a fix after the issue #207 was opened which brought me to a new and more predictable way to install JSI bindings. Basically the potential fix is taken from https://github.com/mrousavy/react-native-mmkv.

It's not released yet but you should test this with the master branch:

yarn add https://github.com/ammarahm-ed/react-native-mmkv-storage.git

After installation you will need to remove previous linking on Android.

MainApplication.java:
Remove the commented out lines as below.

package com.example;

import android.app.Application;
import android.content.Context;
import com.facebook.react.PackageList;
import com.facebook.react.ReactApplication;
import com.facebook.react.ReactInstanceManager;
import com.facebook.react.ReactNativeHost;
import com.facebook.react.ReactPackage;
//import com.facebook.react.bridge.JSIModulePackage; <---------------
import com.facebook.soloader.SoLoader;
import java.lang.reflect.InvocationTargetException;
import java.util.List;
//import com.ammarahmed.mmkv.RNMMKVJSIModulePackage; <---------------
public class MainApplication extends Application implements ReactApplication {

  private final ReactNativeHost mReactNativeHost =
      new ReactNativeHost(this) {
        @Override
        public boolean getUseDeveloperSupport() {
          return BuildConfig.DEBUG;
        }

        @Override
        protected List<ReactPackage> getPackages() {
          @SuppressWarnings("UnnecessaryLocalVariable")
          List<ReactPackage> packages = new PackageList(this).getPackages();
          // Packages that cannot be autolinked yet can be added manually here, for example:
          // packages.add(new MyReactNativePackage());
          return packages;
        }

        @Override
        protected String getJSMainModuleName() {
          return "index";
        }

      //    @Override <---------------
      //    protected JSIModulePackage getJSIModulePackage() { <---------------
      //        return new RNMMKVJSIModulePackage(); <---------------
       //   } <---------------
      };

  @Override
  public ReactNativeHost getReactNativeHost() {
    return mReactNativeHost;
  }

  ...
}

If you are using reanimated@v2 then you should remove the modified CustomRNMMKVJSIModulePackage in android/app/main/java/com/yourappid/ and remove any references to it in MainApplication.java.


**For iOS just run**:
```pod install

Important Note: Once you get the iOS build running. Drop a comment here and let me know that storage is working in debug and you are able to read/write data. I don't have a Mac with me at the moment so need a little help here.

@ammarahm-ed
Copy link
Owner

@JoniVR The delay can be optimized with more iterations of smaller interval, 10-15ms. Maybe you can delay the interval and test for the next few days.

@JoniVR
Copy link
Contributor

JoniVR commented Apr 5, 2022

@ammarahm-ed One more question, why do you think we'd need to also apply the fix to get/set/remove methods then? To me it seems like they won't need the check in that case since the data would already be available (because storage can only be initialized once it is safe).

I'll lower the interval to 25ms on my end and increase the amount of tries, don't want to poll too aggressively either.

Current setup I'll be testing:

diff --git a/node_modules/react-native-mmkv-storage/ios/SecureStorage.m b/node_modules/react-native-mmkv-storage/ios/SecureStorage.m
index eacd030..99f0859 100644
--- a/node_modules/react-native-mmkv-storage/ios/SecureStorage.m
+++ b/node_modules/react-native-mmkv-storage/ios/SecureStorage.m
@@ -5,6 +5,7 @@
 #endif
 
 #import "SecureStorage.h"
+#import <UIKit/UIKit.h>
 
 
 @implementation SecureStorage : NSObject
@@ -43,6 +44,14 @@ - (NSString *) getSecureKey:(NSString *)key
     
     @try {
         [self handleAppUninstallation];
+        dispatch_sync(dispatch_get_main_queue(), ^{
+            int readAttempts = 0;
+            // See: https://github.com/ammarahm-ed/react-native-mmkv-storage/issues/195
+            while (![[UIApplication sharedApplication] isProtectedDataAvailable] && readAttempts++ < 100) {
+                // sleep 25ms before another attempt
+                usleep(25000);
+            }
+        });
         NSString *value = [self searchKeychainCopyMatching:key];
         if (value == nil) {
             NSString* errorMessage = @"key does not present";

@ammarahm-ed
Copy link
Owner

@JoniVR I think you are right. Methods other than get will be called later in app lifecycle hence they don't need the above work around.

@JoniVR
Copy link
Contributor

JoniVR commented Apr 12, 2022

Update 1 week later: I got one new report, but that one was from a user that didn't update to my initial patched build yet. So no new reports from any of the 2 patched builds. To me this seems like a pretty clear indication the fix is working as expected and probably ready for release?

@jaltin
Copy link

jaltin commented Apr 12, 2022

@JoniVR is there a PR for this yet? Would be curious to test it too if it is available as a PR.

@JoniVR
Copy link
Contributor

JoniVR commented Apr 12, 2022

@jaltin No PR yet as I wasn't sure this was the solution @ammarahm-ed wanted to use.

If you are eager to test yourself, I'd recommend looking into how to use patch files using patch-package (pretty easy to do)

You might even be able to apply my above patch directly to the bugfix branch.

@jaltin
Copy link

jaltin commented Apr 12, 2022

Thanks for quick follow-up @JoniVR.

Interesting how patch-package works, never seen that before. I prefer to wait for a PR, but thanks for making me aware of that package.

We have been badly affected by this bug (@andrei-tofan that initially reported this issue works with me on the same product). Yesterday we did a testflight release where we replaced the secure storage with a plain storage to see if we could avoid the bug. Today we again were logged out on on of our devices where we were testing.

Is it possible that this bug is also applicable to the plain storage, not just the secure storage?

@JoniVR
Copy link
Contributor

JoniVR commented Apr 12, 2022

@jaltin That would depend on how the plain storage works, if it's using Keychain as well, it might be affected by the same bug.

I have pretty much confirmed with certainty that this issue is related to the protected data being the culprit for our case though, so perhaps there's another issue related to it, not sure.

@ammarahm-ed
Copy link
Owner

ammarahm-ed commented Apr 12, 2022

Thanks for quick follow-up @JoniVR.

Interesting how patch-package works, never seen that before. I prefer to wait for a PR, but thanks for making me aware of that package.

We have been badly affected by this bug (@andrei-tofan that initially reported this issue works with me on the same product). Yesterday we did a testflight release where we replaced the secure storage with a plain storage to see if we could avoid the bug. Today we again were logged out on on of our devices where we were testing.

Is it possible that this bug is also applicable to the plain storage, not just the secure storage?

No it's not possible. maybe your bug report is not accurate and coming from an older version of the app with secure storage.

@jaltin
Copy link

jaltin commented Apr 12, 2022

No it's not possible. maybe your bug report is not accurate and coming from an older version of the app with secure storage.

Thanks for verifying that @ammarahm-ed. I will dig further to see if we can find another cause for that then.

@ammarahm-ed
Copy link
Owner

ammarahm-ed commented Apr 12, 2022

Thanks for quick follow-up @JoniVR.

Interesting how patch-package works, never seen that before. I prefer to wait for a PR, but thanks for making me aware of that package.

We have been badly affected by this bug (@andrei-tofan that initially reported this issue works with me on the same product). Yesterday we did a testflight release where we replaced the secure storage with a plain storage to see if we could avoid the bug. Today we again were logged out on on of our devices where we were testing.

Is it possible that this bug is also applicable to the plain storage, not just the secure storage?

In your case, it might be possible but it is due to the same bug as above. That's because if you change the storage type in code but on user's device at runtime, the storage instance is encrypted, then the encrypted instance is used. Not the plain storage. Maybe that's why you are still being logged out.

@ammarahm-ed
Copy link
Owner

No it's not possible. maybe your bug report is not accurate and coming from an older version of the app with secure storage.

Thanks for verifying that @ammarahm-ed. I will dig further to see if we can find another cause for that then.

See my last comment.

@jaltin
Copy link

jaltin commented Apr 12, 2022

Thanks for verifying that @ammarahm-ed. I will dig further to see if we can find another cause for that then.

An update on this. The error we saw was actually due to a coding error, so there is most likely not an issue in the non-encrypted storage.

@ammarahm-ed
Copy link
Owner

Thanks for verifying that @ammarahm-ed. I will dig further to see if we can find another cause for that then.

An update on this. The error we saw was actually due to a coding error, so there is most likely not an issue in the non-encrypted storage.

The above case i mentioned will still be true for users who update the app. Hence the patch-fix is the only way to properly fix this bug.

@JoniVR
Copy link
Contributor

JoniVR commented Apr 12, 2022

@ammarahm-ed Any idea on when you can release a version with a fix?

You want me to submit the above patch as PR?

@ammarahm-ed
Copy link
Owner

@ammarahm-ed Any idea on when you can release a version with a fix?

You want me to submit the above patch as PR?

If you can submit a PR that would be great

@JoniVR
Copy link
Contributor

JoniVR commented Apr 12, 2022

@ammarahm-ed Done, let me know if anything else needs to be done :)

@ammarahm-ed
Copy link
Owner

Fix shipped in v0.7.0 Thanks @JoniVR for the insane debugging and finally fixing this too.

@JoniVR
Copy link
Contributor

JoniVR commented Apr 21, 2022

@ammarahm-ed

Bad news, I would reopen this issue. I left in the automatic detection of this issue and I'm still getting this sometimes in production. This automatic detection in production also gives a better sense of the amount of times the issue was actually occurring.

I've received 18 reports from 11 different users (with around 1400 iOS users in total) just today and yesterday combined.

Some of these reports clearly showcase the workaround working, for example:

0 | Thu Apr 21 2022 00:58:52 | DEBUG | ios | MMKV:encryptedStorage.getString | undefined
1 | Thu Apr 21 2022 00:58:52 | ERROR | ios | MMKV:encryptedStorage.getString | Expected null is not null
2 | Thu Apr 21 2022 00:58:58 | DEBUG | ios | persistErrorHandlerMiddleware | Rehydrating auth, token: eyJhbG...byZ3LY
3 | Thu Apr 21 2022 00:58:58 | DEBUG | ios | AuthService:refreshAuthToken | Attempting token refresh with: PQTUZ1...KdN9o=

Notice how there's a 6s delay between line 1 -> 2 => indicates the delay worked

Other reports don't show a delay and also seem to still log the user out:

0 | Wed Apr 20 2022 21:13:31 | DEBUG | ios | MMKV:encryptedStorage.getString | undefined
1 | Wed Apr 20 2022 21:13:31 | ERROR | ios | MMKV:encryptedStorage.getString | Expected null is not null
2 | Wed Apr 20 2022 21:13:31 | DEBUG | ios | persistErrorHandlerMiddleware | Rehydrating auth, token: null
3 | Wed Apr 20 2022 21:13:31 | DEBUG | ios | App | Notification permission status: 1
4 | Wed Apr 20 2022 21:13:31 | DEBUG | ios | setMessagingToken | OS: ios, Token: cJyUje...7-QCo4
5 | Wed Apr 20 2022 21:13:31 | DEBUG | ios | httpInterceptor:responseHandler | {
"requestUrl": "/endpoints.emergency.json",
"responseCode": 200,
"bearer": null
}

So, for some reason the delay isn't always happening.. any ideas as to why?

edit: a third kind of log:

0 | Thu Apr 21 2022 14:28:47 | DEBUG | ios | MMKV:encryptedStorage.getString | undefined
1 | Thu Apr 21 2022 14:28:47 | ERROR | ios | MMKV:encryptedStorage.getString | Expected null is not null
2 | Thu Apr 21 2022 16:45:38 | DEBUG | ios | persistErrorHandlerMiddleware | Rehydrating auth, token: null

Here the timing between the "read" and actual re-hydration seems to be +2hrs, which I think indicates that the app launched in the background?

@zhaiyjgithub
Copy link

Still on v0.7.5.

@JoniVR
Copy link
Contributor

JoniVR commented Jun 30, 2022

I've created a follow-up PR, see: #263, which is related to the discussion in #246 and specifically #246 (comment)

@mkhotib20
Copy link

still on 0.9.0

@scblason
Copy link

I'm on 0.8.0 (because of < 0.71 project) and also having this issue when testing on iOS simulator. Initialized shows as false when checking MMKV's options object.

@rlueder
Copy link

rlueder commented May 30, 2024

This is still an issue with 0.9.1, easy to reproduce by using setString and getString, react-native-mmkv-storage often returns null after the value has supposedly been set, sometimes it'll return the value as expected, but not possible to determine what value it'll return.

After some more testing it does seem that app installs has an effect on it, if an app install is a "good" one you'll consistently get the values back as expected, if it's a "bad install" it'll return null every single time, restarting the app doesn't fix it.

I also tested side-by-side with react-native-mmkv and this other library consistently returns the string value as expected even after reinstalls so we're switching over to it: https://github.com/mrousavy/react-native-mmkv/

@muhammadarsal
Copy link

Hey @rlueder , we are also facing the same issue with v0.9.1 and now migrating to react-native-mmkv. Just wanted to know if you guys were able to get rid of this issue after switching. Any heads-up would be greatly appreciated. Thanks!

@rlueder
Copy link

rlueder commented Jul 22, 2024

@muhammadarsal yes, we finished the migration a few weeks ago and the issue was fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.