fix: Subscription failed event should be terminal event #74
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes:
The caller of a subscription should only receive the
.failedevent once. A.failedevent to the caller indicates that the subscription is in terminal state. In AppSync SDK, this event is propagated back to the caller in the resultHandler. In APIPlugin, this is in the completionListener. Data/Connection events are propagated in a different handler (statusHandler, progressHandler).The bug is that the subscription will send multiple
.failedevents in a few scenarios..failedevent..failedevent.Multiple
.failedevents can be caused by a combination of 1 and 2 or multiple instances of 1 or 2. The below scenario describes one scenario where there are multiple instances of 1. The 101st subscription reaches terminal state, and then the 102nd subscription causes subscription 101 to retry and send another.failedevent to the caller.MaxSubscriptionReached failure scenario
When there are 100 subscriptions subscribed to the connection, the 101st subscription will fail with MaxSubscriptionsReached and retry, and eventually the caller will receive the subscription failed to subscribe.
flowchart A[101st Subscription receives MaxSubscriptionReached] --> B{Retry Advice - Should retry?}; B -- Yes --> C[Connection.connect]; B -- No --> D[Send Failed event]; D ----> E[Caller]; C --> F[First 100 Subscriptions]; C --> G[101st Subscription]; F --> H[Already subscribed, No-op]; G --> I[Not subscribed, subscribe]; I --> AWhen both 101st and 102nd subscriptions fail with MaxSubscriptionsReached, they both wait for retry to trigger. When retry is triggered, it will call Connection.connect. The connection will inform every subscription to handle the connected status. The first 100 subscriptions will ignore the connection event since they are already subscribed.
When both subscriptions for waiting to retry, one of them will fire before the other. Say subscription 101 fires slightly before subscription 102, while 102 is still waiting to fire. 101's retry attempt causes both 101 and 102 to retry. Because 102 is attempting earlier than it should, it's retry attempts are exhausted first. One way to see this is to wait for 101 to retry a few times until it reaches a long wait time, then start 102 on its retry path.
Sub 102 causes Sub 101 to retry despite it's retry attempts have been exhausted. The problem is that sub 101 attempted to retry based on another subscriptions retry logic, and that it sent multiple failure events indicating that it failed.
This PR removes the subscription from the connection so that it will no longer handle connection messages after it has sent out a failed event to the caller. This doesn't solve the problem with cross-subscription fired retry attempts, however, it does have the desired behavior for callers to be able to react properly to the
.failedevent.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.