feat(api): Subscription Reconnection #2074

Equartey · 2022-08-30T18:25:35Z

Issue #, if available:

Description of changes:
Provides GraphQL subscription reconnection and associated quality of life additions.

Introduces a reconnection strategy for GraphQL Subscription web sockets.
Monitors hardware level network changes to trigger reconnect on resume (wifi on/off, wifi to mobile, airplane mode, etc).
Utilizes an exponential back off strategy when attempting to reconnect.
Exposes configuration options for this retry strategy at plugin init time.
Introduces Hub Events to allow tracking of connection status. Statuses include:
-- Connected The network is connected and has active subscriptions.
-- Connecting - The network is disconnected and has active subscriptions. Triggers for initial connect and reconnect events.
-- PendingDisconnect - The network is connected, but has no active subscriptions.
-- Disconnected - The network has been disconnected and no active subscriptions.
-- Failed - The network could not be connected.

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

* feat(api): GraphQL API key auth mode * BREAKING CHANGE: GraphQL response errors now nullable

…r registers in Auth (aws-amplify#1851)

…mplify#1893)

…HTTP requests (aws-amplify#1913)

Co-authored-by: Elijah Quartey <Equartey@users.noreply.github.com>

packages/amplify_core/lib/src/types/api/hub/api_subscription_hub_event.dart

packages/api/amplify_api/example/lib/main.dart

packages/api/amplify_api/example/pubspec.yaml

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

packages/api/amplify_api/test/util.dart

…fy#2080)

ragingsquirrel3

Still reviewing details, but left a few comments for now, plus will DM with some offline comments that I think need to be addressed.

packages/amplify_core/lib/src/types/api/graphql/graphql_subscription_options.dart

ragingsquirrel3 · 2022-09-07T20:29:58Z

packages/amplify_core/lib/src/types/api/graphql/graphql_subscription_options.dart

+
+/// Confiuration options for GraphQL Subscriptions and their websockets.
+class GraphQLSubscriptionOptions {
+  /// Configure the ping interval for AppSync polling for subscription connections.


question: Would it make sense to state the default here? I'm guessing customers will see these comments when seeing what options they can provide and when deciding to provide a value or not it helps to know what the default is.

packages/api/amplify_api/test/util.dart

packages/amplify_core/lib/src/types/api/hub/api_subscription_hub_event.dart

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

ragingsquirrel3 · 2022-09-07T22:53:16Z

packages/api/amplify_api/lib/src/graphql/ws/web_socket_message_stream_transformer.dart

@@ -52,11 +52,12 @@ class WebSocketSubscriptionStreamTransformer<T>

  /// executes when start_ack message received
  final void Function()? onEstablished;
+  final Set<String> _establishedRequests = {};


question: Is the intention here to ensure onEstablished not called again for same sub when reconnect occurs? Seems like a fine design decision if so.

Correct, was seeing some scenarios that it was firing more than once.

Should probably be static, no? Otherwise, each set will only ever contain one object since an instance of this is created for each request.

Or if it's just for this request, then a boolean will do

ragingsquirrel3 · 2022-09-07T22:54:57Z

packages/amplify/amplify_flutter/example/macos/Flutter/GeneratedPluginRegistrant.swift

@@ -5,6 +5,8 @@
 import FlutterMacOS
 import Foundation

+import connectivity_plus_macos


when I run aft bs I get similar file modifications in packages/api/amplify_api/example/macos/Flutter/GeneratedPluginRegistrant.swift. Should that be added as well?

Yeah, I noticed some extra generated files didn't committed. Will include them next push 👍

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

dnys1

Looking good! Some minor comments mostly

packages/amplify_core/lib/src/types/api/api_types.dart

packages/amplify_core/lib/src/types/api/hub/api_subscription_hub_event.dart

packages/api/amplify_api/example/lib/main.dart

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

dnys1 · 2022-09-08T19:23:46Z

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

+        _hubEventsController?.add(SubscriptionHubEvent.failed());
+
+        if (_hasNetwork &&
+            _hasConnectionBroken &&


Not sure we want to check _hasConnectionBroken here since that will only be true if _hasNetwork is false really.

Actually, in what situation would we get here with network and not already be retrying?

I see what you're saying. _hasConnectionBroken indicates that a connection existed at one point, but was interrupted & a retry attempt hasn't been made yet. If it's false, then we've already tried to recover.

That being said, since the called out code lives in the onError handler I agree _hasConnectionBroken doesn't need to be checked. Network related interruptions are being handled else where. The goal here is to recover from errors.

Good catch.

dnys1 · 2022-09-08T19:26:15Z

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

@@ -203,12 +391,15 @@ class WebSocketConnection implements Closeable {

  /// Times out the connection (usually if a keep alive has not been received in time).
  void _timeout(Duration timeoutDuration) {
+    _timeoutTimer?.cancel();


Is the timeout timer still running when we lose network? Should it be?

Currently no, the timer gets canceled during network interruptions. I think this is how we want it, do you think otherwise?

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

…2119) * chore(api): Refactor http client & cancelable operation

ragingsquirrel3

just left some comments about possible rebase errors here

packages/amplify_core/lib/src/category/amplify_api_category.dart

packages/amplify_core/lib/src/category/amplify_categories.dart

packages/amplify_core/lib/src/plugin/amplify_api_plugin_interface.dart

packages/amplify_core/lib/src/types/api/rest/http_payload.dart

packages/amplify_core/pubspec.yaml

packages/api/amplify_api/lib/src/amplify_api_config.dart

packages/api/amplify_api/lib/src/amplify_authorization_rest_client.dart

packages/api/amplify_api/lib/src/util.dart

packages/api/amplify_api/test/util_test.dart

packages/api/amplify_api/test/ws/web_socket_auth_utils_test.dart

ragingsquirrel3

Getting closer! Liking the change in tests, nice job there. Generally would try to get rid of all the lint warnings and get tests passing so CI on PR is green and flutter analyze from amplify_api dir has no warnings.

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

ragingsquirrel3 · 2022-09-27T22:04:19Z

packages/api/amplify_api/test/ws/web_socket_connection_test.dart

+    return MockWebSocketChannel();
+  }
+
+  MockWebSocketConnection getWebSocketConnection() {


suggestion/question: Could this get moved to test util so you can use in dart_graphql_test.dart too?

I moved some of the logic, but can't completely reuse this logic as each test is implemented slightly different.

packages/api/amplify_api/test/ws/web_socket_connection_test.dart

packages/api/amplify_api/example/lib/rest_api_view.dart

packages/api/amplify_api/example/lib/graphql_api_view.dart

packages/api/amplify_api/example/amplify/README.md

ragingsquirrel3

Still gonna test again but left a few small comments.

ragingsquirrel3 · 2022-09-28T20:21:30Z

packages/amplify_core/lib/src/hub/amplify_hub_impl.dart

@@ -85,6 +85,10 @@ class AmplifyHubImpl extends AmplifyHub {
    Future.wait<void>([
      for (final stream in _availableStreams.values) stream.close(),
    ]).ignore();
+    Future.wait<void>([


Missed this from before, but what does this block do?

No worries, this was just added. @dnys1 helped debug this one. It cleans up hub event subscriptions on cancel, an issue I was seeing in testing.

cool, thanks for info. Might be worth a small comment here to explain.

packages/amplify_core/lib/src/plugin/amplify_api_plugin_interface.dart

packages/api/amplify_api/test/util.dart

packages/api/amplify_api/test/ws/web_socket_connection_test.dart

codecov-commenter · 2022-09-28T21:00:28Z

Codecov Report

Merging #2074 (dc00c44) into feat/api-next (2809cc6) will increase coverage by 0.45%.
The diff coverage is 53.10%.

@@                Coverage Diff                @@
##           feat/api-next    #2074      +/-   ##
=================================================
+ Coverage          39.23%   39.69%   +0.45%     
=================================================
  Files                120      120              
  Lines               6915     7039     +124     
=================================================
+ Hits                2713     2794      +81     
- Misses              4202     4245      +43

Flag	Coverage Δ
android-unit-tests	`∅ <ø> (∅)`
flutter-unit-tests	`27.87% <53.10%> (+0.81%)`	⬆️
ios-unit-tests	`94.92% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/api/amplify_api/lib/amplify_api.dart	`100.00% <ø> (ø)`
...kages/api/amplify_api/lib/src/api_plugin_impl.dart	`77.39% <0.00%> (-1.37%)`	⬇️
..._api/lib/src/graphql/ws/web_socket_connection.dart	`63.46% <52.89%> (+3.23%)`	⬆️
...phql/ws/web_socket_message_stream_transformer.dart	`90.90% <100.00%> (+0.90%)`	⬆️

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart

dnys1

LGTM 🚀

packages/api/amplify_api/example/lib/main.dart

dnys1 · 2022-09-29T21:10:45Z

packages/api/amplify_api/test/dart_graphql_test.dart

-  WebSocketConnection getWebSocketConnection({String? apiName}) =>
-      MockWebSocketConnection(testApiKeyConfig, getTestAuthProviderRepo());
+  WebSocketConnection getWebSocketConnection({String? apiName}) {
+    return mockWebSocketConnection;


Should it be the same for every test?

Yeah, I believe this is okay given the tests implementations.

packages/api/amplify_api/test/util.dart

ragingsquirrel3

🚢 🔥 , love it! nice work

Travis Sheppard and others added 11 commits August 23, 2022 13:45

chore!(api): migrate API category type definitions (aws-amplify#1640)

110672a

chore(api): API Native Bridge for .addPlugin() (aws-amplify#1756)

66d99bf

chore(api): API Pigeon update (aws-amplify#1813)

2ff8e51

feat(api): REST methods in dart with auth mode none (aws-amplify#1783)

f4211af

feat!(api): GraphQL API key auth mode (aws-amplify#1858)

5608b59

* feat(api): GraphQL API key auth mode * BREAKING CHANGE: GraphQL response errors now nullable

feat!(core,auth): auth providers definition and CognitoIamAuthProvide…

985b045

…r registers in Auth (aws-amplify#1851)

feat(core,api): IAM auth mode for HTTP requests (REST and GQL) (aws-a…

7b6ce8e

…mplify#1893)

feat(api): GraphQL Custom Request Headers (aws-amplify#1938)

cb6fbdb

feat(auth,api): cognito user pools auth provider & auth mode for API …

a1e8d9d

…HTTP requests (aws-amplify#1913)

fix(auth): correct auth providers imports from rebase (aws-amplify#2042)

3ac59f4

feat(api): .subscribe() for GraphQL (aws-amplify#1915)

4009458

Co-authored-by: Elijah Quartey <Equartey@users.noreply.github.com>

Equartey requested review from ragingsquirrel3 and dnys1 August 30, 2022 18:25

Equartey requested a review from a team as a code owner August 30, 2022 18:25

dnys1 reviewed Sep 1, 2022

View reviewed changes

Equartey and others added 7 commits September 1, 2022 16:51

chore(api): Api Example - generate files for multi-plaform (aws-ampli…

48c9582

…fy#2080)

feat(api): Subscription Reconnection

bef7f19

fix: unexposed retryTimeout

fcc53a0

fix: more robust uri replacement

32178ca

fix: syntax errors

8bc47dd

fix: code review feedback 1

826d470

fix: export retry package

b786902

Equartey force-pushed the feat/graphql-sub-reconnect branch from e6bb82b to b786902 Compare September 1, 2022 23:42

ragingsquirrel3 reviewed Sep 7, 2022

View reviewed changes

Equartey added 2 commits September 8, 2022 11:09

fix: Hub Event errors

2abff51

fix: typos & PR comments

80aa0d5

dnys1 reviewed Sep 8, 2022

View reviewed changes

Equartey and others added 3 commits September 8, 2022 15:28

fix: review comments

7fc1cea

fix: hub events types

ff85ce2

chore!(api): migrate API category type definitions (aws-amplify#1640)

9ddea6b

Equartey and others added 4 commits September 13, 2022 10:13

Merge branch 'feat/api-next' into feat/graphql-sub-reconnect

85c4227

chore(api): Refactor http client & cancelable operation (aws-amplify#…

e6230c5

…2119) * chore(api): Refactor http client & cancelable operation

Merge branch 'feat/api-next' into feat/graphql-sub-reconnect

12de4f0

fix: unit tests sans-reconnect & hub events

5431552

ragingsquirrel3 force-pushed the feat/api-next branch 2 times, most recently from 6a40c9f to f2b8414 Compare September 15, 2022 18:07

ragingsquirrel3 force-pushed the feat/api-next branch from 1fe8b50 to 2809cc6 Compare September 26, 2022 18:18

Merge branch 'feat/api-next' into feat/graphql-sub-reconnect

c926e66

ragingsquirrel3 reviewed Sep 27, 2022

View reviewed changes

Equartey added 4 commits September 27, 2022 15:19

Nothing gets past Travis 💪

426e0fa

Integration file clean up

51e8917

missed two files

4aab1e1

Removed http dep

b8677e9

ragingsquirrel3 reviewed Sep 27, 2022

View reviewed changes

Equartey added 2 commits September 28, 2022 14:44

Types, Lints, overrides, and Passing Tests.

3264fc7

Fixed pingClient init value

d3896e7

ragingsquirrel3 reviewed Sep 28, 2022

View reviewed changes

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart Outdated Show resolved Hide resolved

packages/api/amplify_api/lib/src/graphql/ws/web_socket_connection.dart Outdated Show resolved Hide resolved

fix: some nits

6b5bf50

dnys1 approved these changes Sep 29, 2022

View reviewed changes

Equartey added 2 commits September 30, 2022 09:32

fix: some nits

dc00c44

fix: ios network none issue on second sub

b1f162b

ragingsquirrel3 approved these changes Sep 30, 2022

View reviewed changes

Equartey merged commit 9d1319c into aws-amplify:feat/api-next Sep 30, 2022

Equartey deleted the feat/graphql-sub-reconnect branch September 30, 2022 18:41

ragingsquirrel3 pushed a commit that referenced this pull request Oct 19, 2022

feat(api): Subscription Reconnection (#2074)

56221f8

ragingsquirrel3 pushed a commit that referenced this pull request Nov 8, 2022

feat(api): Subscription Reconnection (#2074)

b05976e

HuiSF pushed a commit to HuiSF/amplify-flutter that referenced this pull request Nov 10, 2022

feat(api): Subscription Reconnection (aws-amplify#2074)

c7a25d9

ragingsquirrel3 pushed a commit that referenced this pull request Nov 10, 2022

feat(api): Subscription Reconnection (#2074)

1e97c1c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): Subscription Reconnection #2074

feat(api): Subscription Reconnection #2074

Equartey commented Aug 30, 2022

ragingsquirrel3 left a comment

ragingsquirrel3 Sep 7, 2022

ragingsquirrel3 Sep 7, 2022

Equartey Sep 8, 2022

dnys1 Sep 8, 2022

dnys1 Sep 8, 2022

ragingsquirrel3 Sep 7, 2022

Equartey Sep 8, 2022

dnys1 left a comment

dnys1 Sep 8, 2022

Equartey Sep 8, 2022

dnys1 Sep 8, 2022

Equartey Sep 8, 2022

ragingsquirrel3 left a comment

ragingsquirrel3 left a comment

ragingsquirrel3 Sep 27, 2022

Equartey Sep 29, 2022

ragingsquirrel3 left a comment

ragingsquirrel3 Sep 28, 2022

Equartey Sep 29, 2022

ragingsquirrel3 Sep 29, 2022

codecov-commenter commented Sep 28, 2022 •

edited

Loading

dnys1 left a comment

dnys1 Sep 29, 2022

Equartey Sep 30, 2022

ragingsquirrel3 left a comment

feat(api): Subscription Reconnection #2074

feat(api): Subscription Reconnection #2074

Conversation

Equartey commented Aug 30, 2022

ragingsquirrel3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnys1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragingsquirrel3 left a comment

Choose a reason for hiding this comment

ragingsquirrel3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragingsquirrel3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2022 • edited Loading

Codecov Report

dnys1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragingsquirrel3 left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 28, 2022 •

edited

Loading