Concurrent playlist queuing causes out-of-order queue

[rafaeldamasceno]

Description

When multiple people are using a /play command with playlists, the tracks are added as soon as they are processed, which means the original order of the playlists isn't preserved.

Expected vs. Observed

-	-
Expected	The queued playlists are queued one after the other, in the order they were called.
Observed	Playlists are queued in an interleaved way.

Repro Steps

1. Use /play end with a playlist.
2. Use /play end with a playlist very quickly after the previous one.
3. Use /queue.

Environment

Key	Value
Operating System	Ubuntu 21.10 impish
Kernel	x86_64 Linux 5.13.0-28-generic
CPU	Intel Core i5-6500 @ 4x 3,6GHz
RAM	15893MiB

Screens

[afonsojramos]

I'm not sure if this is a bug or just expected behaviour, but I'm leaning towards the latter... 🤔

[rafaeldamasceno]

It really shouldn't be, specially with the /play next and /play jump commands now available. A lot of weird behavior happens with these if you try to queue a playlist, and this is the root cause.

[joao-conde]

I really see no way of achieving this. The command execution is asynchronous. Calling /play means executing our play async command, which is doing its work asynchronously like all Serenity commands.

[rafaeldamasceno]

I understand what you mean, but the reality is that that the bulk of the song processing is done in enqueue_source and this function is always invoked in the lock. Just spitballing and I have no real deep knowledge, but it looks like adding a new lock in https://github.com/aquelemiguel/parrot/blob/main/src/commands/play.rs#L243 wouldn't affect performance or behavior. The play commands are both fighting for the enqueue_source lock to queue their playlist songs: what would be the difference if we let a single playlist do all of its calls before the other?

[rafaeldamasceno]

The play commands are both fighting for the enqueue_source lock to queue their playlist songs

On second thought, it's not so obvious that this is case. I'd need to have a talk with you to better understand the asynchronism in the project.

[afonsojramos]

It really shouldn't be, specially with the /play next and /play jump commands now available.

Well, let's see what results from the discussion in #151 then :eyes: But still, I think it is expected behaviour.

[joao-conde]

@rafaeldamasceno If you take locks you prevent all commands from running, like if you lock while you enqueue 100 songs, I think you wouldn't be able to use other commands (@aquelemiguel do you confirm)

[aquelemiguel]

@joao-conde @rafaeldamasceno I can confirm this is 100% expected behaviour and that when a command locks the Songbird handler, it locks it for the entire application. This means that you would not be able to use any other commands that interact with the queue while the playlist is processing (this is a big no-no).

It really shouldn't be, specially with the /play next and /play jump commands now available. A lot of weird behavior happens with these if you try to queue a playlist, and this is the root cause.

I'm a little more concerned with this instead, could you provide repro steps for this?

[danrpinho]

I don't really agree with it being expected behaviour. Visualising this in another way, if I wanted to insert two bags of marbles (playlists) in a tube (the queue) I'd expect that all marbles in the first bag would be inside of the tube before the first marble of the second bag entered. If I wanted to mix the songs in the two playlists, I could use /shuffle to get that effect.

From a UX perspective, I don't find being unable to use other queue-based commands as a bad thing if we provide enough feedback to the user. A message saying "adding songs to queue" when a playlist past a certain size threshold could be enough in my opinion. Are there any other reasons as to why these commands should be available 100% of the time?

[rafaeldamasceno]

This is a little unfortunate. I think it’s not crazy to say that any user, when queueing some playlists, would expect the playlists to be queued in order, one after the other. I understand the ansynchronous nature of the Songbird handler complicates this, but is there no way to achieve this?

I'm a little more concerned with this instead, could you provide repro steps for this?

@aquelemiguel I’m off my machine for today, but it consisted mainly of spamming the play commands with big playlists and then using the jump command on another playlist. I expected all of the tracks of the playlist to be enqueued at the top of the queue and, when done queueing, for the first track of this playlist to start playing. Instead, some other random track played. I’ll try to get better repro steps and finding a proper pattern tomorrow.

[aquelemiguel]

@rafaeldamasceno @danrpinho Don't get me wrong, it's intended but I agree it's confusing behaviour.

I can't think of a simple way to implement this other than creating a queueing system where tracks wait for their turn to be decoded. I think this is actually a cool improvement.

[joao-conde]

From a UX perspective, I don't find being unable to use other queue-based commands as a bad thing if we provide enough feedback to the user

@danrpinho this would block everything, meaning while you queue 100 songs, for about, I don't know, 2 minutes, you can't use ANY command, like /queue /np and what not. This is absolutely not great.

[joao-conde]

@rafaeldamasceno @danrpinho Don't get me wrong, it's intended but I agree it's confusing behaviour.

I can't think of a simple way to implement this other than creating a queueing system where tracks wait for their turn to be decoded. I think this is actually a cool improvement.

We would need to implement our own queueing system or separate thread that then enqueues it. Seems we are over-engineering it for a not-so-common use case that is not that shocking. In fact, we should probably look into speeding up the enqueueing system itself :D

[rafaeldamasceno]

That would be a great idea! In fact, most bots I have used before were really fast at queueing tracks. The one I ran before only downloaded metadata for queueing purposes, and only downloaded the track when playing (in fact, a lot of times it errored out because it wasn’t able to play it).

[StaticRocket]

In fact, we should probably look into speeding up the enqueueing system itself

Eh, from what I've seen there's not a lot we could do for that. We could attempt to add some form of info.json caching and loading, but the initial lookup is still ~1 second unless we want to start querying directly through an innertube api host.

Really unfortunate. I was hoping some of the Piped mirrors they added to the extractor would help with lookup times.