Skip to content

Avoid spawning the same scheduled task multiple times#252

Merged
attzonko merged 5 commits into
attzonko:mainfrom
unode:fix_schedule
Aug 29, 2021
Merged

Avoid spawning the same scheduled task multiple times#252
attzonko merged 5 commits into
attzonko:mainfrom
unode:fix_schedule

Conversation

@unode

@unode unode commented Aug 19, 2021

Copy link
Copy Markdown
Collaborator

TLDR: This is not a complete solution and currently two of the tests are not fixable. Relates to #231 .

Currently scheduled tasks are repeatedly executed causing all kinds of unexpected behavior.

2021-08-17 19:24:53.760 | Thread-1 | mmpy.threadpool - INFO: Scheduler thread started.
(...)
2021-08-17 20:24:54.487 | Thread-13 | Bot.plugins.admin - INFO: Executing scheduled admin tasks
2021-08-17 20:24:55.488 | Thread-14 | Bot.plugins.admin - INFO: Executing scheduled admin tasks
2021-08-17 20:24:56.490 | Thread-15 | Bot.plugins.admin - INFO: Executing scheduled admin tasks
2021-08-17 20:24:56.677 | Thread-13 | Bot.plugins.admin - INFO: Done executing scheduled admin tasks
2021-08-17 20:24:57.698 | Thread-14 | Bot.plugins.admin - INFO: Done executing scheduled admin tasks
2021-08-17 20:24:58.621 | Thread-15 | Bot.plugins.admin - INFO: Done executing scheduled admin tasks
2021-08-17 21:24:59.072 | Thread-16 | Bot.plugins.admin - INFO: Executing scheduled admin tasks
2021-08-17 21:25:00.073 | Thread-17 | Bot.plugins.admin - INFO: Executing scheduled admin tasks
2021-08-17 21:25:01.075 | Thread-18 | Bot.plugins.admin - INFO: Executing scheduled admin tasks
2021-08-17 21:25:01.361 | Thread-16 | Bot.plugins.admin - INFO: Done executing scheduled admin tasks
2021-08-17 21:25:02.236 | Thread-17 | Bot.plugins.admin - INFO: Done executing scheduled admin tasks
2021-08-17 21:25:03.425 | Thread-18 | Bot.plugins.admin - INFO: Done executing scheduled admin tasks

Above we can see two iterations of schedule.every(60).seconds.do(...) for a task that takes roughly 2.2 seconds to complete. Given the schedule loop is pre-defined to 1 second, the task is launched 3 times, the first at second 0, and the last at second 2. At second 3 the task is marked complete and is not relaunched.
In the above log admin task should twice, once every minute, but instead ends up running 6 times.

We have two possible solutions here:

  1. Revert to mmpy_bot 1.0 behavior by running scheduled tasks sequentially in a dedicated thread.
  2. Submit a patch to https://github.com/dbader/schedule that prevents tasks from being relaunched if already busy.

The current PR implements 1. but renders the schedule.do().tag("process") approach useless since the schedule thread (Thread-1) still blocks. The main thread (MainThread) runs normally.

The two schedule tests that verify if tasks can run in parallel are therefore marked as skipped for the time being.

unode added 5 commits August 19, 2021 18:39
This reverts commit d4f78c5.

The same bug exists not only in schedule.once() but also
schedule.every() so a better fix is necessary.
The threadpool already has a dedicated thread to run scheduler tasks
which is not the same as running in the main thread.
This forces code to run sequentially on the scheduler thread.

fixes attzonko#231 (again)
@qlty-cloud-legacy

Copy link
Copy Markdown

Code Climate has analyzed commit e393992 and detected 0 issues on this pull request.

View more on Code Climate.

@unode

unode commented Aug 19, 2021

Copy link
Copy Markdown
Collaborator Author

After looking at the schedule repo, there are a few issues/PRs proposing support for async/threaded execution but this doesn't seem to be a feature maintainers are interested in.

See:

I propose merging the current PR for sequential execution until a better solution is found.

@jneeven jneeven left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get rid of this schedule library altogether. I don't like how it's structured under the hood anyway. Not necessarily related to this repo, but I think it would be nice to create a subclass of concurrent.futures.Executor that supports scheduling tasks. It could use the python sched library under the hood. Might be something for https://github.com/jneeven/practipy, but I have never needed to schedule tasks from Python so I probably won't be working on it the coming year 😅

@unode

unode commented Aug 20, 2021

Copy link
Copy Markdown
Collaborator Author

I'm using scheduled tasks extensively, and will use more in the near future, so if we want to replace it we need a compatible candidate.

Anything that supports cronjob-like execution and one-time-tasks would fit for me. I haven't yet run into a bottleneck with too many active tasks.
I don't currently need a precise time execution but I could see others needing it. Most of my use-cases are "run this every X amount of minutes or once at a specific time of the day. +/- a minute or two doesn't make a difference as long as they are executed.

@unode

unode commented Aug 20, 2021

Copy link
Copy Markdown
Collaborator Author

I think sched is a reasonable candidate but we'll have to implement all the task juggling and semaphores ourselves. Something that could live as an external lib.

@attzonko

attzonko commented Aug 29, 2021

Copy link
Copy Markdown
Owner

Looking back at the discussion, I agree that option 2 would have been ideal if the other project was willing to change. Since that seems like a dead-end, I am inclined to merge this change in as is, and if future improvements are needed (i.e. change to using native sched module) we can address them in the future 😄

@attzonko attzonko merged commit 8a0eb7b into attzonko:main Aug 29, 2021
@unode unode deleted the fix_schedule branch August 30, 2021 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants