Just-Some-Bots / MusicBot

:musical_note: The original MusicBot for Discord (formerly SexualRhinoceros/MusicBot)
https://just-some-bots.github.io/MusicBot/
MIT License
3.12k stars 2.35k forks source link

Stop spotify playlists fetch on clear command #2378

Closed itsTheFae closed 9 months ago

itsTheFae commented 9 months ago

Description

Adds yet another server-specific data entry to stop playlist loops from queuing songs after a clear command has been issued. This PR has only focused on the spotify lists, since those seem the worst offender.
Please inform if other playlist logic should be checked!

In short, when command clear is issued the halt flag is set, and it will be cleared when the next call to play a song is made.

Related issues (if applicable)

closes #2375

BabyBoySnow commented 9 months ago

I don't see any reason to just make this for spotify, might as well make it work for any event where clear is issued and song(s) are still being downloaded

itsTheFae commented 9 months ago

After reading and poking through the code I have concluded that this feature cannot currently be extended to the other playlist types that are actually handled by ytdl. At least, not without restructuring how we interface with ytdl.

While all playlists take time to process and download, those handled by ytdl appear to queue all extracted URLs at once when the extraction process is finished. Only the spotify playlist implementation suffers from the continuous, sequential queuing issue, which this patch improves but does not 100% solve.

In my testing I have found that some of the time, issuing a clear command would not halt the playlist unpack. This is a result of the flag to halt playlist unpack being unset on a subsequent play call, and the current playlist implementation directly calls play to enqueue the track search terms. A race condition that cannot be avoided without restructuring how we deal with spotify.
Issuing multiple clear commands typically overcomes this, and I think this is acceptable for the time being considering that without this patch, one must issue clear an equal number of times to the number of playlist tracks. So a potential reduction from dozens or hundreds to only 2 or 3 commands.

With all this said, I think this PR is as good as it is going to get for now and is ready for merge.