E-Kuerschner / useAudioPlayer

React hooks for controlling audio on the web
MIT License
315 stars 35 forks source link

HTML5 network requests in Chrome stuck as pending after 6th song? #85

Closed uncvrd closed 2 years ago

uncvrd commented 2 years ago

Hi! Thanks for creating this excellent library. I ran in to a bug, and I'm not quite sure if it's HowlerJS or this library's implementation of HowlerJS that's causing the following issue, so any guidance would be super helpful!

I discovered that Chrome is unable to resolve an HTML5 206 partial response network request after the 6th file has been loaded. So for example, if I have a queue of songs, I could click "play" to stream each track, but when I try and stream the seventh track, it hangs as "Pending" in the network tab. After some searching, I discovered that Chrome limits the number of active network requests the browser can poll, see this SO answer for more of an explanation: https://stackoverflow.com/a/29639535/6480913

This has left me puzzled because observing the network tab, we can see that all 206 partial requests have resolved, so why would this last file still hang? I figure this has to do with how Howler manages the state of the audio nodes generated programmatically? And for some reason Chrome thinks that the previous 6 audio files are still polling even though their corresponding network requests have resolved? I think I'm on the right track but I am stuck on how to move forward.

Here's a small code sandbox reproducing this problem. I added two play buttons, every time you click a play button, a unique ID is appended to the url to prevent the browser from caching the audio file.

Sandbox: https://codesandbox.io/s/hopeful-kare-ih0omh?file=/components/Music.tsx:0-996

Steps (Must be on Chrome / Brave):

  1. Open up the network tab (specifically the media network tab)
  2. Cycle between clicking play of the first button and second button 6 times
  3. On the 7th click, observe the network tab and see how the request never resolves (stuck at pending).

Here's a video showing the problem: https://streamable.com/qlqryy

Expected behavior I expect Chrome to not prevent loading of more than 6 songs.

Environment (please complete the following information):

Thank you for any insight!!

uncvrd commented 2 years ago

Hi again! I believe I've uncovered the issue. In the AudioProvider, nodes are not being unloaded when a new src is loaded. As a result, sockets are not released and Chrome has a limit (as explained above) of 6 active sockets and will hold any more requests in a network pending state until a socket closes.

A user on stackoverflow succinctly explains the problem here: https://stackoverflow.com/a/32275851/6480913

The above resolution is actually what HowlerJS tries to do on unload to release the socket too.

I discovered in the AudioProvider, there's a line to only unload a Howl IF it's still being loaded (as determined by the reducer state), but we still need to unload the component when changing tracks.

Therefore I propose to update to add the following to the Provider, referenced here: https://github.com/E-Kuerschner/useAudioPlayer/blob/c65d28917210dff060d59b3e115d0be6f7c962d0/src/AudioPlayerProvider.tsx#L58

// Existing Code 
if (loading) {
    // existing code!
    prevPlayer.current = playerRef.current
    prevPlayer.current.once("load", () => {
        prevPlayer.current?.unload()
    })
}  else {
    // this else statement is new!
    // the howl has already loaded but when we change tracks we need to make sure
    // that the existing track unloads so that sockets are released

    prevPlayer.current = playerRef.current
    prevPlayer.current?.unload()
}

As a result we make sure to unload a howl when changing a sound. I've run this locally in my code and this fixes the problem. I'm not sure if this will cause any other problems, was there are reason to avoid unloading a howl here?. I can make a PR with this implementation if you deem this would be acceptable?

Thanks!

E-Kuerschner commented 2 years ago

@uncvrd thanks for the excellent discovery! Yeah, as I was reading your first comment I was beginning to suspect something to do with garbage collection or poor cleanup.

If you'd like to take initiative, feel free to start a PR and we can base the rest of the conversation around specific code.

Thanks!

E-Kuerschner commented 2 years ago

@uncvrd just published version 1.2.6 with the fix!

uncvrd commented 2 years ago

@E-Kuerschner excellent - huge thanks for being so quick to push out an update! I'll report back if I run in to any problems

E-Kuerschner commented 1 year ago

Jordan, nice catch! When I was reading your first comment I was starting to think it must be bad garbage collection or failure to clean something up. Thanks for putting in the time to find the source of error. Feel free to get a PR up and I can work with you to get that in this week!

On Sun, May 15, 2022, 9:46 PM Jordan Lewallen @.***> wrote:

Hi again! I believe I've uncovered the issue. In the AudioProvider, nodes are not being unloaded when a new src is loaded. As a result, sockets are not released and Chrome has a limit (as explained above) of 6 active sockets and will hold any more requests in a network pending state until a socket closes.

A user on stackoverflow succinctly explains the problem here: https://stackoverflow.com/a/32275851/6480913

The above resolution is actually what HowlerJS tries to do on unload to release the socket too.

I discovered in the AudioProvider, there's a line to only unload a Howl IF it's still being loaded (as determined by the reducer state), but we still need to unload the component when changing tracks.

Therefore I propose to update to add the following to the Provider, referenced here: https://github.com/E-Kuerschner/useAudioPlayer/blob/c65d28917210dff060d59b3e115d0be6f7c962d0/src/AudioPlayerProvider.tsx#L58

// Existing Code if (loading) { // existing code! prevPlayer.current = playerRef.current prevPlayer.current.once("load", () => { prevPlayer.current?.unload() }) } else { // this else statement is new! // the howl has already loaded but when we change tracks we need to make sure // that the existing track unloads so that sockets are released

prevPlayer.current = playerRef.current
prevPlayer.current?.unload()

}

As a result we make sure to unload a howl when changing a sound. I've run this locally in my code and this fixes the problem. I'm not sure if this will cause any other problems, was there are reason to avoid unloading a howl here?. I can make a PR with this implementation if you deem this would be acceptable?

Thanks!

— Reply to this email directly, view it on GitHub https://github.com/E-Kuerschner/useAudioPlayer/issues/85#issuecomment-1127157632, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQWVEQCJZZ42BU4HFV5XL3VKGZJFANCNFSM5V6OJCCQ . You are receiving this because you were assigned.Message ID: @.***>