Fix retain cycle

[gpambrozio]

This change turned a weak reference into a strong one and created a cycle that might keep all watch objects in memory.

This pull request adds a test that fails without my fix and succeeds otherwise. Test was pushed in a first, separate commit to make it easier to check out the branch at that commit and see the error.

This might be related to this story

[apollo-cla]

@gpambrozio: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[designatednerd]

Ohhhh, that's interesting - so moving it to an Atomic actually caused a retain cycle since that created an extra non-weak reference to the Cancellable. Sneaky!

[AnthonyMDev]

Thanks so much for this fix! Next we need to test if this actually fixes #516 or if there is more work to do there.

[gpambrozio]

@AnthonyMDev any idea of when this will make it into a release? I need this for my project and would love to not have to point to my fork.

[AnthonyMDev]

So, we've had some transient test failures in our CircleCI builds recently that I have been trying to track down, but haven't been able to repro locally.
https://app.circleci.com/pipelines/github/apollographql/apollo-ios

The test failures are always in SQLiteWatchQueryTests. But after merging this in, I just ran another job and THIS test (the retain cycle one you just wrote) failed. I've got a suspicion that this has nothing to do with your tests, but is the same underlying issue that was causing our other transient test failures.
https://app.circleci.com/pipelines/github/apollographql/apollo-ios/1891/workflows/939d5adb-5ad2-419d-ae52-40278a4d4d22/jobs/20644

As soon as we can figure out what is causing this and fix that issue, I'll release a new version. I'm working on something now, but it's a shot in the dark.

[gpambrozio]

@AnthonyMDev I had test issues and this line is what solved it as you need a new runloop to clean up memory. It might be that CI needs more time. On our project we have different timeouts for ci and local because our ci uses virtual machines and those are very slow and unreliable.

[AnthonyMDev]

Thanks for the info. That might help! Looking into it currently!

[AnthonyMDev]

I think I just fixed this more elegantly by creating a "Busy Assertion". I just created an XCTAssertTrueEventually function that runs the loop, checks, then runs the loop again until it passes or hits a timeout.

It's included in this PR #1830

I'll talk to @designatednerd about pushing a release tomorrow, or at least sometime this week, to include this fix! Thanks @gpambrozio

[AnthonyMDev]

@gpambrozio This release has been pushed!