Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

jobs: fix job 'running state' #1927

Merged
merged 1 commit into from
Aug 25, 2020
Merged

jobs: fix job 'running state' #1927

merged 1 commit into from
Aug 25, 2020

Conversation

Exirel
Copy link
Contributor

@Exirel Exirel commented Aug 22, 2020 •
edited
Loading

Description

For some reason it was broken? I don't understand what happened here. I'll probably have to investigate further to understand the "why did I do that?" later. That will do at the moment, so no worries.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@Exirel Exirel added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Aug 22, 2020
@Exirel Exirel added this to the 7.1.0 milestone Aug 22, 2020
@dgw
Copy link
Member

dgw commented Aug 22, 2020

Reading the docstring gives a clue. It says something about only setting/clearing the running flag if not already set by the caller.

Also, looks like I gotta kick Travis again later. Yay. /s

@Exirel
Copy link
Contributor Author

Exirel commented Aug 22, 2020

Reading the docstring gives a clue.

Yeah but it's not used really because when the job is a thread the "is running" flag is set, so it's not clear inside the execute function. Which is probably the real bug here.

But heh. I'll fix the docstring, that will probably kick Travis.

@dgw
Copy link
Member

dgw commented Aug 24, 2020

@Exirel I came back to properly review this but the docstring fix isn't done yet. :S

Guess the real question is: Is a threaded job always considered is_running even if it's, um, idle? Not sure I understand the intended behavior or use of the flag, then. 😕

@Exirel
Copy link
Contributor Author

Exirel commented Aug 24, 2020

@dgw the goal is to prevent such job to run multiple time in parallel. Like, you have a job and you want it in a thread because it takes, like, 30s. It's fine, since it' should run only every 5min, but then you have a very weird case where it takes more than 5 mins, and "oh no" you don't want it to run again.

I think I wanted to be "too smart" with the with job: job.execute() thing, and it didn't come out as intended initially.

In short: sloppy job on my part. Will fix the docstring asap. Soon. This week.

@Exirel Exirel force-pushed the fix-job-scheduler branch from a6fa7d5 to 1b9869d Compare August 25, 2020 10:09
@Exirel
jobs: fix job 'running state'
82dcbe0 
So, the problem was: job's "running" state was never clear. So of
course, they would execute once, and never again. I did some sloppy job
at tracking down how this state was set and clear, and messed up what
was responsible for what.

Now:

* the job scheduler still set `job.is_running` before running it in a
  thread
* the `job.execute` method doesn't handle the running state anymore
* the job scheduler `_call` wrapper now uses `with job` to manage the
  job's state

This should work. This time.
@Exirel Exirel force-pushed the fix-job-scheduler branch from 1b9869d to 82dcbe0 Compare August 25, 2020 10:10
@Exirel
Copy link
Contributor Author

Exirel commented Aug 25, 2020

OK fixed it.

Quick note about KeyboardInterrupt: I checked, and this exception is raised only in the main thread. So it can't happen in the job scheduler thread!

dgw
dgw approved these changes Aug 25, 2020
View reviewed changes
Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Let's get this onto master 🚢

@dgw dgw merged commit 06c9099 into sopel-irc:master Aug 25, 2020
@Exirel Exirel deleted the fix-job-scheduler branch May 1, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgw dgw dgw approved these changes

Assignees
No one assigned
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s)
Projects
None yet
Milestone
7.1.0
Development

Successfully merging this pull request may close these issues.
2 participants
@Exirel @dgw