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

Bad state Exception when call player.dispose() #183

Closed
AhmedNourJamalElDin opened this issue Sep 9, 2020 · 18 comments
Closed

Bad state Exception when call player.dispose() #183

AhmedNourJamalElDin opened this issue Sep 9, 2020 · 18 comments
Assignees
Labels
1 backlog bug Something isn't working

Comments

@AhmedNourJamalElDin
Copy link

Which API doesn't behave as documented, and how does it misbehave?

disposing the payer throws: You cannot close the subject while items are being added from addStream.

Minimal reproduction project
No need. any project you have which have player in different screens and when you want to go from screen to another, you have to dispose the running player, you face this issue.

To Reproduce (i.e. user steps, not code)
Steps to reproduce the behavior:

  1. Navigate to any screen
  2. Run the player (optional)
  3. Navigate back to the old screen
  4. See error: You cannot close the subject while items are being added from addStream

Error messages

E/flutter (23080): [ERROR:flutter/lib/ui/ui_dart_state.cc(166)] Unhandled Exception: Bad state: You cannot close the subject while items are being added from addStream
E/flutter (23080): #0      Subject.close (package:rxdart/src/subjects/subject.dart:152:7)
E/flutter (23080): #1      AudioPlayer.dispose (package:just_audio/just_audio.dart:681:30)
E/flutter (23080): <asynchronous suspension>
E/flutter (23080): #2      Audio.dispose (package:hellodoctorapp/services/audio.dart:53:25)
E/flutter (23080): <asynchronous suspension>
E/flutter (23080): #3      _ChatPageState.dispose (package:hellodoctorapp/pages/chat_page.dart:73:11)
E/flutter (23080): #4      StatefulElement.unmount (package:flutter/src/widgets/framework.dart:4773:12)
E/flutter (23080): #5      _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1935:13)
E/flutter (23080): #6      _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #7      SingleChildRenderObjectElement.visitChildren (package:flutter/src/widgets/framework.dart:5861:14)
E/flutter (23080): #8      _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #9      _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #10     ComponentElement.visitChildren (package:flutter/src/widgets/framework.dart:4600:14)
E/flutter (23080): #11     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #12     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #13     SingleChildRenderObjectElement.visitChildren (package:flutter/src/widgets/framework.dart:5861:14)
E/flutter (23080): #14     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #15     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #16     SingleChildRenderObjectElement.visitChildren (package:flutter/src/widgets/framework.dart:5861:14)
E/flutter (23080): #17     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #18     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #19     ComponentElement.visitChildren (package:flutter/src/widgets/framework.dart:4600:14)
E/flutter (23080): #20     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #21     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #22     SingleChildRenderObjectElement.visitChildren (package:flutter/src/widgets/framework.dart:5861:14)
E/flutter (23080): #23     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #24     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #25     SingleChildRenderObjectElement.visitChildren (package:flutter/src/widgets/framework.dart:5861:14)
E/flutter (23080): #26     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #27     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #28     ComponentElement.visitChildren (package:flutter/src/widgets/framework.dart:4600:14)
E/flutter (23080): #29     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #30     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #31     ComponentElement.visitChildren (package:flutter/src/widgets/framework.dart:4600:14)
E/flutter (23080): #32     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #33     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #34     ComponentElement.visitChildren (package:flutter/src/widgets/framework.dart:4600:14)
E/flutter (23080): #35     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #36     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #37     SingleChildRenderObjectElement.visitChildren (package:flutter/src/widgets/framework.dart:5861:14)
E/flutter (23080): #38     _InactiveElements._unmount (package:flutter/src/widgets/framework.dart:1931:13)
E/flutter (23080): #39     _InactiveElements._unmount.<anonymous closure> (package:flutter/src/widgets/framework.dart:1933:7)
E/flutter (23080): #40     ComponentElement.visitChildre

Expected behavior
Shouldn't throw exception, and dispose the player normally.

Screenshots
Not needed

Desktop (please complete the following information):

  • OS: [e.g. MacOS + version]
  • Browser [e.g. chrome, safari + version]

Smartphone (please complete the following information):

  • Device: Samsung Note 9 API 29 (Emulator)
  • OS: Android 10.0

Flutter SDK version

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, 1.20.3, on Linux, locale en_GB.UTF-8)
 
[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.3)
[!] Android Studio (version 4.0)
    ✗ Flutter plugin not installed; this adds Flutter specific functionality.
    ✗ Dart plugin not installed; this adds Dart specific functionality.
[✓] IntelliJ IDEA Ultimate Edition (version 2019.2)
[✓] Connected device (2 available)

! Doctor found issues in 1 category.

Additional context
This is tooooooo weird. how does it happen but not many people notice it? maybe it's incompatible issue with flutter version?

@tnaseem
Copy link

tnaseem commented Sep 9, 2020

I'm having the same problem, when using positionStream:

  Widget build(BuildContext context) {
    return StreamBuilder<Duration>(
      stream: widget.player.positionStream,
      builder: (context, snapshot) {
        return _seekBar(
          onChangeEnd: (newPosition) {
            widget.player.seek(newPosition);
          },
        );
      },
    );
  }

Disposing the AudioPlayer in dispose() gives me the exact same error as @AhmedNourJamalElDin's above.

I did try to drain the stream, in an async dispose(), as below:

  void dispose() async {
    await voiceoverPlayer.positionStream.drain();
    voiceoverPlayer.dispose();
   
    super.dispose();
  }

But it appears to get stuck in the await and never gets past it, to run the rest of the code. A little bit stuck now!

@ryanheise
Copy link
Owner

Hi @tnaseem @AhmedNourJamalElDin has graciously made a pull request that you can try out (#182 ). Let me know how it goes for you.

@tnaseem
Copy link

tnaseem commented Sep 10, 2020

Nice work. That did the trick. :)

@ryanheise
Copy link
Owner

Great to know it works, I'll still need to do some testing across all the platforms. This issue is complicated by the fact that the stream is now closed on the platform side, and so we may see different behaviours across platforms, and if so it may require further thought on whether the platform side should do this.

Minimal reproduction project
No need. any project you have which have player in different screens and when you want to go from screen to another, you have to dispose the running player, you face this issue.

It would be great to have some automated unit testing for this project eventually, but until then, each bug fix requires the manual creation of a reproduction project specific for that bug to demonstrate the bug and allow testing before and after fixing the bug, if that bug is not reproducible on the example (which is the case here). I appreciate that you contributed a fix so I'd be more than happy to create the missing minimal reproduction project on my end this time (although providing one will help me to get around to merging this a bit sooner).

@tnaseem
Copy link

tnaseem commented Sep 10, 2020

Great to know it works, I'll still need to do some testing across all the platforms. This issue is complicated by the fact that the stream is now closed on the platform side, and so we may see different behaviours across platforms, and if so it may require further thought on whether the platform side should do this.

I'm wondering then, would it be better to expose _positionStreamController?.close() so we can do it, rather than leaving it to AudioPlayer's dispose() method, as it is in the fix?

ie. Something like:

  _audioplayer.positionStream.close();

We can then just do that prior to disposing the audio player ourselves, for these cases.

@ryanheise
Copy link
Owner

@tnaseem I was also thinking of something along those lines. There is also the fact that it is possible for apps to call createPositionStream manually and create more than one instance of the stream, so it may not make sense to have a global variable to keep track of only one stream controller within the plugin.

@ryanheise
Copy link
Owner

I should mention that the intention of the existing code was to figure out when to call close on EACH instance of the controller that the app created, but clearly this implementation runs into issues if you call dispose without first calling stop. This suggests that one solution may be to just embed a call to stop within dispose, although I'd really prefer to finish off the original implementation in the way that it was intended.

@AhmedNourJamalElDin
Copy link
Author

AhmedNourJamalElDin commented Sep 10, 2020

Hi @ryanheise ,
Apparently, we need to close the controller in any way. feel free to suggest any changes. I'd be happy to do them.

if the users must have access to createPositionStream function, then they need access to the controller to dispose it, but now no one can.

so we can either

  • give them access to the controller (not the stream), at that point, we can store reference to the controller at the class level when calling positionStream for the first time, same as _positionSubject logic. i.e. we have one controller that must be disposed, and the users are responsible for dispoing their own controllers returned by createPositionController (originally called createPositionStream).

  • manage all the controllers and dispose them.

I don't know which is better and I'm not expert at these, but I think giving access to the users is the best option here, what do you think?

@AhmedNourJamalElDin
Copy link
Author

BTW I'm not sure if there is a better way to handle this situation (issue), maybe we don't need to dispose the controller at all, and there is another way to fix this? I tried hard to figure out the issue and this is the fastest way I could overcome it.

@ryanheise
Copy link
Owner

The existing code does try to close every controller automatically as soon as the player is disposed (see line 409 of git master), but there could be a timing issue, and the reason why I didn't notice that timing issue is because I normally call stop before I call dispose. This suggests that another solution might be simply to embed a call to stop within dispose.

@ryanheise
Copy link
Owner

(Although I would rather try to understand why this code is failing and do it correctly)

@ryanheise
Copy link
Owner

BTW I'm not sure if there is a better way to handle this situation (issue), maybe we don't need to dispose the controller at all, and there is another way to fix this?

Possibly, I mentioned elsewhere that it is probably not necessary to close some of these things if the platform side has already given an endOfStream signal on the event channel. As soon as I added that code on the platform side, I noticed some of the closes on the Dart side were failing, so it's a bit of a complicated situation with respect to which part of the code is considered the owner and responsible for closing.

@AhmedNourJamalElDin
Copy link
Author

but there could be a timing issue,

I think that too. relying on a timer to dispose the controller whereas disposing the subject in dispose method. However I tried to stop the currentTimer, but it didn't work. I'm not sure if I was doing it the right way.

@tnaseem
Copy link

tnaseem commented Sep 10, 2020

I should mention that the intention of the existing code was to figure out when to call close on EACH instance of the controller that the app created, but clearly this implementation runs into issues if you call dispose without first calling stop. This suggests that one solution may be to just embed a call to stop within dispose, although I'd really prefer to finish off the original implementation in the way that it was intended.

I did try the stop bofore dispose on v0.4.3, but the problem was still rearing it's ugly head. I then thought it was because they're async calls, so tried:

  voiceoverPlayer.stop().then((_) => voiceoverPlayer.dispose());

But it didn't help either, unfortunately.

@ryanheise
Copy link
Owner

I've finally had a chance to create a minimal reproduction project and do some testing.

The correct solution is as suspected in the original #169 issue which is to simply delete this line from dispose:

await _positionSubject.close();

_positionSubject is getting its events from another stream via addStream (effectively the same as pipe) which means that it will be closed automatically when the original stream controller is closed (which it is already, so no additional code is needed).

Let me know how that goes, and I'll include this in the next release.

@AhmedNourJamalElDin
Copy link
Author

Perfect, Thanks.

@tnaseem
Copy link

tnaseem commented Sep 13, 2020

Just tried it (0.4.4) and it works great. Nice work and thanks!

@github-actions
Copy link

github-actions bot commented Nov 6, 2021

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs, or use StackOverflow if you need help with just_audio.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 backlog bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants