Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Critical Bug: ref.invalidate() Calls The Notifier Class Build Method Before It Disposes When There Are No Listeners #3760

Open
walidwalid23 opened this issue Oct 3, 2024 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@walidwalid23
Copy link

Describe the bug
I have a chat screen that watches a family provider that retrieves the messages using the chat Id as a family parameter, when the user receives a message notification I try to invalidate the messages provider from the notifications notifier class, if the chat screen has never been opened before which means there are no listeners the build method of the messages notifier class gets called and a redundant Api call is made to the server to retrieve the messages is made before the provider gets disposed, I have debugged and found out that this problem only occurs when we invalidate a family provider with a parameter from another notifier class and you can Reproduce this bug using the code below you will find out that the first time you click on the invalidate provider button before you open the chat screen the build method of the get messages will be called and getting messages will be printed then provider is disposed will be printed after, but if you opened the chat screen before you click on the invalidate provider then you clicked on invalidate provider the normal behavior will occur.

To Reproduce
** get_messages_notifier.dart **

import 'package:riverpod_annotation/riverpod_annotation.dart';

part 'get_messages_notifier.g.dart';

@Riverpod(keepAlive: true)
class GetMessagesNotifier extends _$GetMessagesNotifier {
  @override
  Future<List<String>> build(int chatId) async {
    ref.onDispose(() {
      print("provider is disposed");
    });

    // simulate api request
    print("getting messages");
    await Future.delayed(const Duration(seconds: 3));

    return ["message 1", "message 2", "message 3"];
  }
}

** notifications_notifier.dart **

import 'package:invalidateissueproject/get_messages_notifier/get_messages_notifier.dart';
import 'package:riverpod_annotation/riverpod_annotation.dart';

part 'notifications_notifier.g.dart';

@Riverpod()
class NotificationsNotifier extends _$NotificationsNotifier {
  @override
  Future<Object?> build() async {
    return null;
  }

  void invalidateMessages() {
    int chatId = 1;
    ref.invalidate(getMessagesNotifierProvider(chatId));
  }
}

** chat_screen.dart **

import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:invalidateissueproject/get_messages_notifier/get_messages_notifier.dart';

class ChatScreen extends ConsumerStatefulWidget {
  const ChatScreen({super.key});

  @override
  ConsumerState<ChatScreen> createState() => _ChatScreenState();
}

class _ChatScreenState extends ConsumerState<ChatScreen> {
  int chatId = 1;
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(),
      body: ref.watch(getMessagesNotifierProvider(chatId)).when(
          skipLoadingOnRefresh: false,
          data: (List<String> messages) {
            return Center(
              child: Column(
                children:
                    messages.map((String message) => Text(message)).toList(),
              ),
            );
          },
          error: (er, st) => null,
          loading: () {
            return const Center(child: CircularProgressIndicator());
          }),
    );
  }
}

** main.dart **

import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:invalidateissueproject/chat_screen.dart';
import 'package:invalidateissueproject/notifications_notifier/notifications_notifier.dart';

void main() {
  runApp(const ProviderScope(child: MyApp()));
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      home: MyHomePage(),
    );
  }
}

class MyHomePage extends ConsumerStatefulWidget {
  const MyHomePage({super.key});

  @override
  ConsumerState<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends ConsumerState<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: ElevatedButton(
            onPressed: () {
              Navigator.push(context,
                  MaterialPageRoute(builder: (context) => const ChatScreen()));
            },
            child: const Text("go to chat screen")),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: () {
          int chatId = 1;
          ref.read(notificationsNotifierProvider.notifier).invalidateMessages();
        },
        child: const Text(
          "Invalidate provider",
          style: TextStyle(fontSize: 12),
        ),
      ),
    );
  }
}

Expected behavior
I expect the family provider to be disposed without calling the build method before its disposed for the first time.

@rrousselGit
Copy link
Owner

It's inconvenient, but I'm not sure about the critical part
Anyway I'll look into it.

For now, one workaround is to wrap your "ref.invalidate(p)" into a "ref.exists(p)".
That should do the trick

@walidwalid23
Copy link
Author

@rrousselGit thanks for your response, so I should do it like this right?

 if (ref.exists(getMessagesNotifierProvider(chatId))) {
      ref.invalidate(getMessagesNotifierProvider(chatId));
    } 

@rrousselGit
Copy link
Owner

Yes

@walidwalid23
Copy link
Author

thanks, also I mentioned its critical because this could lead to a redundant API call which leads to extra network and financial cost as well as unexpected behavior for example in my case users didn't receive notification for the first message they received because when get messages is called the message notifications gets reset.

@rrousselGit
Copy link
Owner

It'll definitely need to be fixed. But considering there's an easy workaround and that it's likely a bit rare, I think it can easily wait a bit :)

@walidwalid23
Copy link
Author

walidwalid23 commented Oct 3, 2024

Okay also I would be happy to contribute and try to fix it if possible, as I invalidate many family providers from notifiers in my app and writing if condition for each one could be easily forgotten xD

@rrousselGit
Copy link
Owner

Sure, if you feel comfortable doing so!
It should be in ProviderContainer.invalidate.

Make sure to test it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants