Permalink
Browse files

this works with SQLServer and appears to be generally less database s…

…pecific and fragile
  • Loading branch information...
superchris committed Feb 27, 2013
1 parent a0d9807 commit b01561861c930797a7786f84c30cc462d6b01a96
Showing with 7 additions and 15 deletions.
  1. +7 −15 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.

1 comment on commit b015618

@aripollak

This comment has been minimized.

Show comment Hide comment
@aripollak

aripollak Mar 22, 2013

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

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
Please sign in to comment.