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

Send SubscriptionComplete message when using graphqlTransportWs protocol #1368

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

GregVS
Copy link

@GregVS GregVS commented Aug 8, 2023

According to the graphql-transport-ws protocol, when the client is closing a subscription, it is supposed to send a Complete message. Currently this library handles sending the stop operation for graphql-ws protocol, but is not in compliance with the graphql-transport-ws procedure. Consequently, the server does not close it's subscription with the client and resources are being consumed. Therefore, I have added code to provide this proper behavior which tells the server that the client no longer wishes to receive additional message.

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Merging #1368 (26174e3) into main (d41edf5) will increase coverage by 0.43%.
The diff coverage is 100.00%.

❗ Current head 26174e3 differs from pull request most recent head 8587340. Consider uploading reports for the commit 8587340 to get more accurate results

@@            Coverage Diff             @@
##             main    #1368      +/-   ##
==========================================
+ Coverage   64.19%   64.63%   +0.43%     
==========================================
  Files          41       41              
  Lines        1715     1719       +4     
==========================================
+ Hits         1101     1111      +10     
+ Misses        614      608       -6     
Files Changed Coverage Δ
...lib/src/links/websocket_link/websocket_client.dart 83.33% <100.00%> (+0.72%) ⬆️

... and 1 file with indirect coverage changes

@vincenzopalazzo
Copy link
Collaborator

Looks like that this PR is breaking the CI

@GregVS
Copy link
Author

GregVS commented Aug 9, 2023

Ok will look into it when I get the chance

@vincenzopalazzo
Copy link
Collaborator

@GregVS Any update on this? I am going to prepare the new beta release, so if you want this in this is a good moment to fix the CI

@catapop84
Copy link

I can confirm this.
subscription.cancel() doesn't send to server complete message.
Also if the client doesn't listens to any subscription should disconnect.
The javascript client has a lazy option which is true by default.

Controls when should the connection be established.
false: Establish a connection immediately. Use onNonLazyError to handle errors.
true: Establish a connection on first subscribe and close on last unsubscribe. Use the subscription sink's error to handle errors.

@catapop84
Copy link

About closing web socket connection, client sends complete on subscription -> wait for server complete response then closes connection. Basically on receiving complete message from server if number of subscriptions is zero -> close connection
Next time you subscribe you open a new connection.

@vincenzopalazzo
Copy link
Collaborator

I did the work for you and fixed also the commit header.

But I change also the rules of how to get the review on a PR #1375

BTW, Thanks to work on this

@vincenzopalazzo vincenzopalazzo merged commit 8c9cc86 into zino-hofmann:main Aug 30, 2023
4 checks passed
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