Potential memory leak with VoiceClient.stop

[PaeP3nguin]

Summary

Calling VoiceClient.stop while the voice client is reconnecting may result in a memory leak

Reproduction Steps

The basic gist of what I think happened is:

1. The VoiceClient gets DCed and attempts to reconnect. This happens naturally during normal operation. At some point, the _connected event is unset
2. VoiceClient.stop is called while the event is unset
3. The VoiceClient's AudioPlayer stop call and advances through its while loop and blocks on the _connected event: https://github.com/Rapptz/discord.py/blob/master/discord/player.py#L679
4. If the VoiceClient is fully disconnected, such as through a call to disconnect and the instance is not reused, the AudioPlayer remains stuck

Unfortunately, I'm not familiar enough with the discord.py internals to know the exact sequence of events and internal calls involved. I've attached a log I took during a local repro. logs.txt

Minimal Reproducible Code

@commands.command()
async def min_repro(self, ctx: commands.Context):
    content: discord.FFmpegOpusAudio = <get some audio>

    voice_channel = self.bot.get_channel(<voice channel ID>)
    voice_client = await voice_channel.connect()
    voice_client.play(content)
    await asyncio.sleep(5)
    # Simulate a disconnected voice client that is trying to reconnect. Not sure of a better way to repro.
    bot.loop.create_task(voice_client.potential_reconnect())
    voice_client.stop()
    await voice_client.disconnect()
    await ctx.message.add_reaction('✅')

When running the repro code, you can tell if the repro worked by checking if there's a "started" AudioPlayer thread once the command completes:
!jsk py

import objgraph
objgraph.by_type('discord.player.AudioPlayer')

You'll see something like:

[<AudioPlayer(Thread-46, started daemon 22188)>]

Note that this is not a 100% consistent reproduction for me, so there might be some timing component.

Expected Results

The AudioPlayer thread should stop.

Actual Results

The AudioPlayer thread remains running and blocked on the _connected event. This leaks because the AudioPlayer holds onto the VoiceClient, which holds onto the voice channel, which holds onto the guild. Since AudioPlayer is a live thread, it is held by the threading module. You can verify the stack frame like so:

!jsk py

import io
import objgraph
import traceback
import sys

with io.StringIO() as f:
  ap = objgraph.by_type('discord.player.AudioPlayer')[0] # Pick the index for the stuck AudioPlayer
  stack = traceback.format_stack(sys._current_frames()[ap.ident])
  f.write('\n'.join(stack))

  f.seek(0)
  out_file = discord.File(f, filename='graph.txt')

  await _ctx.reply(
        file=out_file,
  )

Intents

Regular intents + message

System Information

Repros on local machine:

- Python v3.11.0-final
- discord.py v2.0.1-final
- aiohttp v3.8.3
- system info: Windows 10 10.0.19044

Also on docker with python:3.11-slim-buster image and aiohttp==3.8.3 and discord.py[voice]==2.0.1.

Checklist

• I have searched the open issues for duplicates.
• I have shown the entire traceback, if possible.
• I have removed my token from display, if visible.

Additional Context

No response

[Rapptz]

I'm not exactly sure how to repro this and I don't really have the time to boot up a bot to test this theoretical out right now, but looking at this code it might be worth rewriting parts of it to use a state machine instead of multiple events. Python doesn't particularly have atomics though which would make this rewrite slightly simpler but I can think of a few ways of doing it with a condvar instead.

That being said, I can see how this would be theoretically possible by looking at the code.