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

[gql_websocket_link] enhancement for websocket connection errors #412

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dehypnosis
Copy link
Contributor

@dehypnosis dehypnosis commented Aug 9, 2023

Background

Hi Team,

This PR is recreated from #388 due to lack of feedbacks and a branch issue from my side. Please check original threads because there were some feedbacks and patch history for that (@agent3bood @knaeckeKami).

Resolved issues

1. Subscription stream become broken when got WebSocketChannelException.

Currently Subscription request is losing message streams when the client got WebSocketChannelException.
And this problem is cannot be recovered automatically when even autoReconnect is on and so the websocket connection recovered. It means when client have a connection problem, all message stream subscriptions for 'Subscription Operation' should be manually recreated (by navigating routes or whatever)

Fixed this issue by not closing message subscription on WebSocketChannelException.

2. No way to get ConnectionError directly from application.

Added StreamController _connectionErrorController and public accessor connectionErrorStream.

A practical use case is that, when web socket connection failed due to server side decision or unexpected error, client can be notified about detail of error to take some action.

For example from my case, I need to refresh my client-side id token when it has expired. while I am using Firebase Authentication, It can be detected on App startup logic. But when id token became inactive while using app, my server will close the connection with error message to make the app to refresh token. But there were no way for client to detect connection issue.

Note

I made this patches to deal with above cases and any other server-side connection errors.

And both fixes are working on production for six months without an issue.

Thank you for reviewing it.

Comment on lines 276 to 281
}, onError: (Object error) {
if (autoReconnect && error is WebSocketChannelException) {
_connectionErrorController.add(ConnectionError(error));
return;
}
_messagesController.addError(error);
Copy link
Contributor Author

@dehypnosis dehypnosis Aug 9, 2023

Choose a reason for hiding this comment

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

FYI; With this logic, a message stream won't be closed with temporary connection issue.
So the first issue can be solved.

@knaeckeKami
Copy link
Collaborator

@agent3bood are you able to triage this? I'm not actively using the websocket so I cannot say too much about this

@agent3bood
Copy link
Collaborator

@dehypnosis Thanks for the second PR, I ned to take a closer look at it.
By reading our current code I can see other possible improvements to be made.

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.

3 participants