search

marcoCasamento / Hangfire.Redis.StackExchange

HangFire Redis storage based on original (and now unsupported) Hangfire.Redis but using lovely StackExchange.Redis client
Other
456 stars 109 forks source link

Redis Lock fix #17

Closed palomo2 closed 8 years ago

palomo2 commented 8 years ago

Redis lock owner refers to ManagedThreadId

send2vinnie commented 8 years ago

Considering the clustered Hangfire server topology (e.g. 2 hangfire processes monitor same job queues), the Redis lock owner should refer to Storage Id + Thread Id to make sure the distributed lock work properly.

marcoCasamento commented 8 years ago

@send2vinnie I agree, it should. It seems however, that the @palomo2 suggested changes causes problems with batch continuations. I'd need some integration tests on this but the Batch assemblies are part of the Pro and this complicate things. Do you have any problem with the actual release (without the ThreadId) ?

marcoCasamento commented 8 years ago

@palomo2 I also think it could worth to repeat the failed batch test again on a clean environment. Do you have any setup for this ?

send2vinnie commented 8 years ago

@marcoCasamento I got multiple Hangfire processes monitoring the same queues so I decided to use what @palomo2 was committed after reviewing the whole changes to the lock owner. I just found the RedisLock in the actual release would not be thread safe while different threads hold the same lock owner (Storage Id). (instead, if won't be a problem if FetchedJobsWatcher is used by a dedicated thread)

marcoCasamento commented 8 years ago

Ok I've "conceptually" merged the @palomo2 PR and added the check @send2vinnie suggested. I've also done some integration tests on 100% cpu and 13 different servers contending the jobs and it worked smoothly both for regular jobs and batch jobs. Thanks all guys