Make !stop work when chillin

[Cephian]

Closes #77.

How it works:
Instead of splitting "end the bust" code between command_stop() and play_next_song() depending on whether a song is stopped or just hits the end of the playlist, it was unified via a function finish_bust().

• If the bust ends because the end of the playlist was reached, then play_next_song() detects that current_channel_content is empty (but not None) and runs finish_bust()
• If the bust ends because !stop was called during a song, then play_next_song() is called because FFmpeg finished playing, but detects that current_channel_content is None (so finish_bust() has already been called) and just returns
• If the bust ends because !stop was called while chillin, we need to detect that !stop was during the asyncio.sleep() statement in play_next_song when we return from sleeping. We can NOT simply check to see if current_channel_content is None, since it's possible that during the sleep time we stopped, started another bust and populated that list again. Then we add a variable current_bust_invocation which counts the total number of busts that have begun, and ensure that the bust counter before and after we sleep are the same (as well as ensuring current_channel_content is not None)

[anoadragon453]

I think the bug was that this check should actually be or not active_voice_client.is_connected(), rather than using is_playing(). The latter is only True when audio is actively playing, which it isn't during our asyncio.sleep.

[anoadragon453]

I think this might be a copy-paste error from finish_bust? We shouldn't need any of these as we're not modifying these values in this function.

[anoadragon453]

Another way we could implement this is to simply cancel the execution of this whole function when !stop is called.

We can wrap this coroutine (inner_f) in an asyncio.Task, which can be interrupted whenever we like. Thus we don't need to worry about this coroutine continuing on with false assumptions.

This commit demonstrates how it might be done: fd1c754

[Cephian]

I'd like to ask a few questions as I'm not super familiar with how coroutines work:

1. Since we call finish_bust() from within inner_f(), and finish_bust() cancels the play_next_song_task (which is running inner_f()), is there any danger of finish_bust() cancelling itself? this doesn't seem to happen in my testing, but with async code that doesn't mean it can't happen

EDIT: This can happen, I was just getting the condition wrong. If you let a bust complete normally instead of stopping it, the finish_bust() command will be stopped mid run and will never cleanup properly. However I think we should be able to get around this by stopping the coroutine only in command_stop() EDIT2: we can

2. When implementing the changes in your test commit I get an inconsistently reproducable error

Task exception was never retrieved
future: <Task finished name='Task-227' coro=<play_next_song.<locals>.inner_f() done, defined at /home/jack/Unsynced/busty/main.py:389> exception=TypeError("'NoneType' object is not subscriptable")>
Traceback (most recent call last):
  File "/home/jack/Unsynced/busty/main.py", line 414, in inner_f
    current_channel_content = current_channel_content[skip_count:]

I've been able to reproduce it at least 3x by 1) starting a bust, 2) stopping during the first song, 3) starting a bust again (though it doesn't happen every time). The exception happens right when the first "Currently chillin" pops up. The bust itself appears normal from a user's perspective, so I assume this is somehow a leftover from the previous bust which is throwing an exception.

I'm pretty sure I implemented all your suggestions correctly, but just in case I've pushed the code that throws this exception so you can see it yourself.

[anoadragon453]

@Cephian could you update this PR now that #112 has merged, and test with the latest version of nextcord to see if the error mentioned in #78 (comment) still occurs?

[Cephian]

Rewrote this to accommodate all changes in master. The structure is now such that #78 (comment) is no longer relevant, as the task is now ONLY cancelled for !stop and not if the bust ends normally.

play_next_song() is now always called wrapped by method play_next_coro, which launches play_next_song() as the task play_next_task which is stored globally, so it can be cancelled at any time.

I also added a say_goodbye flag to finish_bust() so that it can be called easily without the "Thanks for coming" message when the bust is !stopped.

Detecting cancelled tasks

The Python interface for cancelling tasks can be somewhat unintuitive, in that

task.cancel()
print(task.cancelled())

will output False, since task.cancel() only marks task to be cancelled, and it is only /actually/ cancelled once it's woken up (see cancel method here). Weirdly, when I cancel play_next_task while a song is playing, in the ffmpeg post-hook play_next_task.cancelled() still comes up as False (I still don't really understand how the hook runs before the task is cancelled but ¯\_(ツ)_/¯). In order to get around this I introduce a global boolean play_next_cancelled.

Side note

I suspect that there is all kinds of weird behavior theoretically but not practically possible with async where you can start a bust before the previous one finishes cleaning up. For example, it's in theory possible to finish a bust and start a new one while the bot waits for the most recent "now playing" message to be unpinned while stopped, in which case you'd get two different play_next_song coroutines going at once. We could either

1. Hack around this type of stuff more with some global bust counter (no)
2. Rewrite our global-variable centric architecture to use one "bust object" per bust that gets passed around to methods manually
3. Ignore it

I think 2) would be good from a software engineering perspective, but I'm not really sure how much effort it would be worth considering the bot works fine as-is.

[anoadragon453]

This looks sensible in terms of the overall change. I just have the usual small code nits for you now :)

I think 2) would be good from a software engineering perspective, but I'm not really sure how much effort it would be worth considering the bot works fine as-is.

I've had thoughts about doing this before. It would bring a few benefits:

1. One bot instance could manage multiple servers at once.
2. IDEs currently complain about the typing in our codebase, as our global variables are marked as either <type> or None. We use None to indicate that a bust is not currently active, but that means that in several places where we use the variable - where it doesn't make sense for the value to be None - we get type warnings.
   Having an "bust" object that instantiated when a bust is created, and where the values are only used when a bust is running, should allow us to eliminate the need to mark than as Optional.
3. Better compartmentalization of code; both in the case you mentioned, as well as being able to easily move such a dataclass out to a separate file.

I have shied away from doing it before though, mostly because reason 1 in this list isn't really necessary for our use case. But I think it'd be a nice to do eventually.

I still don't really understand how the hook runs before the task is cancelled

Without digging into nextcord too deeply, I can see a few places where it catches CancelledError, which is what gets raised when .cancel is called.

https://github.com/nextcord/nextcord/blob/a49ffd2a102b339aa9834d7d70c040bb305478a0/nextcord/shard.py#L183-L184

.cancelled will not be True unless the coroutine bubbled up the CancelledError. Nextcord seems to just eat it here, so that may be why.

[Cephian]

Knowing that nextcord may swallow CancelledError makes me slightly suspicious, it's fine if the ffmpeg step stops without raising the cancelled exception since play_next_cancelled is checked directly after, but if it's swallowed by one of the current_channel.send(embed=embed) lines it may eventually throw an exception or play things it shouldn't.

In practice it's probably not a huge deal since almost everything will be cancelled either during song playback or during the await asyncio.sleep(seconds_between_songs) line. I'm still in favor of merging.

[anoadragon453]

Yes, I agree and then we should merge and see what happens.

Definitely test privately before the bust tomorrow though :)