rrousselGit / riverpod

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

FutureProvider disposed unexpectedly in 1.0.0-dev3 #601

Closed limenote135 closed 2 years ago

limenote135 commented 3 years ago

Describe the bug

In the following code, future returns error with Bad state: The provider AutoDisposeFutureProvider<String>#25550(example) was disposed before a value was emitted..

v0.14.0+3 works fine.

To Reproduce

final controllerProvider =
    StateNotifierProvider<Controller, List<Future<String>>>(
        (ref) => Controller(ref.read));

final futureProvider =
    FutureProvider.autoDispose.family<String, String>((ref, str) async {
  final ret = await Future.delayed(Duration(seconds: 1), () => str);
  ref.maintainState = true;
  return ret;
});

class Controller extends StateNotifier<List<Future<String>>> {
  Controller(this._read) : super([]);

  Reader _read;

  void add(String str) {
    final f = _read(futureProvider(str).future);
    state = [f, ...state];
  }
}

class MyHomePage extends ConsumerWidget {
  const MyHomePage({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final items = ref.watch(controllerProvider);

    return Scaffold(
      floatingActionButton: FloatingActionButton(
        onPressed: () {
          ref.read(controllerProvider.notifier).add("example");
        },
        child: const Icon(Icons.add),
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          return FutureBuilder(
            future: items[index],
            builder: (context, snapshot) {
              if (snapshot.hasError) {
                return ListTile(
                  title: Text("${snapshot.error}"),
                );
              }
              if (!snapshot.hasData) {
                return const ListTile(
                  title: Center(child: CircularProgressIndicator()),
                );
              }
              return ListTile(
                title: Text("${snapshot.data}"),
              );
            },
          );
        },
      ),
    );
  }
}

Expected behavior

Show 'example' in ListTile.

Fraa-124 commented 3 years ago

I'm also experiencing this, with v1.0.0-dev6

PapyElGringo commented 3 years ago

Same with hooks_riverpod: ^1.0.0-dev.6

final messageListBoxFamily =
    FutureProvider.family.autoDispose((ref, tribuId) async {
  final secureStorage = ref.read(secureStorageProvider);
  final hiveKey = await secureStorage.read(key: 'hiveKey') ?? ''; // The error is triggered here
  final encryptionKey = base64Url.decode(hiveKey);
  Hive.registerAdapter(MessageAdapter());
  final box = await Hive.openBox<Message>('${tribuId}_messageList',
      encryptionCipher: HiveAesCipher(encryptionKey));
  return box;
});
ablbol commented 3 years ago

I am running into the same problem in hooks_riverpod: ^1.0.0-dev.7

I get an exception like this: Bad state: The provider ... was disposed before a value was emitted.

Any plans on this?

TimWhiting commented 3 years ago

@rrousselGit Technically a future provider could be disposed before being used. (Opening a page where it is used and then pressing back before the future completes). So is this really 'a bad state'? Not sure if that is the issue reported here or if it is something else here. @PapyElGringo can you try using ref.watch instead of ref.read on the secureStorageProvider in your sample?

rrousselGit commented 3 years ago

This is a known bug that I will fix in due time

This error is necessary to stop provider.future. But it shouldn't be considered as uncaught

sarakisar97 commented 2 years ago

I have the same issue, any solution? @rrousselGit

TimWhiting commented 2 years ago

I also ran into this in one of my projects which was a command line app where I was using a container and not wanting to listen to the future just read the future. I solved it by not making my provider autodispose for now, but depending on your use case that might not be a long-term or good solution.

marinkobabic commented 2 years ago

This issue now exists in the released version. ref.watch instead of ref.read worked for me.

7mada123 commented 2 years ago

Facing the same issue on 1.0.0 stable release @marinkobabic if you use ref.watch the provider won't get dispose after the future completed

7mada123 commented 2 years ago

I found a work around for now until the fix is out

this would work if you only use ref.read (not ref.watch or .when method) to get a value

since it is now possible to "await" all providers that emit an AsyncValue you can use "Provider" instead of "FutureProvider"

instead of

final provider = FutureProvider<bool>((ref) async {
  try {
    await futureCall();
    return true;
  } catch (e) {
    return false;
  }
});

use

final myAsyncProvider = Provider.autoDispose<Future<bool>>((ref) async {
  try {
    await futureCall();
    return true;
  } catch (e) {
    return false;
  }
});
tomoyuki28jp commented 2 years ago

Faced the this issue. I solved it by not making my provider autoDispose for now.

golane-august commented 2 years ago

@rrousselGit I think the reason is, that the single subscriptions aren't considered when closing the provider, although they are still waiting for the response: By adding _subscriptions.isNotEmpty || to provider_base I could circumvent the error for the autodisposables. This should already be covered by _subscribers.isNotEmpty || but they seem to be empty.

But then I get an error error: Bad state: Stream has already been listened to, which has no stacktrace. I also see there are a lots of TODOs in the code. Unfortunately our project relies on autoDisposables and the tests fail, because the error occurs in all places :) Would be nice, if you can give more background, what is needed to be changed in order to avoid the error.

rrousselGit commented 2 years ago

By adding _subscriptions.isNotEmpty || to provider_base

This change is incorrect. _subscriptions is the list of what this provider depends on. Whereas hasListeners is about whether the provider is listened or not

The only reason this could potentially "solve" the problem in your case is because it broke autoDispose by preventing the provider from being disposed (since it now thinks that it is being listened).


I'll look into it later, but I doubt that this error is still a thing on the latest versions. It should've been fixed when I added a future.ignore() in those cases

esenmx commented 2 years ago

This bug prevents usage of FutureProvider as cache repository like this:

final future = FutureProvider.autoDispose.family<Response, Request>((ref, req) {
  ref.maintainState = true;
  Future.delayed(cacheDuration, () => ref.maintainState = false);
  return service.call(req);
});

Disabling autoDispose is not a good solution obviously, so I created a StateNotifier that works on top of AsyncValue and mimics the FutureProvider with RefreshIndicator compatibility(which is not valid anymore; see riverpod 2.0.0 changes). It is much more verbose but it works. So any fix greatly appreciated @rrousselGit 😄 , thanks in advance!

golane-august commented 2 years ago

Ok thanks for clarification and the really quick reply. Nether the less the problem still occurs in current master (v2.0.0-dev.0). See full example.

rrousselGit commented 2 years ago

@golane-august Your case is expected. The provider was indeed aborted before the future emitted any value.

rrousselGit commented 2 years ago

I should've realized it sooner, but in the reproduction example given, the exception is indeed expected.

Your issue is that you're not listening to the provider when it's marked as autoDispose. You probably want to use either ref.watch or ref.listen instead.

To begin with, the code given in the example is not the proper way to make such architecture. Here's the fixed code:

import 'package:flutter/material.dart';
import 'package:hooks_riverpod/hooks_riverpod.dart';

void main() {
  runApp(ProviderScope(child: MaterialApp(home: MyHomePage())));
}

final controllerProvider = StateNotifierProvider<Controller, List<String>>(
  (ref) => Controller(),
);

final futureProvider =
    FutureProvider.autoDispose.family<String, String>((ref, str) async {
  final ret = await Future.delayed(Duration(seconds: 1), () => str);
  ref.maintainState = true;
  return ret;
});

class Controller extends StateNotifier<List<String>> {
  Controller() : super([]);

  void add(String str) {
    state = [str, ...state];
  }
}

class MyHomePage extends ConsumerWidget {
  const MyHomePage({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final items = ref.watch(controllerProvider);

    return Scaffold(
      floatingActionButton: FloatingActionButton(
        onPressed: () {
          ref.read(controllerProvider.notifier).add("example");
        },
        child: const Icon(Icons.add),
      ),
      body: ListView.builder(
        itemCount: items.length,
        itemBuilder: (context, index) {
          final str = items[index];

          return FutureBuilder(
            future: ref.watch(futureProvider(str).future),
            builder: (context, snapshot) {
              if (snapshot.hasError) {
                return ListTile(
                  title: Text("${snapshot.error}"),
                );
              }
              if (!snapshot.hasData) {
                return const ListTile(
                  title: Center(child: CircularProgressIndicator()),
                );
              }
              return ListTile(
                title: Text("${snapshot.data}"),
              );
            },
          );
        },
      ),
    );
  }
}
golane-august commented 2 years ago

@rrousselGit So ref.read isn't expected to be used at all with autodisposable FutureProvider futures ? Maybe this should throw an assertion error or something with a reason phrase, as it seems like it is possible, but it leads to unexpected behavior (?). Or do you know a use case where this constellation is needed, but will not throw this disposing error? Thx ;D

rrousselGit commented 2 years ago

I didn't say "never use read". But there is a reason the doc is saying "prefer watch" and "avoid read" 😜

golane-august commented 2 years ago

I didn't say "never use read"

Sure 😅 , but for this case, I would say it. It should be disallowed by the library to use it on autodisposable futures, if it's not supposed to. For me its important to make it as predictable as possible, especially with errors.

rrousselGit commented 2 years ago

Using read is generally safe. Problems start when you're using it when you shouldn't

If we were to use your logic, read wouldn't exist at all. We could remove it, but this would make some scenarios really inconvenient to implement.

rrousselGit commented 2 years ago

In any case this error is expected, so I'll close this issue for now. If you think this is wrong, please share another reproducible example for this issue. One where the error isn't caused by a a misuse of Riverpod

neilswinton commented 2 years ago

@rrousselGit here are two tests where the first passes and the second fails because the provider was unexpectedly disposed. I'm new at Riverpod, Dart, and Flutter, so apologies if I've missed something obvious. In general, I've been struggling with how to unit test StreamProviders in Dart vs widget tests.

import 'package:flutter_test/flutter_test.dart';
import 'package:riverpod/riverpod.dart';

Stream<int> countGenerator(int count) {
  return Stream<int>.periodic(const Duration(microseconds: 10), (x) => x)
      .take(count);
}

final counterStreamProvider = StreamProvider<int>((ref) async* {
  await for (final item in countGenerator(10)) {
    yield item;
  }
});

final counterDisposingStreamProvider =
    StreamProvider.autoDispose<int>((ref) async* {
  await for (final item in countGenerator(10)) {
    yield item;
  }
});

void main() {
  var container = ProviderContainer();

  setUp(() {
    container = ProviderContainer();
  });

  tearDown(() => container.dispose());

  void intListener(AsyncValue<int>? from, AsyncValue<int> to) {
    expect(to, isNotNull);
  }

  test('StreamProvider test', () async {
    final streamSubscription =
        container.listen(counterStreamProvider, intListener);

    final result = await container.read(counterStreamProvider.future);
    expect(result, isNotNull);
    streamSubscription.close();
  });

  test('Disposing StreamProvider test', () async {
    final streamSubscription =
        container.listen(counterDisposingStreamProvider, intListener);

    final result = await container.read(counterDisposingStreamProvider.future);
    expect(result, isNotNull);
    streamSubscription.close();
  });
}
rrousselGit commented 2 years ago

Same story here, you probably don't want to use read and instead use watch/listen

  test('Disposing StreamProvider test', () async {
    final sub = container.listen(counterDisposingStreamProvider.future, (prev, value) {});
    final result = await sub.read();
    expect(result, isNotNull);
  });

Doing this will keep the provider until the subscription is closed.

neilswinton commented 2 years ago

@rrousselGit The await is on a non-future and returns AsyncLoading. Any way to wait for a data value?

I hadn't thought to read using the subscription and not the ProviderContainer. If I've got this right, the listen creates an instance of the provider which I access using the subscription; whereas, when I do a read I get a new, ephemeral instance which is disposed of immediately. If it's useful, I'm happy to turn this into something for the docs once I get it right.

rrousselGit commented 2 years ago

The await is on a non-future and returns AsyncLoading. Any way to wait for a data value?

Shouldn't be. Maybe you forgot to pass the generic type of to specify ".future"

rrousselGit commented 2 years ago

If I've got this right, the listen creates an instance of the provider

It doesn't create a new instance. It only subscribes to the cache

The problem is with "read", which does not subscribes to the cache. So since it's an autoDispose provider and it's not listened, the cache gets destroyed before the future resolved.

neilswinton commented 2 years ago

Thanks so much for the explanation. I did forget to specify future. I switched the test to use streams. I'm still puzzled by why the non-autodispose test sees the right stream while the autodispose provider test fails with a closed stream, but I'm also feeling like I've used enough of your time.


import 'package:flutter_test/flutter_test.dart';
import 'package:riverpod/riverpod.dart';

const listLength = 10;

Stream<int> countGenerator(int count) {
  return Stream<int>.periodic(
    const Duration(microseconds: listLength),
    (x) => x,
  ).take(count);
}

final counterStreamProvider = StreamProvider<int>((ref) async* {
  await for (final item in countGenerator(listLength)) {
    yield item;
  }
});

final counterDisposingStreamProvider =
    StreamProvider.autoDispose<int>((ref) async* {
  await for (final item in countGenerator(listLength)) {
    yield item;
  }
});

void main() {
  var container = ProviderContainer();

  setUp(() {
    container = ProviderContainer();
  });

  tearDown(() => container.dispose());

  void listener(Object? from, Object to) {
    expect(to, isNotNull);
  }

  void emitTest(ProviderSubscription<Stream<int>> subscription) {
    expect(
      subscription.read(),
      emitsInOrder(List<int>.generate(listLength, (index) => index)),
    );
  }

  test('StreamProvider test', () async {
    final streamSubscription = container.listen(
      counterStreamProvider.stream,
      listener,
    );

    // Test passes
    emitTest(streamSubscription);

    streamSubscription.close();
  });

  test('Disposing StreamProvider test', () async {
    final streamSubscription = container.listen(
      counterDisposingStreamProvider.stream,
      listener,
    );

    // Test fails with:
    // Actual: <Instance of '_AsBroadcastStream<int>'>
    // Which: emitted x Stream closed.
    //        which didn't emit an event that <0>
    emitTest(streamSubscription);
    streamSubscription.close();
  });
}
rrousselGit commented 2 years ago

You've refactored the code to use expect(..., emits)

The problem is, emits expectations are performed asynchronously while expect returns synchronously. So your subscription is closed before the emits matcher actually executes.

As such writing:

final sub = container.listen(...);
expect(sub.read(), emits(value));
sub.close();

is strictly equivalent to writing:

final sub = container.listen(...);
sub.close();
expect(sub.read(), emits(value));

which should look obviously wrong.

You probably want to use await expectLater:

final sub = container.listen(...);
await expectLater(sub.read(), emits(value));
sub.close();
neilswinton commented 2 years ago

I put the working example into this gist: riverpod_stream_provider_test.dart. Thank you for all the good work you do. My testing world is much better now.

jackv24 commented 2 years ago

I've been keeping an eye on this issue for a while in hopes of a solution to an issue I've been having, but I'm not sure how to avoid using ref.read in this situation:

@override
  void initState() {
    super.initState();

    ...

    context
        .read(userComicFamily(widget.comicId).last)
        .then((userComicDoc) async {
      final currentPageId = userComicDoc?.data()?.currentPageId;

      // Get ref to current page once for centering pages on start
      final currentPageRef = currentPageId != null
          ? context.read(sharedComicPageRefFamily(SharedComicPageInfo(
              comicId: widget.comicId, pageId: currentPageId)))
          : null;

      if (currentPageRef != null) {
        // Centre on current page
        _centerPagesOnRef(currentPageRef);
      } else {
        final firstPage =
            await context.read(endPageFamily(SharedComicPagesQueryInfo(
          comicId: widget.comicId,
          descending: false,
        )).future);

        // Start at first page if no current page
        if (firstPage != null) {
          _centerPagesOnDoc(firstPage);
        } else {
          _getPages(_ScrollDirection.none);
        }
      }
    });
  }

I just need to read a few autoDispose future providers once on page open. This works fine in 0.14.0+3

rrousselGit commented 2 years ago

I've been keeping an eye on this issue for a while in hopes of a solution to an issue I've been having, but I'm not sure how to avoid using ref.read in this situation:

Remove the stateful widget. Move the initState logic in a separate FutureProvider which uses ref.watch

And watch that FutureProvider

talksik commented 1 year ago

what if we want to synchronously get a futureprovider value in a asyncnotifier custom method. Currently it requires that the futureprovider actually is used in a widget, but that seems like an odd architecture.

I wonder if we can go more towards react query architecture where dependencies resolve and fetches will happen if there is a read. That should be an abstraction as that's the point of a state/data cache library.

rrousselGit commented 1 year ago

@talksik I'm not sure what this is about. Providers also resolve/fetch on read.