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

AsyncLock: public getter for _taken? #34

Closed
chrfin opened this issue Sep 21, 2015 · 2 comments
Closed

AsyncLock: public getter for _taken? #34

chrfin opened this issue Sep 21, 2015 · 2 comments
Assignees

Comments

@chrfin
Copy link

chrfin commented Sep 21, 2015

Would it be possible to add a public getter for _taken to the AsyncLock. I have a usecase where I have an asnyc lock in a method which should not execute a second time if it is already running, so the following would do the trick:

public async Task DoWorkAsync()
{
    if (asyncLock.IsTaken)
        return;

    using (await asyncLock.LockAsync().ConfigureAwait(false))
    {
         // do work
    }
}

In my case the possible race condition for the small gap between the check and the lock beeing taken does not matter as it is only a "performance penalty" if a second call gets to the lock, but maybe you could even implement some kind of TryLockAsync which would be thread-safe without a race condition...

@StephenCleary
Copy link
Owner

No; this would be introducing behavior into the AsyncLock class that it shouldn't have.

AsyncLock is a coordination primitives, and it is specifically for restricting access to one block of code at a time. This is all it should do.

Adding a Taken property would invite misuse; as you noted, there is an unavoidable race condition inherent in the property even existing.

There's also a more insidious race condition even with a TryLockAsync: the question of "what does 'already running' mean?" Presumably, your code under lock is doing something important, but the lock is still held for some amount of time after the important work is done.

using (await asyncLock.LockAsync())
{
   DoImportantWork();
   // Lock is still held here, after work is done.
}

As such, most of the time when there's a "skip this if it's already in progress" requirement, the requirement itself is incorrect. It can happen - it's just extremely rare. Usually, some kind of throttling (with at least one "waiting" to get in) is a more reliable solution.

I did consider adding both a TryLockAsync and Taken members, but have decided not to. AsyncLock as it currently stands does one thing extremely well; there are lots of other solutions if you need something else (in this case, Rx and/or async-compatible producer/consumer queues).

If you really do need the semantics you describe, then I recommend that you continue doing what your are probably doing now: keeping a separate boolean variable meaning "there's some other code already in this lock and I should skip processing even though there's a small chance that the processing may already be complete so this signal is lost". That way the AsyncLock does what it does well (mutual exclusion), and something else (the boolean) handles the unusual semantics.

@chrfin
Copy link
Author

chrfin commented Oct 4, 2015

Thanks for your extensive feedback and you are right in everything you said - just haven't thought of everything yet. Especially using some kind of "throtteling" - I just started using Rx someplace else in my application, but I didn't think about using it here until now...
I'll think about it again and I am fairly convident now that it can be solved better using either Rx or maybe even with one or two simple batching-block from TPL data-flow, as basically my case is (simplified) "send a batch of data now, if it is not already sending a batch" - the reason I did not use that until now is that I can not send a batch of data immediatly as soon as it is ready, but need to wait until my HW is ready to receive the next batch - and that is the case where I would have used the Taken property, because I do not need to try sending another batch if the "old one" is still waiting for the HW and a "one time miss" because of the race condition wouldn't have mattered, because after all data is ready I loop a "wait some time and try sending a batch" until everything is sent...

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

No branches or pull requests

2 participants