FYI I opened <https://github.com/apollographql/apollo-android/issues/2899> which is super weird, and...

# apollo-kotlin

[wasyl]

FYI I opened https://github.com/apollographql/apollo-android/issues/2899 which is super weird, and we spent better part of recent days chasing it down. I’m available for clarification if needed, esp. if it’s non-reproducible for Apollo folks

[mbonnin]

Thanks for the awesome reproducer, I can reproduce as well

[mbonnin]

If you get a chance to try https://github.com/apollographql/apollo-android/pull/2900, that fixes it for me.

[wasyl]

Is it published somewhere?

[wasyl]

Btw what do you mean by

NETWORK_ONLY watcher was cancelled when trying to refetch

does the first query refetch something in my repro? Or it’s just that it’s cancelled right after it emits first value that’s the issue?

[mbonnin]

Is it published somewhere?

Not yet, there will be snapshots once it is merged. If you have the repo checked out locally, you can do ./gradlew publishToMavenLocal and add mavenLocal to your repositories to try it out.

[mbonnin]

does the first query refetch something in my repro?

Yea, the first query is a watcher so technically it refetches with a cache_first refetch policy but gets cancelled as soon as the network finishes because the flow gets cancelled

[mbonnin]

If you replace the first query toFlow() by .await() you won't have the problem

[wasyl]

Thanks a lot 🙂 We can’t apply .toFlow() -> .await() fix in our codebase because abstractions, also we create watcher quite early and there’s no .await() on that. And testing on CI would be slightly too complicated if we had to build the library locally, so we’ll have to wait for a snapshot. I could only confirm I don’t observe the issue on my repro project anymore with the snapshot

[mbonnin]

👍👍I just merged the pr. Snapshots should start to appear in ~30min

[wasyl]

🙏 we’ll test it first thing tomorrow 🙂

[wasyl]

Although how the fix works escapes me 😉 In the PR you’re delaying the error that happens in onCacheRecordsChanged

[wasyl]

Is the RealApolloStore#publish called from somewhere else than the networkOnly watcher? I thought the issue is that cancelling the watcher also interrupts publish call

[mbonnin]

The store has 2 listeners. The first one throws so the second one doesn't get the update

[mbonnin]

And the second one is the one that we want to get the update

[mbonnin]

Tbh I think the first one shouldn't throw. It might be ok call refetch in 'cancelled' state

[mbonnin]

But that's a more intrusive fix which I'd keep for 3.0

[wasyl]

So the ApooloStore#publish is just posted on some executor, this call is not interrupted by anything?

And the race condition is that with working case, we’d have:
• network call completes, posts runnable to publish cache changes
• watcher is canceled, unsubscribes recordChangeSubscriber
• store publishes the changes
And the not working case would be that
• network call completes, posts runnable to publish cache
• store starts publishing the changes, gets the first subscriber
• first() cancels the flow, so the watcher is cancelled
• store calls the first subscriber’s callback but it’s already canceled and throws exception
?

[wasyl]

Feel free to let me know if this is too much to explain 🙂 I just want to understand better how Apollo works internally, as it’s quite helpful sometimes

[wasyl]

Seems the most recent snapshot fixes the issue for us 🎉 Thank you 🙂

[wasyl]

Is there any release planned soon or not yet?

[mbonnin]

Seems the most recent snapshot fixes the issue for us

Nice glad to hear that! 🎉

Is there any release planned soon or not yet?

There's not much activity in the main branch these days so releases tend to stretch a little bit. Ok if I do one on monday? I like Monday releases

[wasyl]

Sure, no worries 🙂

[mbonnin]

I just want to understand better how Apollo works internally, as it’s quite helpful sometimes

Sorry I missed the question above. No problem at all, brainstorming is good and we can have more people know about the internals 👍

[mbonnin]

Yup what you described seems accurate

[mbonnin]

Nitpicking, I think in the not working case this happens:
• network call completes, posts runnable to publish cache
• first() cancels the flow, so the watcher is cancelled
• the runnable is dispatched
• store calls the first subscriber’s callback but it’s already canceled and throws exception
• the second subscriber (cache_only) doesn't get the store update

[mbonnin]

The publishing is all synchronous, I don't think it can be interrupted

[wasyl]

If the watcher is cancelled, it unregisters the listener though, right? So the cancelling would’ve had to happen already after dispatching the runnable and acquiring reference to still-not-unregistered listener?

[mbonnin]

Mmm good call

[wasyl]

I think that’s why it’s happening so rarely. So my last question: if the publish call is just a runnable on a dispatcher, what happens with the exception? Is it thrown just to stop some potential further processing? Is it logged? Because as I understand it shouldn’t be propagated anywhere to the app code

[mbonnin]

So the cancelling would’ve had to happen already after dispatching the runnable and acquiring reference to still-not-unregistered listener

Right, would need to happen somewhere here:

val recordChangeSubscriber: RecordChangeSubscriber = object : RecordChangeSubscriber {
    override fun onCacheRecordsChanged(changedRecordKeys: Set<String>) {
      if (dependentKeys.isEmpty() || !areDisjoint(dependentKeys, changedRecordKeys)) {
        // scheduling would need to happen somewhere here 
        refetch()

[mbonnin]

what happens with the exception

Excellent question, it's lost forever I'm afraid

[mbonnin]

The semantics of all these dispatchers and and callbacks aren't great TBH, I'm hoping we can do something better with coroutines

[mbonnin]

Thinking about this a bit more, I think we should consider errors in subscribers as fatal errors, I don't see a reason to allow failure there

[mbonnin]

The proper fix is maybe to silently ignore a refetch after a cancel

[mbonnin]

(and crash if someone throws in a store listener)

[mbonnin]

but that scares me a bit 😅

[wasyl]

I think I got it 🙂 I think the missing piece for me was that publishing cache updates was a posted runnable, and not part of making an actual call. That is, I though writeToCacheAsynchronously=false would mean that publishing is also synchronous. But it’s not, just saving to cache is synchronous, publishing the changes is not

[mbonnin]

The fix is released: https://github.com/apollographql/apollo-android/releases/tag/v2.5.3