Exception in cog_unload stops module cleanup

[SebbyLaw]

Summary

Throwing an error in a cog's cog_unload method leaves the cog's module to stay in sys.modules and the internal Bot.__extensions list. This means that the cog is stuck in a state where it can't be loaded (since the module it resides in is "still loaded") until the unload_extension is called for a second time to clean up. (For n cogs in an extension raising errors, the extension must be unloaded n + 1 times for the module to be fully unloaded.)

Reproduction Steps

in cogs/BadCog.py:

from discord.ext import commands

class BadCog(commands.Cog):
    def cog_unload(self):
        raise Exception()

    @commands.command()
    async def foo(self, ctx):
        await ctx.send("still loaded!")

def setup(bot):
    bot.add_cog(BadCog())

Using jishaku:

?jsk load cogs.BadCog
📥 cogs.BadCog

?foo
still loaded!

?jsk unload cogs.BadCog
📤⚠️ cogs.BadCog`

?foo << this message receives no response

?jsk unload cogs.BadCog
📤 cogs.BadCog

Expected Results

One of the following:

• Proper cleanup ignoring exceptions in either Cog._eject, Bot.remove_cog, Bot._remove_module_references, or Bot.unload_extension
• Atomic unloading, meaning that Cogs are re-loaded to the bot if unloading the extension fails.

Actual Results

I am sad.

Intents

N/A.

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.
• I have provided the intents that my bot is using.

System Information

N/A.

[Rapptz]

So I'm of the opinion that unload callers shouldn't error and if they do then they should get ignored. This is also what happens with e.g. teardown in extensions.