apackin commented 2 years ago

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.

codecov[bot] commented 2 years ago

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.

vincenzopalazzo commented 2 years ago

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

apackin commented 2 years ago

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.