rrousselGit / riverpod

A reactive caching and data-binding framework. Riverpod makes working with asynchronous code a breeze.
https://riverpod.dev
MIT License
5.94k stars 918 forks source link

ProviderElementBase runOnDispose #3609

Closed SdxCoder closed 1 day ago

SdxCoder commented 2 weeks ago

Describe the bug We are getting an exception reported on firebase crashlytics. Concurrent modification during iteration: Instance(length:2) of '_GrowableList'. Stacktrace refers to the this file /riverpod/lib/src/framework/element.dart line 922, we have following code there

void runOnDispose() {
    if (!_mounted) return;
    _mounted = false;

    final subscriptions = _subscriptions;
    if (subscriptions != null) {
      while (subscriptions.isNotEmpty) {
        late int debugPreviousLength;
        if (kDebugMode) {
          debugPreviousLength = subscriptions.length;
        }

        final sub = subscriptions.first;
        sub.close();

        if (kDebugMode) {
          assert(
            subscriptions.length < debugPreviousLength,
            'ProviderSubscription.close did not remove the subscription',
          );
        }
      }
    }

    _onDisposeListeners?.forEach(runGuarded); <------------------ Line 922 (Problem is here)

    for (final observer in _container.observers) {
      runBinaryGuarded(
        observer.didDisposeProvider,
        _origin,
        _container,
      );
    }

    _onDisposeListeners = null;
    _onCancelListeners = null;
    _onResumeListeners = null;
    _onAddListeners = null;
    _onRemoveListeners = null;
    _onChangeSelfListeners = null;
    _onErrorSelfListeners = null;
    _didCancelOnce = false;
  }
Crash log ``` # Crashlytics - Stack trace # Date: Sat Jun 08 2024 08:11:38 GMT-0600 (Mountain Daylight Time) Non-fatal Exception: io.flutter.plugins.firebase.crashlytics.FlutterError: Concurrent modification during iteration: Instance(length:2) of '_GrowableList'. at List.forEach(dart:core) at ProviderElementBase.runOnDispose(element.dart:922) at AutoDisposeProviderElementMixin.runOnDispose(auto_dispose.dart:47) at ProviderElementBase.invalidateSelf(element.dart:279) at ProviderElementBase._markDependencyChanged(element.dart:585) at ProviderElementBase._notifyListeners(element.dart:551) at ProviderElementBase.setState(element.dart:138) at AsyncTransition.asyncTransition(common.dart:28) at FutureHandlerProviderElementMixin.onData(base.dart:277) at FutureHandlerProviderElementMixin._handleAsync.(base.dart:383) at FutureHandlerProviderElementMixin.handleFuture..(base.dart:350) ```

To Reproduce This bug has been reported on firebase crashlytics and there are no clear steps to reproduce this issue.

Expected behavior No exception thrown.

rrousselGit commented 2 weeks ago

You're likely doing something you shouldn't inside the onDispose of one of your providers.

For example, you may end-up doing:

ref.onDispose(() {
  ref.onDispose(() {});
});

That's not supported. In debug mode you'd get a custom error message. Those error messages are trimmed in release mode, should could lead to this error instead.

Without further information, I'll have to assume that this was your mistake.

SdxCoder commented 3 days ago

@rrousselGit Thanks for your response. I have checked our code base, we are not using anything like that, however we are doing something like this.

1- 
ref.onDispose(() {
      ref
        ..invalidate(getTrendingProvider)
        ..invalidate(getTrendingBrandFacetsProvider)
        ..invalidate(getTrendingCategoryFacetsProvider);
    });

Can this be problemetic?

rrousselGit commented 2 days ago

Can this be problemetic?

That's not allowed. Pretty sure it throws in the 3.0 dev branch.

onDispose needs to be a "pure" function. Invalidating other providers there definitely isn't a supported thing.

SdxCoder commented 1 day ago

@rrousselGit Thanks for pointing out. Can you help with the right approach here.

These providers are supposed to stay active for complete app session user is using the app. These providers should be disposed in following scenarios.

1- On logout and logging in again (For this its simple) 2- On app close.

Scenario 2: We can't dispose them in dispose method of StatefulWidget Screen. Then how to dispose them? Do i need to invalidate them manually or on app close they will dispose of automatically and i only handle scenario 1?

Providers implementation look like this

final getTrendingProvider = FutureProvider.autoDispose
    .family<RecommendedProducts, int>((ref, args) async {
  final recommendationRepository = getIt<RecommendationsRepositoryInterface>();
  final result =
      await recommendationRepository.getTrending(IntValue(value: args));
  KeepAliveLink? keepAlive;
  ref.onDispose(() {
    keepAlive?.close();
  });
  return result.when(
    success: (data) {
      keepAlive = ref.keepAlive();
      return data;
    },
    failure: (failure) => RecommendedProducts.empty(),
  );
});
rrousselGit commented 1 day ago

Use ref.watch in those providers instead.

final getTrendingProvider = ...((ref) {
  ref.watch(authTokenProvider); // Clear state on login/logout
});
SdxCoder commented 1 day ago

Alright thanks @rrousselGit.

I used this approach. This works too?

 ref.listen(userProvider.select((v) => v.user.id), (prev, next) {
      if (prev != next) {
        ref
          ..invalidate(getTrendingProvider)
          ..invalidate(getTrendingBrandFacetsProvider)
          ..invalidate(getTrendingCategoryFacetsProvider);
      }
    });
rrousselGit commented 1 day ago

There could be some problems if the user ID changes while widgets are building. The recommended approach is to use ref.watch like I showed above.

But it should be fine.

SdxCoder commented 1 day ago

Using watch make the provider to get called multiple times before last result. The idea is to dispose it based on userProvider only when id has changed.

SdxCoder commented 1 day ago

Also for future developments, can we reduce redundanncy in listeners listen calls? for example, if listening

ref.watch(userProvider.select((v) => v.user.id));

Should only be rebuilt once the id actually changes. Same with ref.listen, Currently i have noticed that it keeps getting even for same previous and next data.