2

Put simply, I have something like this:

class MyClass
{
    double SomeProperty {get; private set;}

    public async Task SomeMethod()
    {
        SomeProperty = await someService.SomeAsyncMethod();
    }
}

Could SomeProperty get corrupted if I call SomeMethod() many times concurrently ?

i3arnon
  • 113,022
  • 33
  • 324
  • 344
Ibrahim Najjar
  • 19,178
  • 4
  • 69
  • 95
  • It would be exactly the same if you just retrieved your `double` from somewhere else. – zerkms Dec 19 '14 at 00:29
  • Yes, it could get corrupted if you're calling `SomeMethod()` many times concurrently on the same MyClass instance. – gknicker Dec 19 '14 at 00:31
  • @zerkms So it could get corrupted right ? – Ibrahim Najjar Dec 19 '14 at 00:31
  • @Sniffer: that's right. `await` does not add any new semantics here. – zerkms Dec 19 '14 at 00:34
  • see also: [is-reading-a-double-not-thread-safe](http://stackoverflow.com/questions/3676808/is-reading-a-double-not-thread-safe?lq=1) applies to setting a double as well – DrKoch Dec 19 '14 at 04:07
  • 2
    The fact that you're asking the question indicates that you are considering doing something dangerous as a result of a likely bad decision at the architectural level. **Design your application so that you do not share memory across threads**. Imagine that a thread was a instead process and you couldn't easily share memory; how would you design your application then? – Eric Lippert Dec 19 '14 at 16:26
  • @EricLippert Thanks for your input, greatly appreciated. I have no influence on the architecture nor the design of this. This is just something I was curious over. – Ibrahim Najjar Dec 19 '14 at 18:47

5 Answers5

6

Yes, potentially.

This isn't really related to async-await. You are simply setting a value to a property by multiple threads. The value can get corrupted and you should use some synchronization construct (i.e. lock):

public async Task SomeMethod()
{
    var temp = await someService.SomeAsyncMethod();
    lock (_lock)
    {
        SomeProperty = temp;
    }
}

Or an Interlocked.Exchange (using the backing field of the property):

public async Task SomeMethod()
{
    Interlocked.Exchange(ref _backingField, await someService.SomeAsyncMethod());
}

However, if this method runs in a GUI thread (or any other case with a single-threaded SynchronizationContext) then there's no chance of corruption since there's only a single thread handling that property.

i3arnon
  • 113,022
  • 33
  • 324
  • 344
3

Could SomeProperty get corrupted if I call SomeMethod() many times concurrently ?

Potentially, yes, though it depends on a lot of factors.

Writing a double is not guaranteed to be an atomic operation. Since you are calling this method concurrently, you could be writing the value concurrently, which could cause undefined behavior.

There are various mechanisms to handle this. Often, you only need to call the service once, in which case you can store the task directly:

Task<double> backing;
double SomeProperty { get { return backing.Result; } }

MyClass()       // in constructor
{
    backing = someService.SomeAsyncMethod(); // Note, no await here!
}

Otherwise, if you want multiple calls, an explicit lock or other synchronization mechanism is likely the best approach.

Reed Copsey
  • 554,122
  • 78
  • 1,158
  • 1,373
  • Although a different question. What's the best way to fix things in this situation ? – Ibrahim Najjar Dec 19 '14 at 00:34
  • @Sniffer what do you want to achieve? Assuming there are 2 concurrent threads assign, which one must win? – zerkms Dec 19 '14 at 00:35
  • Writing a double is guaranteed to be atomic on 64 bit machines though, isn't it? I recall that being mentioned on a similar question – cost Dec 19 '14 at 00:35
  • @zerkms I just want to assure atomic writes to `SomeProperty` because I learned it could get corrupted. – Ibrahim Najjar Dec 19 '14 at 00:36
  • 1
    Even if it's atomic, you'd need some kind of barrier. Just updating the value can cause inconsistent views from different threads. – Stephen Cleary Dec 19 '14 at 14:49
2

async and await do not add any concurrency protection. If it can get corrupted without async and await, it can get get corrupted with them as well.

Robert Harvey
  • 178,213
  • 47
  • 333
  • 501
1

Yes.

Because you're using await, the method gets called, grabs the Task from someService.SomeAsyncMethod(), and then yields control until said Task completes.

When that task does complete, control returns (normally on the same thread of the calling task, though this depends on SynchronizationContext as pointed out by @Reed) and then calls the set function of SomeProperty. At this point, said call could occur from multiple threads, or from a thread other than the one that started the tasks, resulting in unpredictable get/set behaviour without locks.

David
  • 10,458
  • 1
  • 28
  • 40
1

Reading and writing a double is not thread safe so it is best to use a lock around those. Using Interlocked.COmpareExchange will write the variable correctly but you need to read it in a thread safe manner as well, so basically you need a lock. See this answer from Eric Lippert.

Community
  • 1
  • 1
NeddySpaghetti
  • 13,187
  • 5
  • 32
  • 61