zino-hofmann / graphql-flutter

A GraphQL client for Flutter, bringing all the features from a modern GraphQL client to one easy to use package.
https://zino-hofmann.github.io/graphql-flutter
MIT License
3.25k stars 622 forks source link

Send SubscriptionComplete message when using graphqlTransportWs protocol #1368

Closed GregVS closed 1 year ago

GregVS commented 1 year ago

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[bot] commented 1 year ago

Codecov Report

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

:exclamation: 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%) :arrow_up:

... and 1 file with indirect coverage changes

vincenzopalazzo commented 1 year ago

Looks like that this PR is breaking the CI

GregVS commented 1 year ago

Ok will look into it when I get the chance

vincenzopalazzo commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/zino-hofmann/graphql-flutter/pull/1375

BTW, Thanks to work on this