-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Comments
Are you calling
Can you post here the exact stub of the function that you are using to call setStringAsync and getStringAsync? |
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. |
Hi @ammarahm-ed, Thanks for the response.
No, we setStringAsync on login and getStringAsync when the app is started
We do that
Will create a demo app. |
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? |
Many thanks!
Yes |
Same for us. We're using it as a custom storage for redux persist as |
@andrei-tofan @haikov It happens only to iOS users? How many users have been affected until now? |
Yes, only iOS.
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. |
Yes, only iOS.
I've created a sample project with how we use react-native-mmkv-storage. Note that the issue can't be reproduced consistently. |
@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. |
Thx! @ammarahm-ed With the demo app not yet, but I'm running it on my phone and will update you once it happens. |
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. |
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. |
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). 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. |
@JoniVR I will post here a list of things to Log. That will help in debugging. |
@JoniVR The very basic thing you should log:
This should return |
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. |
Currently a project i'm working on has been having this issue also ( but for the android version only, ios is working fine ). Any additional info needed?
|
@HT7-develop You are getting this in the release build or while debugging? |
@ammarahm-ed thanks for the quick reply, I get it in the release build and also when debugging. |
@HT7-develop Does it happen always for you in debug? Is it always undefined? |
Storage has been working for the project for months without a problem, so both debug and release were working without problems. |
@HT7-develop How often does it happen while debugging and in release app? How many occurrences? |
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. |
@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? |
@ammarahm-ed , yep i'll try the example project and compare both projects for differences. and post an update here. |
We might have a potential fix to this issue. I will work on it asap. |
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:
After installation you will need to remove previous linking on Android.
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
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 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. |
@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"; |
@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. |
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? |
@JoniVR is there a PR for this yet? Would be curious to test it too if it is available as a PR. |
@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. |
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? |
@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. |
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. |
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. |
See my last comment. |
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. |
@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 |
@ammarahm-ed Done, let me know if anything else needs to be done :) |
Fix shipped in v0.7.0 Thanks @JoniVR for the insane debugging and finally fixing this too. |
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:
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:
So, for some reason the delay isn't always happening.. any ideas as to why? edit: a third kind of log:
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? |
Still on v0.7.5. |
I've created a follow-up PR, see: #263, which is related to the discussion in #246 and specifically #246 (comment) |
still on 0.9.0 |
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. |
This is still an issue with 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 |
Hey @rlueder , we are also facing the same issue with |
@muhammadarsal yes, we finished the migration a few weeks ago and the issue was fixed. |
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
returnsundefined
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:
this.storage.setStringAsync('token', tokenValue);
on login to store the secret tokenthis.storage.getStringAsync('token');
on each app start, if the call returns a value it means the user is logged inthis.storage.getStringAsync('token');
returns undefined after the value is set.Expected behavior
After calling
setStringAsync
, all the calls togetStringAsync
should return the value set.Platform Information:
Additional context
We are also curious if other people experience the same issue?
The text was updated successfully, but these errors were encountered: