Strong retain cycle of self inside of AppSyncSubscriptionWithSync

[LachlanMcCulloch]

Describe the bug
When using any of the AWSAppSyncClient sync api, they return a AppSyncSubscriptionWithSync object, polymorphed as a Cancellable object. When calling cancel() and releasing the reference to this object I noticed that it would not deinitialize and persist to exist and continue its sync behaviour.

I discovered that within the AppSyncSubscriptionWithSync there are two points where self is taken as a strong reference. The first is in performInitialSync():

...
internalStateSyncQueue.addOperation {
    self.registerForNotifications()
    self.initializeLastSyncTimeFromCache()
    self.initializeBaseQueryResultsFromCache()
    self.performSync()
}
...

and the second is in performSync(at:):

...
nextSyncTimer = DispatchSource.makeOneOffDispatchSourceTimer(deadline: deadline, queue: handlerQueue) {
    AppSyncLog.debug("Timer fired, queueing sync operation")
    self.internalStateSyncQueue.addOperation {
        AppSyncLog.debug("Perform sync queued by timer")
        self.performSync()
    }
}
...

By updating both of these calls to take a weak self ([weak self] in), this removes the retain cycle and thus does not cause a memory leak when the returned Cancellable reference is released.

To Reproduce
Steps to reproduce the behavior:
Add the following unit test to existing master branch in AWSAppSyncUnitTests/AppSyncSubscriptionWithSyncTests.swift:

func testCancelWithBaseQueryAndDeinitNoStrongReferenceRetained() {
    var subscriptionWithSync: AppSyncSubscriptionWithSync? = AppSyncSubscriptionWithSync(
        appSyncClient: appsyncClient,
        baseQuery: listPostsQuery,
        deltaQuery: emptyQuery,
        subscription: emptySubscription,
        baseQueryHandler: listQueryHandler,
        deltaQueryHandler: emptyDeltaQueryHandler,
        subscriptionResultHandler: emptySubscriptionResultHandler,
        subscriptionMetadataCache: nil,
        syncConfiguration: .init(baseRefreshIntervalInSeconds: 1),
        handlerQueue: queue)

    weak var weakRef = subscriptionWithSync
    subscriptionWithSync?.cancel()
    subscriptionWithSync = nil
    let expectation = XCTestExpectation(description: "subscriptionWithSync reference deinitialized")
    DispatchQueue.main.async {
        while true {
            if weakRef == nil {
                expectation.fulfill()
                break
            }
        }
    }
    wait(for: [expectation], timeout: 5.0)
    XCTAssertNil(weakRef, "subscriptionWithSync was not deinitialized")
}

Expected behavior
The expected behaviour of the previous unit tests is that once the variable subscriptionWithSync is set to nil, the weak reference previously created (weakRef) should also show as nil.

Screenshots
NA

Environment(please complete the following information):

• AppSync SDK Version: 3.0.0 (master commit hash: 180dc1cc59f5a5132879087331d1fd3fdfc37c93)
• Dependency Manager: CocoaPods 1.8.4
• Swift Version : Swift 5

Device Information (please complete the following information):

• Device: Simulator iPhone 11 Pro Max
• iOS Version: iOS 13.1

Additional Information:
I have created a Pull request here which contains both the fix and the unit test with the failure behaviour previous to the fix:
#343

[royjit]

Hi @LachlanMcCulloch

Thank you for the PR!! We will review this and try to push the change.

[LachlanMcCulloch]

PR was Merged: #343

[LachlanMcCulloch]

Hey @royjit , I noticed that calling cancel on the Cancellable returned by sync (AppSyncSubscriptionWithSync) currently will not properly cancel if there are operations in the internal queue. The proposed fix in #355 should fix it.

This also adds some [weak self] that I previously missed in my last PR.

[LachlanMcCulloch]

#355 is merged, closing issue again. thanks @lawmicha !