Scheduler Redesign

[MarkKoz]

Note: this is written as if #755 has already been merged.

Current Problems

1. Scheduling different coroutines cannot be done easily
2. With the inheritance design, classes can't really share a scheduler
3. There's no way for end users to see errors which occur within tasks

Description

The scheduler currently designed with inheritance. The task scheduled is an abstract method which subclasses have to implemented. This design is restrictive because it only supports that one coroutine for scheduling. Some features may require several different coroutines to be scheduled. While it's possible to defer to other functions inside the abstract method, it gets messy. A better design seems to be to allow specifying the exact coroutine one wishes to schedule.

The above change would remove reliance on an abstract method thus allowing for composition to be used instead of inheritance. This is useful when different code needs to share a scheduler. For example, the ModManagement cog currently relies on the Infractions cog so it can re-schedule infractions if durations are edited. With composition, the scheduler could be created and passed to both cogs when they get instantiated in setup().

Similar to specifying the coroutine to schedule, a callback for exception handling can also be specified. By default the exception will simply be logged. If a user-invoked command scheduled a task which raised an exception, the user won't know about it - the command may fail silently from the user's perspective. In such case, the aforementioned callback would enable a message to be sent to the user. As another example, it could be used to still send a mod log if an automatic expiration of an infraction raised an unhanded exception.

---

I'm labelling this as "planning" for now to get thoughts on the above. Do you believe these are indeed problems with the current designs? Do you think the proposed changes are good? What other problems can you identify with the scheduler?

[MarkKoz]

Putting this as high priority as this would be helpful for #786, which will need to schedule separate coroutines for making channels dormant and for removing permission overwrites for users.

[eivl]

What you describe is a common situation and using composition to do object delegation is imho the correct solution.

[MarkKoz]

What should happen if an attempt is made to schedule using a taken ID (meaning another task is currently scheduled with that ID)? Currently, only a debug log is sent. The problem is that the coroutine given will never be awaited and will eventually cause a warning.

A boolean value could be returned to indicate success or failure, which will put the burden on the caller to close the coroutine and prevent the warning. However, this is cumbersome; pretty much every caller will want to ensure the coroutine gets closed if unawaited.

Alternatively, the schedule() function could close the coroutine if it cannot be scheduled. This means the caller cannot re-use their coroutine, which may be desirable for some sort of retry logic. However, it is unlikely callers will want to do this.

Finally, a coroutine function and its args could be taken as params rather than taking a coroutine. The scheduler could entirely avoid creating the coroutine if the ID already exists, which means it won't even need to close any coroutine. This is similar to how the standard library does it, as seen here. I have mixed feelings about this, but I think the syntax will end up cleaner than passing a coroutine since it avoids nested function calls. For example,

# Proposal
schedule(1234, my_coro, arg1, arg2)

# Current
schedule(1234, my_coro(arg1, arg2))

Of course, if it was a coroutine, it could be assigned to a variable on a separate line, which is also pretty clean, especially when trying to avoid long lines.

coro = my_coro(arg1, arg2)
schedule(1234, coro)

But, args could also be put in a tuple on another line

args = (arg2, arg2)
schedule(1234, coro, *args)