Skip to content

All calls to save() or fetch() on ParseUser and ParseInstallation will freeze if previous call to saveEventually() has failed #827

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

Closed
cjosepha opened this issue May 19, 2018 · 19 comments

Comments

@cjosepha
Copy link
Contributor

cjosepha commented May 19, 2018

After ParseUser.saveEventually() or ParseInstallation.saveEventually() has failed, the sdk seems to be in a corrupted state as all subsequent calls to save() or fetch() on ParseUser and ParseInstallation will freeze. Restarting the app or the device will not solve the issue and only a re-installation of the app allows to save/fetch ParseUser/ParseInstallation again.
This issue occurs with any version of parse-server and any version of Parse-SDK-Android.

Steps to reproduce the issue on ParseUser (same for ParseInstallation)

  1. Log in
  2. Delete user session on the server
  3. Put new data to user
  4. Call user.saveEventually() -> Returns invalid session token error (expected)
  5. Close app and relaunch it
  6. Put new data to user
  7. Call user.save() -> Freeze forever

Note that terminating the app then relaunching it will not change anything : any call to user.save() will freeze (as user.saveInBackground() never completes).

Parse initialization code used

Parse.setLogLevel(Parse.LOG_LEVEL_VERBOSE);
Parse.initialize(new Parse.Configuration.Builder(application)
.applicationId(BuildConfig.PARSE_APP_ID)
.clientKey(BuildConfig.PARSE_CLIENT_KEY)
.enableLocalDataStore()
.server(BuildConfig.SERVER_URL + ":" + BuildConfig.SERVER_PORT + "/parse/")
.build());

Standalone app showing the issue

https://github.com/DanGTZ/Parse-Android-SDK-Test

The tests testSaveEventuallyInstallation and testSaveEventuallyUser passe the first time they are launched, then subsequent launches will freeze on the first save()

https://github.com/DanGTZ/Parse-Android-SDK-Test/blob/master/app/src/androidTest/java/com/perfexpert/parsetest/ParseSaveEventuallyTest.java

Workaround

This issue occurs after loading invalid data from the cache at step 5. To prevent loading invalid data at startup, clear the ParseUser or ParseInstallation disk cache just after the saveEventually() has failed (step 4). To do so, you can use this gist : https://gist.github.com/DanGTZ/7c95d6fc0bb95d3d7433ab7dc309eeac

@cjosepha
Copy link
Contributor Author

Currently I have users of my app that can't use it because their installation is in this state : any save or fetch in background never returns.
I'm looking for a solution to solve this with an app update, without the need to reinstall the app, in order to prevent data loss.

@flovilmart
Copy link
Contributor

@dangtz this looks quite serious. Are you able to pinpoint the exact reason why this behavior is exhibited?

@cjosepha
Copy link
Contributor Author

cjosepha commented May 22, 2018

@flovilmart it seems that there is a deadlock in the code that manage operations queue for persisted singletons like ParseInstallation and ParseUser, because I'm not able to reproduce this issue with a basic ParseObject, even if I pin the object in LDS.
May be some operations queue resources are not released properly when the saveEventually() fails, but my poor knowledge of the sdk code doesn't allows me to tell you more.

@flovilmart
Copy link
Contributor

@dangtz because you're able ton consistently reproduce it, can you perhaps open a failing PR, that exhibit the deadlock? Perhaps plucking a debugger and checking the stack / threads once dead locked can help pinpoint the issue.

@cjosepha
Copy link
Contributor Author

@flovilmart by "failing PR" you mean creating new unit tests in the sdk?
Also to reproduce the issue, one step is to close the current session (call to a custom cloud function "closeAllSession" that I created for my test) : do I need to mock the server to do this in the sdk?

@flovilmart
Copy link
Contributor

by "failing PR" you mean creating new unit tests in the sdk?

Yep, I mean a new test that is currently failing without a fix.

do I need to mock the server to do this in the sdk?

Most probably, adding a mock would be the way to go as we never run edge to edge tests with a live server for this SDK IIUC.

cjosepha added a commit to cjosepha/Parse-SDK-Android that referenced this issue May 24, 2018
- The eventually queue should be cleared at each test execution but Parse.getEventuallyQueue().clear() crashes, that's why line 1585 is commented
cjosepha added a commit to cjosepha/Parse-SDK-Android that referenced this issue May 29, 2018
@cjosepha
Copy link
Contributor Author

@flovilmart I'm not able to reproduce this issue with unit tests #831
After more integration testing in my own app, it seems that the freeze occurs only after relaunching the app after the saveEventually() fails.

@flovilmart
Copy link
Contributor

Ok, so that means that the full app is in an invalid state which is quite bad as we have a persistent issue.

@cjosepha
Copy link
Contributor Author

Yes, and now I have to take my time to prepare a publication for my users to explain them the only solution to the problem is to reinstall the app and then lost all their local data (pinned in localdatastore) :-(

@flovilmart
Copy link
Contributor

Nah, we can do better I believe no? If we find the soUrce of the deadlock, we can ensure that at next restart everything is back on track

@cjosepha
Copy link
Contributor Author

That's what I was trying to do.

The exact async method that doesn't return is this one :
https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ParseObject.java#L1604

@cjosepha
Copy link
Contributor Author

cjosepha commented May 29, 2018

@flovilmart here is a standalone app showing the issue :

https://github.com/DanGTZ/Parse-Android-SDK-Test

The tests testSaveEventuallyInstallation and testSaveEventuallyUser passe the first time they are launched, then subsequent launches will freeze on the first save()

https://github.com/DanGTZ/Parse-Android-SDK-Test/blob/master/app/src/androidTest/java/com/perfexpert/parsetest/ParseSaveEventuallyTest.java

Expects a local parse-server with application id "parse_app_id" and client key "parse_client_key".

@flovilmart
Copy link
Contributor

@Jawnnypoo and @rogerhu , is that something you guys you could have a look at?

@cjosepha
Copy link
Contributor Author

cjosepha commented Jun 1, 2018

Here is the cloud code that is needed for this test :

Parse.Cloud.define("closeAllSessions", function(request, response) {
  if (!request.user) {
    response.error("A user must be logged in");
  } else {
    var sessionQuery = new Parse.Query(Session);
    sessionQuery.equalTo('user', request.user);
    sessionQuery.find({
      useMasterKey: true
    }).then(function(sessions) {
      console.log(sessions.length + " sessions will be closed");
      return Parse.Object.destroyAll(sessions, {
        useMasterKey: true
      });
    }).then(function() {
      response.success();
    }, function(error) {
      response.error(error.message);
    });
  }
});

@rogerhu
Copy link
Contributor

rogerhu commented Jun 3, 2018

Thanks for the repro -- I see the same thing. Note I had to change your code to be new Parse.Query(Parse.Session); to get things to work.

@cjosepha
Copy link
Contributor Author

cjosepha commented Jun 3, 2018

@rogerhu sorry forgot to add this line before the function : var Session = Parse.Object.extend('_Session');

@cjosepha
Copy link
Contributor Author

cjosepha commented Jun 5, 2018

More info, once the saveEventually has failed :

  • The problem occurs only after relaunching the app

  • The problem doesn't occur if there is a successful saving of the object (User or Installation) before relaunching the app.

  • The problem doesn't occur if LDS is not enabled

I guess something wrong is persisted in LDS when saveEventually fails. Then after relaunch, the wrong data is loaded from LDS and make any subsequent save deadlocking. If the app is not relaunched, user logs in and save the object, the wrong data is removed from LDS, due to successful saving, and the problem never occurs. I think this is the most likely hypothesis.

cjosepha added a commit to cjosepha/Parse-SDK-Android that referenced this issue Jun 10, 2018
@cjosepha
Copy link
Contributor Author

This issue occurs after loading invalid data from the cache at step 5. To prevent loading invalid data at startup, clear the ParseUser or ParseInstallation disk cache just after the saveEventually() has failed (step 4). To do so, you can use this gist : https://gist.github.com/DanGTZ/7c95d6fc0bb95d3d7433ab7dc309eeac

I've also updated my test app so you can see the impact of clearing cache : https://github.com/DanGTZ/Parse-Android-SDK-Test

cjosepha added a commit to cjosepha/Parse-SDK-Android that referenced this issue Jun 13, 2018
* Improved test for parse-community#827 :
- Generalize to any server error that is different to CONNECTION_FAILED (not only INVALID_SESSION_TOKEN)
- Added isDirty() checks
- fixed test tearDown
cjosepha added a commit to cjosepha/Parse-SDK-Android that referenced this issue Jun 13, 2018
Jawnnypoo pushed a commit that referenced this issue Jul 20, 2018
* - Added a failing test for #827
- The eventually queue should be cleared at each test execution but Parse.getEventuallyQueue().clear() crashes, that's why line 1585 is commented

* Removed unused imports

* Fixed ParseUserTest.testSaveEventuallyWhenSessionIsInvalid() so it reproduce exactly issue #827

* Fixed server response mocking in ParseUserTest.testSaveEventuallyWhenSessionIsInvalid()

* Failing test for #827 fails when the deadlock at ParseObject.java:L1604 occurs

* * Suggestion of fix for #827
* Improved test for #827 :
- Generalize to any server error that is different to CONNECTION_FAILED (not only INVALID_SESSION_TOKEN)
- Added isDirty() checks
- fixed test tearDown

* Working fix suggestion for #827
@Jawnnypoo
Copy link
Member

Fixed with #831

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

No branches or pull requests

4 participants