-
-
Notifications
You must be signed in to change notification settings - Fork 689
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
Restart proxy in case of -1004 error #812
base: minor
Are you sure you want to change the base?
Conversation
Since the errors have different meanings on different platforms, can you tell me which platform this -1004 error is relevant to? |
The proxy only turns off on iOS, so, iOS. |
Just checking what -1004 means, and it is related to network errors, so that sounds reasonable. There should at least be a platform check in the code, although I want to go through and check how the Android side works again, and maybe try to route both errors through the same place so that it can be handled consistently. |
Isn't this error code fired by _server = await HttpServer.bind(InternetAddress.loopbackIPv4, 0); ? Maybe the codes are all the same, since it is a Dart server. |
I'll have to check the code later on the first part, but regarding HTTPS, it's not straightforward since you'd need to host an SSL private certificate on the device. |
I am facing the same issue on iPhone device. Happens when the screen goes dark (app goes to background). Once the error happens, I have to close the app and reopen for it to play any songs. |
@rguntha Are you testing this pull request? |
if (e.code == "-1004" && source is LockCachingAudioSource) { | ||
// proxy is offline | ||
try { | ||
await _proxy._server.close(force: true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't closing the proxy after this error potentially disrupt other active connections on the proxy? That is unless the -1004 error implies all connections are affected, but it seems possible that the -1004 error could happen on one connection and not another.
Just want to put it out there, that this issue happens for StreamAudioSource as well. |
-1001 corresponds to However this does raise the question of what other error codes could apply. They are all listed here: https://developer.apple.com/documentation/foundation/1508628-url_loading_system_error_codes |
Understand. Shall we handle these error in our codes and dispose and recreate the AudioPlayer on these errors? |
Yes, recreating the AudioPlayer is the way that people are currently dealing with this issue. It may actually be a good idea if I were to make AudioPlayer.stop() automatically stop the proxy as well, as that would provide a convenient way for apps to reset an existing AudioPlayer instance without having to create a new instance. |
No. I am testing the latest version 0.9.29.
I wanted to point out that, once this error comes, it persists even when
changing the song to another one. Only after reopening the app it starts
working.
…On Thu, 6 Oct 2022 at 9:19 PM, ryanheise ***@***.***> wrote:
@rguntha <https://github.com/rguntha> Are you testing this pull request?
—
Reply to this email directly, view it on GitHub
<#812 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA2WSBI6V6AMIDR6DZTKFYDWB3YH3ANCNFSM6AAAAAAQK3CNUE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Since you are commenting on this pull request, why not use it? |
The problem I am facing is same, but with StreamingAudioSource. But the fix is specific to LockCachingAudioSource. As I suggested in the earlier comment, I request to add StreamingAudioSource, so that I can use the pull request |
The fix looks general to me. Please try it. |
By the way, @rwrz , do you have reproduction steps for the iOS side? Is it triggered when the phone is turned off, or is it when entering airplane mode, or something else? In some of those scenarios, I wonder if we could also consider listening to network connectivity events as a cue for when the proxy has entered this unresponsive state.
On the other hand, this is also not ideal since resetting the player will disrupt all items in the playlist. If some of them are local files, those shouldn't be cleared from the player's buffer just because the proxy needs to be restarted. So either the solution should be something like this PR, or it should be an API that allows the app to manually restart the proxy based on the conditions it determines itself. |
I've tried to manually reproduce the issue, but I couldn't. I believe the OS has some maintenance routines killing open connections and when it happens, the proxy gets stuck into this state. But it is an assumption, I'm not sure what is causing it. I also wonder if this isn't a bug with the |
I have the same suspicions, and once I understand the issue fully I may end up submitting a bug report to the dart sdk. However, bug fixes take a long time to get into the stable dart/flutter release, so until then a workaround is still what we'll have to use. |
Any updates on this issue. I have restorted to disposing and setting up the player again. |
My issues are completely around playing a live audio stream. We have some download capability in the app, but that's not where the issues appear for me. I'm seeing a lot of SocketExceptions in Sentry that seem like they could be similar to what is being discussed here and in #594. I'm not 100% sure, but maybe @ryanheise could point me in the right direction if not.
The stack trace from Sentry shows: Which seem to align with comments above. I also see a similar number of I'm having a hard time reproducing the issues locally, but we definitely have a lot of users that are experiencing the issues. It's hard to tell if it's more on Android or if they're just more vocal so we hear it from those users more than iOS users. I've had issues in the past, mainly with internet cutting out, where I will need create a new AudioPlayer instance and restart the stream, but it's not working well for this issue. I do that in the |
@ryanheise hi, any concerns against merging this fix to main branch and releasing new version? It would be really helpful to get rid of the issue on ios and great if you can do it |
This pr is the recommended solution right now, however I need to understand exactly what other scenarios if any can produce the same error code, and also whether similar scenarios can produce different error codes before using the error code alone to determine the response. In the meantime you can use this pr directly even though it may not be convenient. Alternatives I may want to try may include trying to listen to the network state directly if possible, and it may also be a good idea to report this to the dart repo as a next step. |
@ryanheise any progress, new learnings or workarounds on that issue? |
@rwrz I did apply your changes onto the latest version, and it works so far for me. Anyone can check it out by using the following dependency: # just_audio: ^0.9.37
just_audio:
git:
url: https://github.com/appinteractive/just_audio.git
ref: 555e945d219be8c91dcdda198c0a2a495f6e9f1a
path: just_audio |
I've updated to the most recent changes (from minor) and also fixed the conflict. |
Awesome, can't wait for your changes to land in the next update. 🥇 |
@rwrz Does your change have to have the |
hmmm... I've added it there just as a guardrail initially. |
Is the -1004 Error always a failing proxy? If so, it should be safe to remove, as it would always be beneficial to just keep it alive. |
Yes. It happens when the proxy server is killed by the OS. The problem is that the DART proxy can't understand it by itself, leaving the proxy server in an "unexpected" state where it is not closed neither opened, but it can't receive more connections. |
@rwrz @appinteractive So I've found my experience is slightly different. Maybe this is a discussion for a different thread, but I think it's still relevant here. Anyway, this current fix is great and catches the -1004 in the _load() method. However, this is what I'm seeing:
I'm 99% sure it's the same -1004 error, but since we aren't using _load() and instead calling play() directly we don't see the -1004 error surface. I spent the last couple days trying to find a way to surface the error but I couldn't figure it out. Any thoughts? |
So when it's that case, I think not checking for the source of the issue but only for the code should be safe or not? |
This is the fix for #594 .
Since it is not possible to identify when the proxy server closes if it get disconnected, the only way I have found is to catch the "-1004" error and restart it.
I'm using this in production for a while and it works.
Let me know if you have any idea on how to improve this.
I believe we would need to use a better HTTP server to catch this "disconnection by peer" issue.
The main fix is here:
But I've added a few more handlers (on the
start
method) trying to catch the disconnection, but it never catches. Anyway, I think it doesn't hurt.