graphql-flutter icon indicating copy to clipboard operation
graphql-flutter copied to clipboard

fix(graphql): deduplicate pollers

Open apackin opened this issue 3 years ago • 2 comments

Fixes / Enhancements

Fixed Issue

Calling stopPolling, followed quickly by startPolling, does not remove the first poller and results in duplicate pollers.

Reproducing issue

Create an observable query and call

 void startDupPolling(Duration duration) {
   _observableQuery.startPolling(duration);
   _observableQuery.startPolling(duration);  // Internally calls `stopPolling` if poller is running
}

Or

 void startDupPolling(Duration duration) {
   _observableQuery.startPolling(duration);
   _observableQuery.stopPolling();
   _observableQuery.startPolling(duration);
}

Starts two pollers instead of one.

Explanation

  /// Removes the [ObservableQuery] from one of the registered queries.
  /// The fetchQueriesOnInterval will then take care of not firing it anymore.
  void stopPollingQuery(String queryId) {
    registeredQueries.remove(queryId);
  }

stopPolling is expecting fetchQueriesOnInterval to remove the stopped poll. This only happens if registeredQueries[queryId] == null is true when fetchQueriesOnInterval is run. Calling startPolling again quickly, adds a new entry to registeredQueries[queryId] before fetchQueriesOnInterval, thus leading to duplicate entries in intervalQueries. Since registeredQueries[queryId] is true, all entries for the queryId in intervalQueries get fired, instead of the stopped ones being removed.

The Fix

By making intervalQueries store a set, we prevent duplicate entries from being added for the same duration. Thus the value of registeredQueries[queryId] applies to the only value for that queryId in the set.

apackin avatar Sep 29 '22 17:09 apackin

Codecov Report

Merging #1240 (0990ed7) into main (312dc6c) will increase coverage by 3.68%. The diff coverage is 100.00%.

:exclamation: Current head 0990ed7 differs from pull request most recent head 3cc0942. Consider uploading reports for the commit 3cc0942 to get more accurate results

@@            Coverage Diff             @@
##             main    #1240      +/-   ##
==========================================
+ Coverage   59.29%   62.98%   +3.68%     
==========================================
  Files          41       41              
  Lines        1683     1683              
==========================================
+ Hits          998     1060      +62     
+ Misses        685      623      -62     
Impacted Files Coverage Δ
packages/graphql/lib/src/cache/cache.dart 78.57% <ø> (ø)
packages/graphql/lib/src/scheduler/scheduler.dart 100.00% <100.00%> (+95.83%) :arrow_up:
packages/graphql/lib/src/core/query_manager.dart 77.55% <0.00%> (+1.36%) :arrow_up:
packages/graphql/lib/src/core/query_options.dart 57.48% <0.00%> (+8.38%) :arrow_up:
...ackages/graphql/lib/src/core/observable_query.dart 55.30% <0.00%> (+17.42%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Sep 29 '22 17:09 codecov[bot]

Thanks! I will put on my todo list, overall looks good but I need to clone and see in deep these changes!

vincenzopalazzo avatar Sep 29 '22 17:09 vincenzopalazzo

Would love to get this merged in so we can get back on the latest version of the package. Let me know if there's anything I can help with.

apackin avatar Oct 19 '22 15:10 apackin

Thanks for this fix!

vincenzopalazzo avatar Oct 20 '22 08:10 vincenzopalazzo