Skip to content

Conversation

@greimela
Copy link
Contributor

Issue #, if available:
#5403

Description of changes:
This change allows a user of the socket to restart the subscription after it terminates without an error.
This can sometimes happen when devices enter or leave standby.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This change allows a user of the socket to restart the subscription after it terminates without an error.
This can sometimes happen when devices enter or leave standby.
@greimela greimela requested a review from manueliglesias as a code owner July 29, 2020 08:46
@codecov
Copy link

codecov bot commented Jul 29, 2020

Codecov Report

Merging #6448 into main will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6448      +/-   ##
==========================================
- Coverage   73.31%   73.29%   -0.02%     
==========================================
  Files         208      208              
  Lines       12922    12925       +3     
  Branches     2526     2526              
==========================================
  Hits         9474     9474              
- Misses       3257     3260       +3     
  Partials      191      191              
Impacted Files Coverage Δ
...pubsub/src/Providers/AWSAppSyncRealTimeProvider.ts 18.37% <0.00%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b4f6d8...bc62002. Read the comment docs.

@bensie
Copy link
Contributor

bensie commented Aug 11, 2020

We're seeing the same behavior as described in #5403 and would love to see this merged!

@ericclemmons
Copy link
Contributor

@greimela @bensie I was able to validate this (including @adamup928's example from #5403).

Since a socket reconnect doesn't happen automatically, how are/would you handling reconnecting?

Copy link
Contributor

@ericclemmons ericclemmons left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

I confirmed I can pick up on the close event via the error method:

const subscription = API.graphql({
      authMode: owner ? 'AMAZON_COGNITO_USER_POOLS' : 'AWS_IAM',
      query: onUpdateAlbum,
      variables: { owner },
    }).subscribe({
      error(error) {
        console.log('Subscription error', error);
      },
      next(payload) {
        setAlbum(payload.value.data.onUpdateAlbum);
      },
    });

@greimela
Copy link
Contributor Author

@ericclemmons Nice, thanks for the confirmation!

We are using the error callback to restart our subscriptions. So if the close events arrive there it's perfect for us 👌

@ericclemmons ericclemmons requested a review from elorzafe August 13, 2020 16:41
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Thanks @greimela 🎉 🌮 🥇

@amhinson amhinson merged commit 9d91562 into aws-amplify:main Aug 20, 2020
nubpro pushed a commit to nubpro/amplify-js that referenced this pull request Oct 2, 2020
This change allows a user of the socket to restart the subscription after it terminates without an error.
This can sometimes happen when devices enter or leave standby.

Co-authored-by: Eric Clemmons <[email protected]>
@bensie
Copy link
Contributor

bensie commented Oct 4, 2020

@greimela Would you mind sharing how you trigger the websocket reconnect from within the subscribe error function? It doesn't seem like triggering resubscribes causes the websocket connection to reconnect if it's no longer active. Thanks!

@greimela
Copy link
Contributor Author

greimela commented Oct 5, 2020

@bensie We are actually opening a completely new subscription in the case of an error.

@bensie
Copy link
Contributor

bensie commented Oct 5, 2020

@greimela Guessing I'm missing a step here and I think we might be mixing up terminology - with 1 websocket connection and many subscriptions on that connection, if the websocket connection is closed and the onclose event is fired, how do you reconnect the websocket ahead of re-initiating those subscriptions? Do you have a code snippet you could share?

@greimela
Copy link
Contributor Author

greimela commented Oct 5, 2020

@bensie We are using this lib only through the GraphQL subscriptions, so we might speak of different subscriptions here.

We just call API.graphql(...).subscribe(...) again after we get the error.

@bensie
Copy link
Contributor

bensie commented Oct 5, 2020

@greimela Interesting, thanks! I'll keep digging, when we do that the websocket connection doesn't seem to reconnect automatically and the resubscribe fails.

iartemiev pushed a commit that referenced this pull request Oct 13, 2020
This change allows a user of the socket to restart the subscription after it terminates without an error.
This can sometimes happen when devices enter or leave standby.

Co-authored-by: Eric Clemmons <[email protected]>
@github-actions
Copy link

github-actions bot commented Oct 6, 2021

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants