Save eventually issue #831

cjosepha · 2018-05-24T17:45:52Z

Failing test for issue #827

- The eventually queue should be cleared at each test execution but Parse.getEventuallyQueue().clear() crashes, that's why line 1585 is commented

* master: Correction to gcm artifact name (parse-community#830)

flovilmart · 2018-05-24T18:01:59Z

Awesome ! Thanks! Any chance you had the opportunity to identify what’s deadlocking?

cjosepha · 2018-05-24T18:16:56Z

@flovilmart unfortunately no. Also, to be honest, I'm not sure this test reproduces the exact same issue I have in my app because behavior is a little bit different. So maybe it's not a deadlock.
Can you review this test to make sure mocking is done properly?

flovilmart · 2018-05-24T18:35:40Z

@dangtz I'm not an Android expert but likely @rogerhu or @Jawnnypoo may have a better grasp.

Seems that the assertion that is failing is this one Is it what you'd expect?

cjosepha · 2018-05-24T18:48:54Z

@flovilmart I expected that it fails on another assertion : this one.
In my app test, the first time saveEventually() is called, it returns the invalid session error properly, then all calls to save() should freeze.
Here the first call to saveEventually() freezes, that's why I'm wondering if test conditions are right.

flovilmart · 2018-05-24T18:54:24Z

Doesnt't look it's what's failing on travis, I'll have a look with your code.

cjosepha · 2018-05-24T19:03:04Z

@flovilmart sorry, I misunderstood : yes this test is expected to fail here (ParseUserTest.java:1624).
Thanks!

flovilmart · 2018-05-24T19:03:36Z

Ah good then! Thanks!

…produce exactly issue parse-community#827

* master: Match up all docs with artifact names (parse-community#836) publish fcm / gcm on their subpath (parse-community#835)

cjosepha · 2018-05-29T15:38:49Z

@flovilmart now the test fails at the right line.

We can see that:

The first user.saveInBackground() succeeds
Then user.saveEventually() returns the expected invalid session exception
And then the second user.saveInBackground() freezes.

flovilmart · 2018-05-29T15:40:07Z

Awesome! That helps a lot!

cjosepha · 2018-05-29T15:43:07Z

Yes, now I'm sure the test is right, I will be able to see why the 2nd save freezes. Will let you know.

flovilmart · 2018-05-29T15:44:29Z

Awesome! If you have any questions, don’t hesitate to ask’ we really appreciate you take the time to dive in! That’s the way to go!

cjosepha · 2018-05-29T20:25:52Z

Unfortunately, the reason why the save freezes here is due to the way the server response is mocked here : the same instance of ParseHttpClient should be kept instead of creating a new one. So a ParseException with "bad json response" message is thrown.

…SessionIsInvalid()

coveralls · 2018-05-29T20:57:50Z

Coverage increased (+10.6%) to 69.579% when pulling 70516c3 on DanGTZ:save-eventually-issue into 2817393 on parse-community:master.

codecov · 2018-06-01T19:43:39Z

Codecov Report

Merging #831 into master will increase coverage by 9.77%.
The diff coverage is 75%.

@@             Coverage Diff              @@
##             master     #831      +/-   ##
============================================
+ Coverage     54.61%   64.39%   +9.77%     
- Complexity     1709     1906     +197     
============================================
  Files           123      123              
  Lines          9785     9796      +11     
  Branches       1372     1375       +3     
============================================
+ Hits           5344     6308     +964     
+ Misses         4013     2980    -1033     
- Partials        428      508      +80

Impacted Files	Coverage Δ	Complexity Δ
Parse/src/main/java/com/parse/ParseObject.java	`59.52% <100%> (+8.36%)`	`234 <0> (+29)`	⬆️
Parse/src/main/java/com/parse/Parse.java	`57.07% <42.85%> (+35.71%)`	`28 <2> (+17)`	⬆️
...rse/src/main/java/com/parse/OfflineQueryLogic.java	`66% <0%> (+0.87%)`	`132% <0%> (+5%)`	⬆️
...rse/src/main/java/com/parse/ParseSQLiteCursor.java	`1.69% <0%> (+1.69%)`	`1% <0%> (+1%)`	⬆️
...rse/src/main/java/com/parse/ParseCommandCache.java	`2.18% <0%> (+2.18%)`	`3% <0%> (+3%)`	⬆️
Parse/src/main/java/com/parse/ParseUser.java	`79.59% <0%> (+2.44%)`	`117% <0%> (+10%)`	⬆️
...in/java/com/parse/CachedCurrentUserController.java	`93.96% <0%> (+2.58%)`	`15% <0%> (ø)`	⬇️
...arse/src/main/java/com/parse/ParseRESTCommand.java	`86.36% <0%> (+3.18%)`	`57% <0%> (+5%)`	⬆️
Parse/src/main/java/com/parse/PLog.java	`63.33% <0%> (+3.33%)`	`11% <0%> (+1%)`	⬆️
...arse/src/main/java/com/parse/ParseCorePlugins.java	`67.22% <0%> (+4.99%)`	`51% <0%> (+3%)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2817393...70516c3. Read the comment docs.

rogerhu · 2018-06-03T07:30:09Z

It seems like the deadlock is happening inside ParseObject in the waitFor() call? (

Parse-SDK-Android/Parse/src/main/java/com/parse/ParseObject.java

Lines 1603 to 1604 in ff3e27b

    
           return task.onSuccessTask( 
        
               TaskQueue.<Void>waitFor(toAwait)

)

If I had to guess, it would seem that the task queue never quite runs quite right after sessions are closed (because unhandled exception occurred)

cjosepha · 2018-06-03T16:53:03Z

@rogerhu Yes deadlock occurs there but I can't reproduce it with this test.

natario1 · 2018-06-03T17:18:12Z

I have not followed the conversation with attention but, as someone who has gone crazy on #646 , this smells like it could be it. Just 2 cents, maybe it might save you some time.

That one is an issue with the definition of the default pool thread of bolts. The pool size there depends on the machine processor, so I can see how your tests that run on the PC go well, while the app hangs because your smartphone has less CPU cores.

AFAIK Tasks should not hang if there are unhandled exception. But they hang if you do too much (allocate too much threads with dependencies between them. Task.wait() is a typical example).

cjosepha · 2018-06-04T18:07:48Z

@natario1 I had a look at #646 conversation a few days ago and I would say that it doesn't seem to be related to this issue.

This unit test doesn't reproduce the lock because it misses some code to create the conditions of an app relaunch, and then attempt to save again. This is exactly what to do, on a fresh install, to make the lock occur, whatever the device. You can try it with my test app mentioned in #827

…Object.java:L1604 occurs

cjosepha · 2018-06-10T13:29:53Z

I've updated the test and now it reproduces the deadlock responsible for #827

cjosepha · 2018-06-13T17:50:39Z

Is there a design/spec document describing how the ParseObject.operationSetQueue is expected to work?

I found that the problem is the way this queue it's managed after a saveEventually :

In case of success, this queue is modified and persisted to LDS
In case of failure, this queue is modified and not persisted to LDS
I have tested a fix that seems to work well, but I need to know the expected behavior to make sure the fix is done the right way.

flovilmart · 2018-06-13T18:01:45Z

@dangtz can you shoot away the fix so we can have a look and evaluate from there?

* 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 · 2018-06-13T18:43:19Z

On my side I get regression only at ParseInstallationTest.java:193 and ParseInstallationTest.java:151 but I can see that on Travis there is another regression on ParseCorePluginsTest.java:41

About the ParseInstallation tests : can someone explain why saveAsync is expected to be called twice?

flovilmart · 2018-06-13T18:45:09Z

can someone explain why saveAsync is expected to be called twice?

I believe @dangtz a better question is why is the save called less times now :)

cjosepha · 2018-06-13T19:08:47Z

@flovilmart indeed it is :)
This is because a CACHE_MISS exception is thrown here due to test conditions.

cjosepha · 2018-07-06T22:06:39Z

@flovilmart when is the review planned? Do you need more info?

flovilmart · 2018-07-07T04:35:01Z

@parse-community/android can you guys get a look?

Jawnnypoo · 2018-07-18T15:42:12Z

Just to clarify @dangtz , does this solve the save eventually issue? Or does it create a test that shows how to reproduce it?

cjosepha · 2018-07-18T21:29:25Z

@Jawnnypoo this does both : test and solve. The fix is a modification of the ParseObject. handleSaveResultAsync() method to save the state of the object in LDS when the saveEventually fails.
The test ParseUserTest.testSaveEventuallyWhenServerError() will fail if you remove the fix.
Note that I had to also modify an existing test for my fix to pass : 0f5e5af#diff-5a838ec813bd191cc3fd2173ff95b238

Jawnnypoo · 2018-07-19T13:48:46Z

@dangtz Can you review some of the comments I made when you get a chance?

cjosepha · 2018-07-19T23:16:38Z

@Jawnnypoo sure! Where can I see them ?

Jawnnypoo · 2018-07-19T23:18:28Z

I believe they are marked in this PR if you go to the Files changed tab

cjosepha · 2018-07-19T23:23:57Z

Sorry I can't see any comment in Files changed tab.

Jawnnypoo · 2018-07-19T23:30:03Z

Can you see them here? https://github.com/parse-community/Parse-SDK-Android/pull/831/files/70516c3fb3452c521e4ce26e9287d806bff0fda6

cjosepha · 2018-07-19T23:33:34Z

No, sorry, only code.

Jawnnypoo · 2018-07-19T23:35:18Z

Even if you scroll through it? The comments appear inline, within the files

Jawnnypoo · 2018-07-18T21:48:17Z

Parse/src/main/java/com/parse/Parse.java

@@ -256,7 +260,11 @@ public static void initialize(Configuration configuration) {
    // isLocalDataStoreEnabled() to perform additional behavior.
    isLocalDatastoreEnabled = configuration.localDataStoreEnabled;

-    ParsePlugins.initialize(configuration.context, configuration);
+    if (parsePlugins == null) {


@dangtz Can you explain what the reasoning is for these changes here?

These changes allow to initialize Parse with a mock ParsePlugins object, for test purpose.
The mock ParsePlugins object is needed to reproduce the issue, and calling Parse.initialize() is also needed to reproduce it. These changes are the solution to have both.

Jawnnypoo · 2018-07-18T21:48:49Z

Parse/src/main/java/com/parse/Parse.java

@@ -311,6 +319,8 @@ public Void then(Task<Void> task) throws Exception {
  }

  static void destroy() {
+    ParseObject.unregisterParseSubclasses();


Was this causing problems? Curious if this was added to help the tests pass, or if it was related to the bug?

This was not causing the problem, and yes it's to help test teardown here by aggregating deallocations. Note that Parse.destroy() was not used anywhere in the project before my changes.

Jawnnypoo · 2018-07-18T21:49:11Z

Parse/src/main/java/com/parse/Parse.java

@@ -322,6 +332,8 @@ static void destroy() {

    ParseCorePlugins.getInstance().reset();
    ParsePlugins.reset();
+
+    setLocalDatastore(null);


Same question here, is this for the tests, or related to the bug?

For tests, as Parse.destroy() is only used in tests.

Jawnnypoo · 2018-07-19T23:36:25Z

That was my fault, I guess I didn't submit the review 🤔

cjosepha · 2018-07-20T00:53:54Z

No problem, replied to your comments.

cjosepha

@Jawnnypoo could you share your opinion about this?

cjosepha · 2018-07-20T02:06:59Z

Parse/src/main/java/com/parse/ParseObject.java

+            @Override
+            public Task<Void> then(Task<Void> task) throws Exception {
+              if (task.isFaulted()) {
+                return Task.forResult(null);


This is to not propagate the exception in case fetching the store here fails. If exception was propagated, existing ParseInstallationTests would fail (testMissingRequiredFieldWhenSaveAsync and testObjectNotFoundWhenSaveAsync) because they don't create the right conditions for the fetch to work (a CACHE_MISS exception is then thrown). Maybe we should update those tests instead, by creating the right conditions, and let the exception propagate by using onSuccessTask at line 1451 as on my previous commit.

Yep, I think it is best to update the tests to make them have the right conditions for success/expected failure rather than modifying the library to work with the expectations of the tests. I think we can go ahead and merge this in though, as it fixes the issue at hand, and keep this in mind for potentially another pull request if that is cool with you.

cjosepha added 3 commits May 24, 2018 12:25

- Added a failing test for parse-community#827

c83fae1

- 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

3e55ffb

Merge branch 'master' into save-eventually-issue

d751e2e

* master: Correction to gcm artifact name (parse-community#830)

cjosepha added 2 commits May 29, 2018 11:18

Fixed ParseUserTest.testSaveEventuallyWhenSessionIsInvalid() so it re…

65934bf

…produce exactly issue parse-community#827

Merge branch 'master' into save-eventually-issue

cc047f6

* master: Match up all docs with artifact names (parse-community#836) publish fcm / gcm on their subpath (parse-community#835)

Fixed server response mocking in ParseUserTest.testSaveEventuallyWhen…

ff0064b

…SessionIsInvalid()

cjosepha mentioned this pull request May 29, 2018

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

Closed

Merge branch 'master' into save-eventually-issue

e86df83

Jawnnypoo and others added 2 commits June 7, 2018 15:46

Merge branch 'master' into save-eventually-issue

df31827

Failing test for parse-community#827 fails when the deadlock at Parse…

6dea925

…Object.java:L1604 occurs

* Suggestion of fix for parse-community#827

605e8a2

* 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

Working fix suggestion for parse-community#827

0f5e5af

flovilmart requested review from rogerhu, Jawnnypoo and hermanliang July 7, 2018 04:35

Merge branch 'master' into save-eventually-issue

70516c3

Jawnnypoo reviewed Jul 19, 2018

View reviewed changes

cjosepha commented Jul 20, 2018

View reviewed changes

Jawnnypoo approved these changes Jul 20, 2018

View reviewed changes

Jawnnypoo merged commit b0b00d1 into parse-community:master Jul 20, 2018

Save eventually issue #831

Save eventually issue #831

Conversation

cjosepha commented May 24, 2018

flovilmart commented May 24, 2018

cjosepha commented May 24, 2018

flovilmart commented May 24, 2018

cjosepha commented May 24, 2018 • edited Loading

flovilmart commented May 24, 2018 • edited Loading

cjosepha commented May 24, 2018

flovilmart commented May 24, 2018

cjosepha commented May 29, 2018

flovilmart commented May 29, 2018 • edited Loading

cjosepha commented May 29, 2018

flovilmart commented May 29, 2018

cjosepha commented May 29, 2018

coveralls commented May 29, 2018 • edited Loading

codecov bot commented Jun 1, 2018 • edited Loading

Codecov Report

rogerhu commented Jun 3, 2018

cjosepha commented Jun 3, 2018

natario1 commented Jun 3, 2018

cjosepha commented Jun 4, 2018 • edited Loading

cjosepha commented Jun 10, 2018 • edited Loading

cjosepha commented Jun 13, 2018

flovilmart commented Jun 13, 2018

cjosepha commented Jun 13, 2018

flovilmart commented Jun 13, 2018

cjosepha commented Jun 13, 2018 • edited Loading

cjosepha commented Jul 6, 2018

flovilmart commented Jul 7, 2018 • edited Loading

Jawnnypoo commented Jul 18, 2018

cjosepha commented Jul 18, 2018

Jawnnypoo commented Jul 19, 2018

cjosepha commented Jul 19, 2018

Jawnnypoo commented Jul 19, 2018

cjosepha commented Jul 19, 2018

Jawnnypoo commented Jul 19, 2018

cjosepha commented Jul 19, 2018

Jawnnypoo commented Jul 19, 2018

Jawnnypoo Jul 18, 2018

Choose a reason for hiding this comment

cjosepha Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Jawnnypoo Jul 18, 2018

Choose a reason for hiding this comment

cjosepha Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Jawnnypoo Jul 18, 2018

Choose a reason for hiding this comment

cjosepha Jul 20, 2018

Choose a reason for hiding this comment

Jawnnypoo commented Jul 19, 2018

cjosepha commented Jul 20, 2018

cjosepha left a comment

Choose a reason for hiding this comment

cjosepha Jul 20, 2018

Choose a reason for hiding this comment

Jawnnypoo Jul 20, 2018

Choose a reason for hiding this comment

cjosepha commented May 24, 2018 •

edited

Loading

flovilmart commented May 24, 2018 •

edited

Loading

flovilmart commented May 29, 2018 •

edited

Loading

coveralls commented May 29, 2018 •

edited

Loading

codecov bot commented Jun 1, 2018 •

edited

Loading

cjosepha commented Jun 4, 2018 •

edited

Loading

cjosepha commented Jun 10, 2018 •

edited

Loading

cjosepha commented Jun 13, 2018 •

edited

Loading

flovilmart commented Jul 7, 2018 •

edited

Loading

cjosepha Jul 20, 2018 •

edited

Loading

cjosepha Jul 20, 2018 •

edited

Loading