Replaced spawn of multiple queues in subscriptions for a single queue… #28

MarioBajr · 2018-05-30T11:15:26Z

Currently batch subscriptions spawn multiple background queues, and apart from the running subscription request, all others sit in the wait state, consuming unnecessary resources and affecting requests from other AWSAppSyncClient instances.
Also for every subscription request, AppSyncMQTTClient disconnects all previously connected sockets and connects to the new endpoints, consuming unnecessary resources in case of a batch subscription request.

Description of changes:

Replaced usage of shared SubscriptionsOrderHelper to coordinate sequencial subscription requests with a single queue owned by AWSAppSyncClient
Added a small cancellable delay in MQTTClient to avoid sequential disconnections and reconnections of the socket while subscribing to multiple topics.

rohandubal · 2018-06-06T17:17:26Z

Thanks for the PR @MarioBajr ! I am looking into this and will reply back.

rohandubal

Thanks again for the PR! The changes look good to me. I will do some testing on my side to validate some real-world scenarios and will approve once I am done validating those.

MarioBajr · 2018-06-06T19:34:57Z

Awesome, thank you too.

fans3210 · 2018-06-28T07:17:56Z

AWSAppSyncClient/AWSAppSyncSubscriptionWatcher.swift

+        self.subscriptionsQueue.async { [weak self] in
+            let semaphore = DispatchSemaphore(value: 0)
+
+            self?.performSubscriptionRequest(completionHandler: { (success, error) in


I applied this pr in my personal repo and it improved a lot. only small question about the weak self. Tested and found that I have to manually keep a strong reference of the watcher otherwise the performSubscriptionRequest method won't be called because self is nil and the semaphore will wait forever which will affect all other subscriptions. That is what I found, not sure whether I'm using it correctly,

You're absolutely right, it should bail out on self = nil before acquiring the semaphore. Will submit a fix. The behaviour of not performing the subscription if the watcher gets dealloc is correct though. You should retain the watcher if you have interest in the subscription, releasing it to unsubscribe as you can see in the deinit.

rohandubal · 2018-06-28T22:33:21Z

Hello @MarioBajr

Could you please rebase the changes before I could merge them?

Thanks,
Rohan

… provided by AppSyncClient. Added a small cancellable delay in MQTTClient to avoid sequential disconnections and reconnections socket requests while subscribing to multiple topics.

…acquiring the semaphore while starting the subscription.

… init and removing the requirement of retaining subscriptionQueue - Guaranteeing management of all AppSyncMQTTClient ivars in the same internal background queue.

MarioBajr · 2018-06-29T12:19:15Z

Done 👍

rohandubal · 2018-07-02T01:35:23Z

Great! Will merge this soon 😄

…TTClient - Fixed AppSyncMQTTClient holding MQTTSubscritionWatcher strongly - Added unit tests to AppSyncMQTTClient - Configured travis to run unit tests

mutablealligator · 2018-09-01T07:03:52Z

@MarioBajr Sorry for the delayed response. Can you resolve the conflicts and update the PR?

MarioBajr · 2018-09-07T12:32:38Z

Hi @kvasukib, sorry for the delay, was on holidays :)
While working on this merge, noticed that a new bug will be introduced in this PR because of a new behaviour added in AWSIoTMQTTClient. This PR adds a delay on all MQTTClient connections in AppSyncMQTTClient, then it's possible to have a watcher being deallocated before any attempt to connect. The problem is that AWSAppSyncSubscriptionWatcher tries to unsubscribe on deinit, triggering the new exception Cannot call unsubscribe before connecting to the server inside unsubscribeTopic:ackCallback: because _userDidIssueConnectis FALSE. Will work to solve this issue soon.

- Fixed AppSyncMQTTClient merge error - Fixed AppSyncMQTTClient calling unsubscribe on AWSIoTMQTTClient in unstable states - Disabled AWSAppSyncTests because it requires some work to be able to run within a CI

MarioBajr · 2018-09-07T20:02:38Z

Done, had to skip AWSAppSyncTests.swift from running in travis because it requires some configuration. About these tests, it behaves as integration tests, having mixed feelings if it should be in this SDK. I would replace them by unit tests with responses coming from a mocked networkTransport.

rohandubal · 2018-09-10T21:28:10Z

@MarioBajr I was trying to integrate these changes, but I am running into issues while testing on my side. Whenever I have more than 1 subscription, it was not working. Have you seen any such behavior on your side? I want to get the improvements in too so want to determine the best path ahead.

To be more specific:

In the functional tests, the subscription tests do not pass(but they can be attributed to the way they are written and need to be updated to be correctly threaded.)

But when I use a sample app where I have 3 subscriptions, and I cancel 1, all 3 seem to get cancelled.

rohandubal · 2018-09-10T22:40:59Z

AWSAppSyncClient/AppSyncMQTTClient.swift

+        func remove(subscription: MQTTSubscritionWatcher) {
+            synchronized() {
+                dictionary.forEach({ (element) in
+                    element.value.allObjects.filter({ $0.getIdentifier() != subscription.getIdentifier() }).forEach({ (watcher) in


I think this should be == instead of !=

element.value.allObjects.filter({ $0.getIdentifier() == subscription.getIdentifier() }).forEach({ (watcher) in

rohandubal

I identified a bug which needs to be fixed, it is pretty easy so I might do it if you don't get a chance. Needs additional validation though. I made the above mentioned fix and everything looks good after those changes. I will keep doing some additional validations to make sure there is no breaking behavior.

MarioBajr · 2018-09-10T23:32:55Z

.travis.yml

@@ -13,17 +13,17 @@ before_deploy:
 - carthage build --no-skip-current
 - carthage archive $FRAMEWORK_NAME
 script:
- xcodebuild -workspace AWSAppSyncClient.xcworkspace -scheme AWSAppSync build test -destination 'platform=iOS Simulator,name=iPhone 6,OS=latest'
+- xcodebuild -workspace AWSAppSyncClient.xcworkspace -scheme AWSAppSync build


Any reason to remove tests from running in travis to validade PRs?

It is definitely a good idea to run the tests on travis. I am working to add some encrypted variables(API URL info, auth information) which would help run the functional tests as well. I will enable along with them 😃

MarioBajr · 2018-09-10T23:33:11Z

Nice catch, will add some tests covering this before submitting the fix.

rohandubal · 2018-09-10T23:35:22Z

Awesome! I also identified an issue with the existing integration(functional) tests:

https://github.com/awslabs/aws-mobile-appsync-sdk-ios/blob/master/AWSAppSyncTests/AWSAppSyncTests.swift#L231

Here since we are doing let _ = subscribe and not capturing the result, the object is getting de-allocated (rightfully so). The fix in the tests is to hold a strong reference to the returned object. I think we are nearing the merge 😄

- Added tests covering the bug above - Fixed integration test that were canceling AppSync subscription before fulfilling tests expectations

rohandubal reviewed Jun 6, 2018

View reviewed changes

rohandubal mentioned this pull request Jun 8, 2018

AWSAppSyncSubscriptionWatcher cannot be deallocated until the connection state is able to be observed #31

Closed

fans3210 mentioned this pull request Jun 22, 2018

AWSAppSyncSubscriptionWatcher cancel method block main thread when there are about 20 watchers(subscriptions) #39

Closed

fans3210 reviewed Jun 28, 2018

View reviewed changes

fans3210 mentioned this pull request Jun 28, 2018

when there are more than 1 kind of subscribers, all won't work, the connection state will all be non-observable #41

Closed

Mario Araujo added 4 commits June 29, 2018 11:11

Replaced spawn of multiple queues in subscriptions for a single queue…

ca8dafc

… provided by AppSyncClient. Added a small cancellable delay in MQTTClient to avoid sequential disconnections and reconnections socket requests while subscribing to multiple topics.

Fixed swift 3 compatibility

6a3d470

Bail out earlier if the subscription watcher gets deallocated before …

b7a2246

…acquiring the semaphore while starting the subscription.

- Simplifying startSubscription logic by moving dispatch async to the…

de2aa6e

… init and removing the requirement of retaining subscriptionQueue - Guaranteeing management of all AppSyncMQTTClient ivars in the same internal background queue.

fans3210 mentioned this pull request Jul 6, 2018

MQTT Connection Status Looks Strange #26

Closed

This was referenced Jul 6, 2018

Enhancements: It would be good to expose the detailed connection states to the user #42

Closed

Conflicting memory management behaviour between AppSyncMQTTClient and MQTTSubscritionWatcher #47

Closed

- Added thread safety on all access to topic subscribers in AppSyncMQ…

651ecc2

…TTClient - Fixed AppSyncMQTTClient holding MQTTSubscritionWatcher strongly - Added unit tests to AppSyncMQTTClient - Configured travis to run unit tests

mutablealligator approved these changes Aug 28, 2018

View reviewed changes

Merge branch 'master' into improvement/fasterBatchSubscriptions

2e8fe9e

- Fixed Podfile duplicated AWSAppSyncTests target

b93f555

- Fixed AppSyncMQTTClient merge error - Fixed AppSyncMQTTClient calling unsubscribe on AWSIoTMQTTClient in unstable states - Disabled AWSAppSyncTests because it requires some work to be able to run within a CI

Merge branch 'master' into improvement/fasterBatchSubscriptions

1d57f39

rohandubal reviewed Sep 10, 2018

View reviewed changes

rohandubal suggested changes Sep 10, 2018

View reviewed changes

MarioBajr commented Sep 10, 2018

View reviewed changes

- Fixed stopSubscription removing the wrong watcher

47723d7

- Added tests covering the bug above - Fixed integration test that were canceling AppSync subscription before fulfilling tests expectations

rohandubal approved these changes Sep 11, 2018

View reviewed changes

rohandubal merged commit c69c054 into awslabs:master Sep 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced spawn of multiple queues in subscriptions for a single queue… #28

Replaced spawn of multiple queues in subscriptions for a single queue… #28

MarioBajr commented May 30, 2018

rohandubal commented Jun 6, 2018

rohandubal left a comment

MarioBajr commented Jun 6, 2018

fans3210 Jun 28, 2018 •

edited

Loading

MarioBajr Jun 28, 2018

rohandubal commented Jun 28, 2018

MarioBajr commented Jun 29, 2018

rohandubal commented Jul 2, 2018

mutablealligator commented Sep 1, 2018

MarioBajr commented Sep 7, 2018 •

edited

Loading

MarioBajr commented Sep 7, 2018

rohandubal commented Sep 10, 2018 •

edited

Loading

rohandubal Sep 10, 2018

rohandubal left a comment •

edited

Loading

MarioBajr Sep 10, 2018

rohandubal Sep 10, 2018

MarioBajr commented Sep 10, 2018

rohandubal commented Sep 10, 2018

Replaced spawn of multiple queues in subscriptions for a single queue… #28

Replaced spawn of multiple queues in subscriptions for a single queue… #28

Conversation

MarioBajr commented May 30, 2018

rohandubal commented Jun 6, 2018

rohandubal left a comment

Choose a reason for hiding this comment

MarioBajr commented Jun 6, 2018

fans3210 Jun 28, 2018 • edited Loading

Choose a reason for hiding this comment

MarioBajr Jun 28, 2018

Choose a reason for hiding this comment

rohandubal commented Jun 28, 2018

MarioBajr commented Jun 29, 2018

rohandubal commented Jul 2, 2018

mutablealligator commented Sep 1, 2018

MarioBajr commented Sep 7, 2018 • edited Loading

MarioBajr commented Sep 7, 2018

rohandubal commented Sep 10, 2018 • edited Loading

rohandubal Sep 10, 2018

Choose a reason for hiding this comment

rohandubal left a comment • edited Loading

Choose a reason for hiding this comment

MarioBajr Sep 10, 2018

Choose a reason for hiding this comment

rohandubal Sep 10, 2018

Choose a reason for hiding this comment

MarioBajr commented Sep 10, 2018

rohandubal commented Sep 10, 2018

fans3210 Jun 28, 2018 •

edited

Loading

MarioBajr commented Sep 7, 2018 •

edited

Loading

rohandubal commented Sep 10, 2018 •

edited

Loading

rohandubal left a comment •

edited

Loading