Bug in gluon/scheduler.py

[Tom Clerckx]

Version:

Version 2.22.5-stable+timestamp.2022.06.04.18.13.51

There is a problem with the calculation of next_run_time in gluon/scheduler.py at line 1024

It calculates:

steps = secondspassed // task.period + 1

However, there is no check done for task.period being 0

This can cause the following scheduler error:

ZeroDivisionError: float divmod()

I think it would be better to initialize next_run_time and change the last else condition to "elif task.period".

[Dave S]

How did you get task.period == 0?

I'm wondering if this is a valid use-case; the scheduler has quite a test suite.

/dps

 

[Tom Clerckx]

Our tasks are added using scheduler.queue_task(...) function.

By default, we set the period to 0 

Later on we added to the defaults : prevent_drift = True

It's this last change that triggered the observed error.

[Dave S]

On Saturday, August 20, 2022 at 11:39:55 AM UTC-7 Tom Clerckx wrote:

Our tasks are added using scheduler.queue_task(...) function.

By default, we set the period to 0 

Why?  If you don't know the period, why are you queuing it?   Or are you using the "cron" type of task scheduling? 

 

Later on we added to the defaults : prevent_drift = True

It's this last change that triggered the observed error.

Eveything I know about the scheduler has been shown to me by Stefan (Niphlod), who wrote the subsystem and the tests.  I suspect I would have to spend quite a bit of time to come up with an analysis of what is really wrong, but I'm really suspicious of queuing a period of 0.

/dps

[Tom Clerckx]

We usually set the period to 0 for tasks that run only once.

Hence, the period should not matter at all.

This works fine, unless you also set the prevent_drift to True -- this causes the division by zero.

I agree that it is a bit silly to set prevent_drift=True for a task that runs only once.

I also believe it makes sense to make the code more robust. 

I think it is quite easy to fix this in the code as I mentioned in the initial post.

See the diff in attached screenshot:

The 'next_run_time' doesn't matter when it's a one-time task, so it can be initialized outside of the condition.

And recalculating it using task.period only makes sense if task.period is bigger than 0.

Tom.