Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Throttle AppSync LimitExceeded errors #67

Merged
merged 3 commits into from
Feb 23, 2022
Merged

Conversation

lawmicha
Copy link
Contributor

@lawmicha lawmicha commented Feb 11, 2022

Issue #, if available:

Description of changes:
The LimitExceeded error is received when AppSync throttles the client for making too many subscription requests:

{"type":"error","payload":{"errors":{"errorType":"LimitExceededError","message":"Rate limit exceeded"}}}

This happens once the rate of subscribes has exceeded the limit on AppSync. Since 100 subscriptions can be established successfully, a loop with 200 or more subscriptions requests will reproduce this.

Consider the current flow

flowchart
A[AppSync] --> |sends LimitExceeded| B[Websocket client];
B --> |convert to ConnectionProviderError.other| C1[Subscription1 - subscribed];
B --> |convert to ConnectionProviderError.other| C2[Subscription2 - not subscribed];
C1 --> |resultHandler error| D[Caller];
C2 --> |resultHandler error| D;
Loading
  1. Subscribe 200 times, 100 will be successfully connected. 101th and onward would get MaxSubscriptionsReachedError(subscriptionId) (and go into retry logic). But, when sending 101st to 200th subscription requests all at the same time, AppSync throttles this client with error LimitExceededError.
  2. LimitExceededError is received, translate this to a generic ConnectionProviderError.other because there's no id in the response object. Send ConnectionProviderError.other to all 200 subscriptions (even the subscribed ones).
  3. All subscriptions will transition to .notSubscribed and return a failed event to the caller. Let's say 100 out of 200 subscriptions are throttled, then all 200 subscriptions will get a callback for each error, which is 20000 callbacks. Even though we removed the listener from the connection provider in the previous PR fix: Subscription failed event should be terminal event #74 , the problem is that connection makes a copy of the subscription listeners before broadcasting the message, so even though they are being removed after sending the error from a previous subscription, they continue to emit a failed message. In normal message rates, removing the connection is enough to do the job.

How should we handle LimitExceededError?

  1. The connection receives a LimitExceededError without id (w/o subscription id - we'll treat this as a connection level LimitExceededError.). Convert it over to .limitExceeded(nil) error.
  2. Pass it to a client side throttle to slow down the rate at which subscriptions will receive this error.
  3. Throttle for 150 ms, and then pass it to the subscriptions.
  4. A subscribed subscripiton will ignore this error.
  5. An in progress subscription will return the failure.
flowchart
A[AppSync] --> |sends LimitExceeded| B[Websocket client];
B --> |convert to ConnectionProviderError.limitExceeded w/ nil| C[Throttle];
C --> |send throttled limitExceeded error| C1[Subscription1 - subscribed];
C --> |send throttled limitExceeded error| C2[Subscription2 - notSubscribed];
C1 --> D[no-op];
C2 --> |resultHandler error| E[Caller];
Loading

Manual testing

When subscribing to 500 subscriptions, 100 are connected successfully, 400 failed events. Send a mutation, 100 subscription events are received.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@lawmicha lawmicha changed the title Fix/rate limit exceeded fix: Handle LimitExceededError Feb 11, 2022
@lawmicha lawmicha force-pushed the fix/rate-limit-exceeded branch from 3bfacbb to 1838a73 Compare February 11, 2022 17:50
@lawmicha lawmicha force-pushed the fix/rate-limit-exceeded branch from 1838a73 to 43a4e7b Compare February 21, 2022 16:58
@lawmicha lawmicha changed the title fix: Handle LimitExceededError fix: Throttle AppSync LimitExceeded errors Feb 21, 2022
@lawmicha lawmicha force-pushed the fix/rate-limit-exceeded branch from 43a4e7b to 5fb286a Compare February 21, 2022 23:36
@lawmicha lawmicha force-pushed the fix/rate-limit-exceeded branch from 7486951 to 417eeb8 Compare February 22, 2022 16:42
@lawmicha lawmicha marked this pull request as ready for review February 23, 2022 16:56
@lawmicha lawmicha requested a review from a team February 23, 2022 16:56
if let errors = payload["errors"],
case let .object(errorsObject) = errors,
let errorType = errorsObject["errorType"],
errorType == "LimitExceededError" {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it might be worth to extract all these error types (LimitExceededError, MaxSubscriptionsReachedError, MaxSubscriptionsReachedException) in an enum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do this in the next PR -> https://github.com/aws-amplify/aws-appsync-realtime-client-ios/pull/69/files it is also error handling related

@lawmicha lawmicha merged commit 7f77936 into main Feb 23, 2022
@lawmicha lawmicha deleted the fix/rate-limit-exceeded branch February 23, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants