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.

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

Refactor distributed lock, such that they work with async code #41

Merged
merged 5 commits into from
Jun 24, 2023

Conversation

Spacefish
Copy link
Contributor

  • Changed the locks from Monitor to SemaphoreSlim which works in async contexts, where the lock is acquired by one thread and released by another
  • Moved the lock handling to the MemoryStorageConnection class to avoid spreading state over multiple types
  • Added an Disposable Utillity class to have a quickhand for returning disposables

@perrich
Copy link
Owner

perrich commented Jun 23, 2023

Thank you, I'll try to check and merge your PR tomorrow.

@perrich perrich merged commit 151ebf0 into perrich:master Jun 24, 2023
@perrich
Copy link
Owner

perrich commented Jun 24, 2023

Thanks, seems to work well (not reproduced the sync issue from #31)

@Spacefish Spacefish deleted the fix-async-jobs branch June 25, 2023 12:21
@EvheniyHlushko
Copy link

`Hangfire.Storage.DistributedLockTimeoutException
Timeout expired. The timeout elapsed prior to obtaining a distributed lock on the 'lock:recurring-job:Test' resource.
   at Hangfire.MemoryStorage.MemoryStorageConnection.AcquireDistributedLock(String resource, TimeSpan timeout)
   at Hangfire.RecurringJobExtensions.AcquireDistributedRecurringJobLock(IStorageConnection connection, String recurringJobId, TimeSpan timeout)
   at Hangfire.RecurringJobManager.AddOrUpdate(String recurringJobId, Job job, String cronExpression, RecurringJobOptions options)
   at Hangfire.RecurringJobManagerExtensions.AddOrUpdate[T](IRecurringJobManager manager, String recurringJobId, Expression`1 methodCall, String cronExpression, RecurringJobOptions options)
   at Hangfire.RecurringJobManagerExtensions.AddOrUpdate[T](IRecurringJobManager manager, String recurringJobId, Expression`1 methodCall, Func`1 cronExpression, RecurringJobOptions options)`

Looks like after these changes on version 1.8 encountering test failures when run in parallel. Everything works in the previous version, 1.7.0.

@perrich
Copy link
Owner

perrich commented Jul 29, 2023

Seems that you try to execute same named job in the same time. First is not finished and second request is in timeout.
I don't think it's a bug.

@EvheniyHlushko
Copy link

Right. I'm using WebApplicationFactory for tests. In Strartup each instance adds a background job. Probably it's because Data are stored in a static way

@Spacefish
Copy link
Contributor Author

Spacefish commented Jul 30, 2023

Right. I'm using WebApplicationFactory for tests. In Strartup each instance adds a background job. Probably it's because Data are stored in a static way

yes it uses static variables to store a lot of internal state.. So no separation between different instances.. I tried to make them independent while fixing this synchronization issue, but it would have introduced too many changes / i wasn´t sure if this breaks something else.. The MemoryStorageConnection class is public after all..

Furthemore Hangfire itself uses static classes / advertises the use of static class methods to submit jobs..

I did it now and created a PR:
#43

@EvheniyHlushko
Copy link

EvheniyHlushko commented Jul 31, 2023

Built a package based on your PR, still encountering the same issue.

Furthemore Hangfire itself uses static classes / advertises the use of static class methods to submit jobs..

Most likely it's the case
static JobStorage.Current passed to RecurringJobManager constructor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants