-
-
Notifications
You must be signed in to change notification settings - Fork 584
[12.0][FIX] under pressure #131
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,11 +13,7 @@ | |
| from odoo.service.model import PG_CONCURRENCY_ERRORS_TO_RETRY | ||
|
|
||
| from ..job import Job, ENQUEUED | ||
| from ..exception import (NoSuchJobError, | ||
| NotReadableJobError, | ||
| RetryableJobError, | ||
| FailedJobError, | ||
| NothingToDoJob) | ||
| from ..exception import (RetryableJobError, FailedJobError, NothingToDoJob) | ||
|
|
||
| _logger = logging.getLogger(__name__) | ||
|
|
||
|
|
@@ -26,42 +22,17 @@ | |
|
|
||
| class RunJobController(http.Controller): | ||
|
|
||
| def _load_job(self, env, job_uuid): | ||
| """Reload a job from the backend""" | ||
| try: | ||
| job = Job.load(env, job_uuid) | ||
| except NoSuchJobError: | ||
| # just skip it | ||
| job = None | ||
| except NotReadableJobError: | ||
| _logger.exception('Could not read job: %s', job_uuid) | ||
| raise | ||
| return job | ||
|
|
||
| def _try_perform_job(self, env, job): | ||
| """Try to perform the job.""" | ||
|
|
||
| # if the job has been manually set to DONE or PENDING, | ||
| # or if something tries to run a job that is not enqueued | ||
| # before its execution, stop | ||
| if job.state != ENQUEUED: | ||
| _logger.warning('job %s is in state %s ' | ||
| 'instead of enqueued in /runjob', | ||
| job.uuid, job.state) | ||
| return | ||
|
|
||
| # TODO: set_started should be done atomically with | ||
| # update queue_job set=state=started | ||
| # where state=enqueid and id= | ||
| job.set_started() | ||
| job.store() | ||
| http.request.env.cr.commit() | ||
|
|
||
| env.cr.commit() | ||
| _logger.debug('%s started', job) | ||
|
|
||
| job.perform() | ||
| job.set_done() | ||
| job.store() | ||
| http.request.env.cr.commit() | ||
| env.cr.commit() | ||
| _logger.debug('%s done', job) | ||
|
|
||
| @http.route('/queue_job/session', type='http', auth="none") | ||
|
|
@@ -87,10 +58,23 @@ def retry_postpone(job, message, seconds=None): | |
| job.store() | ||
| env.cr.commit() | ||
|
|
||
| job = self._load_job(env, job_uuid) | ||
| if job is None: | ||
| # ensure the job to run is in the correct state and lock the record | ||
| env.cr.execute( | ||
| "SELECT state FROM queue_job " | ||
| "WHERE uuid=%s AND state=%s " | ||
| "FOR UPDATE", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to use NOWAIT and catch concurrent errors, to avoid locking the second worker?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @guewen I thought about that and opted for the simpler code. My thinking is the benefit is marginal (a nicer log in rare situations) for a more complex code. The lock will never be held long (ie until the job is started immediately after).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't realize it was only until it starts, better to keep it simple yes :) |
||
| (job_uuid, ENQUEUED) | ||
| ) | ||
| if not env.cr.fetchone(): | ||
| _logger.warn( | ||
| "was requested to run job %s, but it does not exist, " | ||
| "or is not in state %s", | ||
| job_uuid, ENQUEUED | ||
| ) | ||
| return "" | ||
| env.cr.commit() | ||
|
|
||
| job = Job.load(env, job_uuid) | ||
| assert job and job.state == ENQUEUED | ||
|
|
||
| try: | ||
| try: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where the race condition occured: two simultaneous runs of the same job both reach this point in enqueued state.