Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Use more generic (and SQL Server compatible) locking in reserve #38

Merged
merged 1 commit into from

4 participants

@superchris

The code in reserve seemed unnecessarily DB specific and fragile, replaced it to use AR lock method instead. The motivation was SQL server compatiblity

@sferik
Owner

What’s the reason not to use the PostgreSQL-optimized code for the PostgreSQL case? I’d be okay with your implementation being the else case. This code was added in #29 as a PostgreSQL-specific performance optimization. I’d hate to rip it out and have a degradation of performance for our PostgreSQL users. However, I would also like to support Microsoft SQL Server. I believe both of these goals can be achieved with a simple conditional statement.

@superchris
@sferik
Owner

Okay, I'm convinced.

Thanks for the patch.

@sferik sferik merged commit a297748 into from
@jnimety

I'm still investigating but since I rolled from 0.4.2 to 0.4.3 in production some of my jobs are running multiple times. Email headers show that duplicate emails are originating from different workers/servers. Seems like locking is broken. I'm using mysql. /cc #40

@aripollak

I think this still contains a race condition - two workers can get to the with_lock at the same time with the same job, and they'll both update the same job & return it in sequence, so the job would get processed twice. Since with_lock reloads the job with the lock, I think this can be fixed by just putting this after the with_lock:

return if job.locked_at || job.locked_by
@albus522 albus522 referenced this pull request
Merged

Fix job locking #46

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 27, 2013
  1. @superchris
This page is out of date. Refresh to see the latest.
Showing with 7 additions and 15 deletions.
  1. +7 −15 lib/delayed/backend/active_record.rb
View
22 lib/delayed/backend/active_record.rb
@@ -52,22 +52,14 @@ def self.reserve(worker, max_run_time = Worker.max_run_time)
nextScope = nextScope.scoped.by_priority.limit(1)
now = self.db_time_now
-
- if ::ActiveRecord::Base.connection.adapter_name == "PostgreSQL"
- # Custom SQL required for PostgreSQL because postgres does not support UPDATE...LIMIT
- # This locks the single record 'FOR UPDATE' in the subquery (http://www.postgresql.org/docs/9.0/static/sql-select.html#SQL-FOR-UPDATE-SHARE)
- # Note: active_record would attempt to generate UPDATE...LIMIT like sql for postgres if we use a .limit() filter, but it would not use
- # 'FOR UPDATE' and we would have many locking conflicts
- quotedTableName = ::ActiveRecord::Base.connection.quote_table_name(self.table_name)
- subquerySql = nextScope.lock(true).select('id').to_sql
- reserved = self.find_by_sql(["UPDATE #{quotedTableName} SET locked_at = ?, locked_by = ? WHERE id IN (#{subquerySql}) RETURNING *",now,worker.name])
- return reserved[0]
- else
- # This works on MySQL and other DBs that support UPDATE...LIMIT. It uses separate queries to lock and return the job
- count = nextScope.update_all(:locked_at => now, :locked_by => worker.name)
- return nil if count == 0
- return self.where(:locked_at => now, :locked_by => worker.name).first
+ job = nextScope.first
+ return unless job
+ job.with_lock do
+ job.locked_at = now
+ job.locked_by = worker.name
+ job.save!
end
+ job
end
# Lock this job for this worker.
Something went wrong with that request. Please try again.