-
-
Notifications
You must be signed in to change notification settings - Fork 275
Description
The TaskScheduler is well-designed in allowing both immediate execution of tasks via enable() or delayed execution via enableDelayed(). The documentation for enable() says that it "enables the task, and schedules it for immediate execution (without delay) at this or next scheduling pass depending on when the task was enabled." That's likely to cause confusion and bugs, however, for a few reasons:
-
running tasks immediately upon enable() means that the may execute at incorrect times. For example, a task may be set to run once per second, but if the enable() is called more than once in a second, the task will run more than once per second. This semantics means that the default behavior can lead to bugs. Programmers have additional burden to code this correctly.
-
"Enable" commonly means to make possible, not to do. So the function name is misleading.
-
For the same reason, enable is a function that should be idempotent. Intuitively, if you enable something that is already enabled, nothing should happen.
-
It doesn't scale. Suppose we have a thousand Tasks all with different timings. Suppose further that enabling them all via with enableDelayed() works: they all run correctly at their appointed times. But with the current semantics, using enable() and all of them would make them all run immediately, likely leading to conflicts or overload.
Therefore enable() should use the delay, and enableImmediate() should be created to run immediately. Or to minimize disruption for existing users, deprecate enable(), leaving enableImmediate() and enableDelayed() to maximize explicitness and clarity.