ammarahm-ed / react-native-mmkv-storage

An ultra fast (0.0002s read/write), small & encrypted mobile key-value storage framework for React Native written in C++ using JSI
https://rnmmkv.now.sh
MIT License
1.58k stars 110 forks source link

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

Closed andrei-tofan closed 2 years ago

andrei-tofan commented 2 years ago

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();
  2. Call this.storage.setStringAsync('token', tokenValue); on login to store the secret token
  3. Call this.storage.getStringAsync('token'); on each app start, if the call returns a value it means the user is logged in
  4. 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:

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

ammarahm-ed commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Many thanks!

Are you using an encrypted storage instance?

Yes

haikov commented 2 years ago

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 commented 2 years ago

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

haikov commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ammarahm-ed commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

HT7-develop commented 2 years ago

@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 commented 2 years ago

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

HT7-develop commented 2 years ago

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 commented 2 years ago

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

HT7-develop commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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

ammarahm-ed commented 2 years ago

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

ammarahm-ed commented 2 years ago

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.

JoniVR commented 2 years ago

Hi @ammarahm-ed, just installed on the iOS simulator without issues here, podfile does say 0.6.10 though, not sure if that's correct?

ammarahm-ed commented 2 years ago

@JoniVR I haven't bumped the version yet but this should be okay. You can confirm you have the latest version by checking if you can import isLoaded function from the library.

import {isLoaded} from "react-native-mmkv-storage"

JoniVR commented 2 years ago

That seems to return true, so at least on debug and sim it seems to be working fine. Will push this change to beta testers. Anything specific you want me to log here? Last build didn't provide any results yet (only testing with 3 affected users).

ammarahm-ed commented 2 years ago

@JoniVR just log isLoaded function's result. It's basically doing the same thing you were doing above manually. true means that everything is working.

ammarahm-ed commented 2 years ago

Released v0.6.11

haikov commented 2 years ago

After upgrading to 0.6.11 we're able to build the app successfully and not able to reproduce the issue mentioned here (although we were unable to reproduce it previously either).

But, we started seeing some new runtime warning from inside MMKV: -[UIApplication applicationState] must be used from main thread only.

image

ammarahm-ed commented 2 years ago

@haikov I will look into the warning.

HT7-develop commented 2 years ago

@ammarahm-ed , I also tested it (for android, as that is where storage the issue was for my project) and the issue seems resolved as of the latest update the storage is working again and saving/retrieving my users. In the debug build and release build, both are working fine. Thanks for looking into it. really appreciate it.

OS: Android

bombillazo commented 2 years ago

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.

If we were using a previous version, do we need to follow these steps to upgrade to >= 6.11?

To confirm, we no longer need this file? image

ammarahm-ed commented 2 years ago

Yes you don't need this. However if you don't remove it, everything should still work without changing anything. @bombillazo

bombillazo commented 2 years ago

Ive updated my MainApplication.java file but this problem still persist for me, I see the store working while the app is running but every time I run my loading logic when the app opens, the stores seem o be empty or something.

EDIT

So printing isLoaded returns false. I installed it and ran npx mmkv-link and the build was successful. I checked the files manually as well. What could be causing isLoaded to return false? @ammarahm-ed

"react-native": "0.67.1",

ammarahm-ed commented 2 years ago

@bombillazo

run ./gradlew clean on /android folder. Delete node_modules. Delete app from your phone. Then run yarn install or npm install. Finally install the app again and everything should be good.

Also you don't need to run npx mmkv link on RN 0.67 and above.

Also make sure to run pod install on iOS.

Did this happen on iOS or Android?

bombillazo commented 2 years ago

Its on Android, I did not reinstall the app, let me try that, thanks.

bombillazo commented 2 years ago

Still set to false :/

On older versions, it did return something when I called { global.set...} when I had import com.ammarahmed.mmkv.RNMMKVJSIModulePackage; in my MainApplication.java file.

Is there any way to check the native files manually to verify if it installed correctly?

ammarahm-ed commented 2 years ago

@bombillazo Try to log NativeModules.MMKVNative.install:

import {NativeModules} from "react-native";
console.log(NativeModules.MMKVNative.install);
bombillazo commented 2 years ago

@bombillazo Try to log NativeModules.MMKVNative.install:

import {NativeModules} from "react-native";
console.log(NativeModules.MMKVNative.install);

I get this back

 function nonPromiseMethodWrapper() { [bytecode] }
ammarahm-ed commented 2 years ago

@bombillazo Seems like everything is updated correctly. Try this:

import {init,isLoaded} from "react-native-mmkv-storage";

console.log(init(),isLoaded(), global.setStringMMKV)
bombillazo commented 2 years ago

@bombillazo Seems like everything is updated correctly. Try this:

import {init,isLoaded} from "react-native-mmkv-storage";

console.log(init(),isLoaded(), global.setStringMMKV}

wow, ok after running it the first time, now it is returning true! Even when I close the app or clear the app data.

ammarahm-ed commented 2 years ago

@bombillazo Seems like your metro cache is old causing a problem when you upgrade loading old bundled js files. Restart metro bundler with cache reset:

yarn start --reset-cache

If you don't know how that works. Just remove node_modules folder and run npm/yarn install

And now everything should work as expected.

bombillazo commented 2 years ago

Its weird, I always run metro with --reset-cache for that same reason, I don't trust the caches. But glad it works now! Thank you for the assistance!

ammarahm-ed commented 2 years ago

Most welcome. Do let me know if you face any other issues

JoniVR commented 2 years ago

Hi @ammarahm-ed

I've just had 2 reports in the same day from one of our testers telling me that the issue still seems to exist. Not sure if it's related to my specific codebase though.

Specifics:

Below is one of the logfiles that demonstrate the scenario: be.frontforce.emergency.mobile_issue_f26bcd4da6f466433e7f065611c4e778_error_session_81fe6016199e44fc9abb239326ff06f8_DNE_0_v2.log

Could it perhaps still be the getStringAsync function? Should I log this? Any other values I can log to identify other potential issues?