parse-community / Parse-SDK-iOS-OSX

The Apple SDK for Parse Platform (iOS, macOS, watchOS, tvOS)
https://parseplatform.org
Other
2.81k stars 865 forks source link

SaveEventually and sudden termination - silent failures? #359

Closed geraldhinson closed 5 years ago

geraldhinson commented 8 years ago

My app uses the following model to save objects.

saveAnObject (PFObject *obj)
{
    // save locally first
    //
    [obj pinInBackgroundWithBlock:^(BOOL success, NSError *error)
     {
         if (success == true)
         {
             // local save succeeded - now save to cloud
             [obj saveEventually];
         }
         else
         {
             // local save failed
             //
             .... do error processing ....
         }
     }];
}

However, after reading this http://stackoverflow.com/questions/32617917/parse-saveeventually-failing-when-app-is-terminated-quickly and then reading the parse ios/osx code on github it appears that saveEventually returns after allocating a task and (important) prior to writing anything to disk. In other words it seems quite possible that a saveEventually could fail silently if an app is killed prior to the task allocated having a chance to persist the "save to cloud" request.

True?

If so, this probably explains why I occasionally see objects in my local store that are never persisted in the cloud. From the documentation I concluded that saveEventually was persisting / logging the save request to disk prior to returning control to the application.... ie. classic write-ahead logging to ensure the data was not lost in the event of untimely app termination.

Assuming the above is true, is there any way for me to query the local datastore to determine if there is data that has not yet been persisted to the cloud (ie. to detect saveEventually failures?).

richardjrossiii commented 8 years ago

Yes, that is true. All IO in the SDK is non-blocking of the calling thread, which is very critical for any user-facing application, as the last thing you want is your UI stuttering while you are attempting to write to disk.

(In fact, even our synchronous APIs don't do IO on the calling thread - the calling thread just does a semaphore wait until the task is completed, while letting the work still be done in the background).

Most applications shouldn't ever run into 'sudden termination' (e.g. crashes), and if you're having these consistently I'd strongly recommend solving the source of these crashes rather than try and patch them up at the data model level...

In terms of having some way of knowing when the objects are persisted locally, this may be a use-case for issue #319 to be implemented, though I'm still not a huge fan of emitting NSNotifications...

If you really must detect these edge cases, I suppose it would be possible to query all local objects with a nil objectId on launch, and emit additional saveEventually calls for them...

That would, of course, require having a fixed set of class names to search for, but I'd wager that most applications would have such a fixed list.

nlutsenko commented 8 years ago

In the case of sudden termination - saveEventually here is going to behave the same way as pinning to Local Datastore, meaning if app was killed before the object was pinned (before you got to the callback) - it's not going to be persisted locally.

What saveEventually does is basically wraps the logic around pinning the object, saving it to the server, unpinning the object. As well as it includes code for executing these saves across app executions.

You can achieve extra guarantee here by wrapping saveEventually calls into pinning the object once again to a pin with specific name (e.g. unsavedObjects) and before calling into saveEventually - pin objects to that pin, after saveEventually succeeds - unpin objects from that pin.

geraldhinson commented 8 years ago

Interesting comments above (and thanks for reformatting that code block for me).

To be clear, the app is not suffering from sudden crashes that need fixing. That said, no app (my code) nor library (your code) is perfect and there needs to be a model for knowing with certainty that a new or updated object is truly saved at the app level and a model for recovering from errors when this is not the case.

I suspect (but have not proven) that what is occurring is an app termination on IOS because either:

1) the app has been moved to the background and IOS nukes it to reclaim the memory for some other app. Apple documents very clearly that all apps / libraries MUST handle this scenario. OR 2) the user is killing the app explicitly using the IOS interface (the good 'ol double punch, swipe up)

If I understand your codebase correctly, either of these occurrences prior to the parse library actually writing to disk the data required for saveEventually semantics will cause a loss of data. (And this is what the stackoverflow link (above) documented occurring as well).

I'll check the device in question to see if I have objects that are missing your server-assigned object id as you suggest. If this is the case, then I can recover from the issue quite easily, but I don't believe that I should be dealing with this issue at the app level.

Also, to further clarify the semantics a bit:

Given the current design (api returns after a task is allocated) it seems that an app can never assume that data is actually saved when using saveEventually (non-callback version at least)? The semantics are "kick off a queue-data-for-the-cloud-task and pray it completes", right?

I'll update this thread after I've had a chance to examine the local datastore objects that did not make it to the cloud to see if indeed they are missing their object ids still. It seems the next step here depends upon that answer.

geraldhinson commented 8 years ago

OK, just retrieved the device that has 3 object in its local datastore that were not successfully queued to the cloud. As suspected, all 3 objects have no objectId assigned and as stated earlier there are no crash logs for the app. The person using the app on the device in question also reported that the app seemed to working fine with no visible crashes during use.

Thoughts?

geraldhinson commented 8 years ago

Richard

Per your recommendation above:

"If you really must detect these edge cases, I suppose it would be possible to query all local objects with a nil objectId on launch, and emit additional saveEventually calls for them..."

I wrote the following:

void
_ParseCloud::Sync_RedoSaveEventually (char* pClass)
{
    NSString* nsClassName = [NSString stringWithUTF8String:(const char *) pClass];

    printf ("\nSync_RedoSaveEventually: Syncing <%s> db\n", pClass);

    PFQuery *query = [PFQuery queryWithClassName: nsClassName];
    [query fromLocalDatastore];
    [query whereKeyDoesNotExist:@"objectId" ];
    query.limit = 1000;

    [query findObjectsInBackgroundWithBlock:^(NSArray *objects, NSError *error)
     {
         if (!error)
         {
             if (objects.count == 0)
             {
                 return;
             }

             NSLog(@"Successfully retrieved %lu rows without objectId assigned.", (unsigned long)objects.count);

             // saveEventually failed to do its thing... try again
             for (PFObject *object in objects)
             {
                 Sync_ForceSyncByOtherDevices (object);  <--- this just changes one of the values
                 saveAnObject (object); <-- this is shown in original question
             }
         }
         else
         {
             // Log details of the failure
             NSLog(@"Error: %@ %@", error, [error userInfo]);
             sprintf (m_LoggedError, "%s", error.localizedDescription.UTF8String);
         }

     }];
}

This successfully finds the rows that are in the local datastore but not in the cloud, BUT calling saveEventually on them again results in a row in the cloud table / class with most values undefined. (ie. the 2nd saveEventually did not propagate the values from the local datastore...)

The results can be seen here: https://dl.dropboxusercontent.com/u/9116971/ParseCloudWithUndefinedFields.png

So, the "find all objects missing an objectId and issue saveEventually() on them again" doesn't seem to work as expected. What would you recommend?

geraldhinson commented 8 years ago

Oh, forgot to mention, I also tried simply issuing a saveEventually call in the for *object in objects loop above. This resulted in no change locally and no objects saved to the cloud either.

So, I modified it to change one of the fields and then issued my usual pin followed by saveEventually. This caused the empty objects to appear in the cloud but did NOT result in an objectId being assigned to the objects in the local datastore... they show up with objectId missing even after the logic above is run and the empty objects are saved to the cloud.

geraldhinson commented 8 years ago

Richard, any further advice on how to get my objects saved to the cloud since the additional saveEventually call on the ones in the local datastore without objectIds assigned didn't work?

geraldhinson commented 8 years ago

Update on this after upgrading from Parse v1.8.3 to v1.8.5 today.

After re-building with 1.8.5 I re-ran the logic that searched for objects in the local datastore missing their objectIds then reissued saveEventually calls on them as such:

''' void _ParseCloud::Sync_RedoSaveEventually (char* pClass) { NSString* nsClassName = [NSString stringWithUTF8String:(const char *) pClass];

printf ("\nSync_RedoSaveEventually: Syncing <%s> db\n", pClass);

PFQuery *query = [PFQuery queryWithClassName: nsClassName];
[query fromLocalDatastore];
[query whereKeyDoesNotExist:@"objectId" ];
query.limit = 1000;

[query findObjectsInBackgroundWithBlock:^(NSArray *objects, NSError *error)
 {
     if (!error)
     {
         if (objects.count == 0)
         {
             return;
         }

         NSLog(@"Successfully retrieved %lu rows without objectId assigned.", (unsigned long)objects.count);

         // saveEventually failed to do its thing... try again
         for (PFObject *object in objects)
         {
             [object saveEventually];
         }
     }
     else
     {
         // Log details of the failure
         NSLog(@"Error: %@ %@", error, [error userInfo]);
         sprintf (m_LoggedError, "%s", error.localizedDescription.UTF8String);
     }

 }];

} '''

This behaved a bit differently from 1.8.3 in that the local datastore objects returned WERE assigned an object id, so subsequence runs of this logic no longer found them. However, the objects stored in the cloud were still as before with almost all of their fields undefined.

So, there are 2 issues: 1) the original call to saveEventually did not do the save. 2) subsequent calls to saveEventually result in incorrect objects being stored in the cloud.

If I can deal with #1 as it is pretty easy to detect via the missing objectId, but I can't go live with this app unless I can get past #2 above.

Please advise.

geraldhinson commented 8 years ago

Richard, et al:

As far as the objects getting saved with undefined fields part of my question here, could it be related to this issue: https://github.com/ParsePlatform/Parse-SDK-iOS-OSX/issues/131

It looks pretty similar to me.

richardjrossiii commented 8 years ago

@geraldhinson It's most likely not related, but if your object graph is cyclic (e.g. A -> B -> A), pinning it can sometimes lose information, especially if the entire object graph has yet to be fetched into memory yet.

Due to the LDS architecture, for performance reasons we cannot fetch the entire object graph & subgraph before re-pinning it, as we could very quickly run out of memory for any complex object graph, and so we limit our search recursion to a depth of 1.

If you can simplify your object model it may solve this problem for you.

geraldhinson commented 8 years ago

I don't use pointers at all - so no cycles. Simple "singleton" objects across the board in my app.

geraldhinson commented 8 years ago

I've uploaded a bug repro project here: https://dl.dropboxusercontent.com/u/9116971/ParseReproSaveEventuallyBug.zip

This illustrates two problems: 1) objects can be lost after saveEventually returns and either IOS terminates an app in the background to reclaim its memory (not an app bug) OR the user terminates an app using the IOS task manager (also not a bug)

2) reissuing a saveEventually on the objects that were successfully pinned in the local datastore, but were never saved to the cloud (hence missing an objectId) does not result in them being saved to the cloud

My app has objects in the local datastore that were not saved to the cloud because of this issue (which I consider a "bug" not a "discussion" as this issue is currently labeled for some reason). I need to hear from you parse-ians how to recover this data because other devices that are sync'ing to the cloud need it as well for app correctness.

Thanks!

geraldhinson commented 8 years ago

Richard, I read the article you reference above. I am aware that saveEventually does not save objects in order (and that it sometimes saves duplicate objects as well), but this is not what is occurring in the case I have outlined here. This being the world of no-sql vs. relational with ACID semantics I kinda assumed that would be the case. My app is resilient to those issues.

What I am reporting here is a loss of data entirely between the local datastore and the cloud when using saveEventually that is not related to bugs in an app.

Regarding the loss of data it seems that something DOES get updated (in the local datastore I assume?) by the first (failed) saveEventually call that causes subsequent saveEventually calls to be ignored. (Logically) it looks as if the failed saveEventually call updates the local datastore object as being cloud-saved even though it wasn't. I illustrate this issue in the repro project I sent - issuing subsequent saveEventually calls on the objects missing objectIds locally (the same ones missing from the cloud) has no effect. If this were not the case (or if you know of a workaround?) then I could easily detect and recover from the saveEventually-caused issues in my app logic.

Please let me know if there is a work-around. This is a show-stopper bug.

Btw, I confirmed the issue occurs on OSX as well as IOS (I assumed it would given the shared codebase). It's harder to catch on OSX given things tend to run a lot faster than on IOS devices.

geraldhinson commented 8 years ago

It's been quite a long time since I asked for some sort of workaround to this issue. Is an answer forth-coming?

Per Richard's recommendation above, it is fairly easy for me to find objects that do not have an object id assigned. However, this does not (from what I understand) distinguish between objects for which saveEventually has failed and thus will not be saved to the cloud in the future vs. those which are queued and just have not yet been saved to the cloud.

Bottom line: I need either a reliable way to save objects both to the local datastore and the cloud and/or a reliable way to detect and re-save to the cloud objects for which saveEventually has failed.

TomWFox commented 5 years ago

Closing this as no one has reported this issue recently, and we don't have the resources to investigate these historic reports.

Please ask me to reopen if you experience this issue in the latest version of this SDK.

geraldhinson commented 5 years ago

Nope. After I provided a sample that proved this was a bug and was then ignored for months I converted my app to SQLite and moved on.

Cheers.

On Thu, Mar 28, 2019 at 9:10 AM Tom Fox notifications@github.com wrote:

Closed #359 https://github.com/parse-community/Parse-SDK-iOS-OSX/issues/359.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/parse-community/Parse-SDK-iOS-OSX/issues/359#event-2236814228, or mute the thread https://github.com/notifications/unsubscribe-auth/AJRwEbdrfU8atHTDMxql0m0PNTVnXw-5ks5vbOmNgaJpZM4GIPxt .

-- Gerald Hinson

TomWFox commented 5 years ago

Sorry about that, obviously it’s hard for us to manage this SDK when there are unsolved issues pre parse server, but we are doing our best and hopefully at some point this SDK will see some more love from a committed maintainer.

geraldhinson commented 5 years ago

No worries. Facebook torpedoed it, not you.

Be aware there is a bug if queuing msgs to be delivered to the cloud. The scenario I documented occurred when the msgs were queued to the device while there was not connectivity to deliver them. If in that scenario the parse-based application was terminated (as IOS is prone to do while it is in the background consuming memory) prior to connectivity being reestablished then queued db rows can be lost (ie. never delivered). Back when I reported this myself and another person both had documented the bug and I also produced a simple app that reproduced it to illustrate the issue. That bug-repro app is long gone at this point. Just replying here in case of interest to know more context.

Good luck with Parse. I was sad to see it go - very cool idea (MBaaS) - but execution matters and being bought by FB was probably not helpful either.

Cheers, -Gerald Hinson

On Fri, Apr 12, 2019 at 12:51 AM Tom Fox notifications@github.com wrote:

Sorry about that, obviously it’s hard for us to manage this SDK when there are unsolved issues pre parse server, but we are doing our best and hopefully at some point this SDK will see some more love from a committed maintainer.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/parse-community/Parse-SDK-iOS-OSX/issues/359#issuecomment-482475949, or mute the thread https://github.com/notifications/unsubscribe-auth/AJRwESaWVCW8O-RnM_6Ddvvo6UfPKofHks5vgDsPgaJpZM4GIPxt .

-- Gerald Hinson