Integrate retryability for outgoing mutation queue

[wooj2]

Work-in-progress. need to find a way to plumb through the hostname from the client config

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[palpatim]

Let's add a default interval to make this more foolproof rather than force-unwrapping; or make the retryInterval non-optional so we know it's always set.

[palpatim]

I know these mutation operations are enqueued on a serial Operations queue; is any of the internal work being done in a way that makes this not-thread-safe? My quick scan makes me think everything is pretty well serialized, but wanted you to confirm since you're closer to this code.

[wooj2]

Just to double check, your question revolves around executing multiple SyncMutationToCloudOperations running at the same time such that we could run into a situation where our code is not thread safe? (not that we'd ever want to do this, because we want to drain the mutation queue one at a time in case any of the data changes)

But... I looked at the code and thought about implications of instance variables if we were to create multiple operations and run them at the same time, and didn't see a situation where we would be overwriting data.

[palpatim]

No, I'm actually concerned about multiple operations happening on a single operation--e.g., a network callback being invoked at the same time as we're trying to make the outgoing call b/c of a retry.

[wooj2]

I see... hmmm.. I suppose it is possible that our timer could wake up at the same exact time that we have a receive() through combine (e.g. a network event).

In this case, I think a pragmatic solution could be to introduce something (atomic write to a variable?) in our notifyCallback function which allows the "first event" to win.

[palpatim]

I'm just now realizing that this is singleton pattern isn't going to work if we want reachability notifiers for different APIs (e.g., one each for a GraphQL and a REST endpoint). I think we're going to need these to be straight up instances. We could consider a Factory that maintains cached reachability notifiers per host and creates or returns those instances at need.

[wooj2]

Thanks for pointing this out.. I will revert this.

Hmm, i think we still want the API Category to own the NetworkReachability stuff since it's closest to knowing what the endpoint is... but this means we'll either need to do something like expose a Combine publisher to the Datastore category so that our MutationRetryNotifier can subscribe (or something to that effect)

I'll dig into this.

[wooj2]

In the short term, I'm going to make the NetworkReachabilityNotifier as an optional -- until we figure out out how to plumb this between API and Datastore.

[wooj2]

Assuming circle ci tests pass, we should be able to merge these changes into master. With the following outstanding items to handle:

• Update model to not wake up on 429 errors
• Add tests around deinit for mutation retry notifier
• Dig into errors for reachability
• Migrate reachability into API category and figure out a way to expose the publisher to datastore.

[wooj2]

Squashed commits.

[palpatim]

1. Clarified of my multi-threading request, but not a ship blocker
2. I assume that the "resolve notifier" is the next thing you're working on, or else filed in the issue backlog. Assuming so, ship it!

[wooj2]

This is where the bug got introduced. Based on the comment of "bubbling up errors" i had introduced (unwittingly not noticing that we were already bubbling up the error):

  if let apiRequest = createAPIRequest(mutationType: mutationType) {
            makeAPIRequest(apiRequest)
+  } else {
+        finish(result: .failed(...)
+ }

This basically was causing us to call finish twice, which caused the unit tests to crash(because expectation's fulfill() calls were being called more than expected.

So, to remedy this, i made createAPI request simpler by moving out the checking of the graphQLMutation type, so we can easily see that the function is indeed handling the bubbling up of those errors.