2

I'm implementing the BackgroundWorker replacement for some reason, and I have to implement following public properties:

public bool CancellationPending { get; private set; }
public bool IsBusy { get; private set; }
public bool WorkerReportsProgress { get; set; }
public bool WorkerSupportsCancellation { get; set; }

I'm sure you know what purpose they serve in BackgroundWorker. So they might be accessed/modified by different threads. I'm concerned about how to "protect" them for multithreading. I thought declaring them as volatile would be enough, but volatile can't be applied to automatic properties.

What should I do? Should I create private fields for these properties, and declare them volatile? Or should I use locking inside each get and set blocks?

I think this should be pretty common scenario - making properties (preferably automatic properties) thread-safe. Note that all properties are of atomic type in this example.

EDIT:

To clarify what I need: I need to be sure that all threads always read the most up-to-date value of the property. See this: https://stackoverflow.com/a/10797326/1081467

So again, do you advice using volatile, or locking, or anything else?.. When using the bool properties atomicity is guaranteed, so only the second problem is left (reading the up-to-date values), so how do you solve this correctly? What about when you have properties of non-primitive types? Do you put locks in each get and set blocks?

Community
  • 1
  • 1
TX_
  • 5,056
  • 3
  • 28
  • 40
  • You can't use the `BackgroundWorker` class? – Mike Perrenoud Sep 11 '12 at 15:37
  • 1
    use `locki`ng inside each `get` and `set` block. Alternatively, make your types immutable. For instance, `CancellationPending` is not likely to have an issue. – Oded Sep 11 '12 at 15:38
  • 1
    What exactly do you mean by `thread safe` in this context? – asawyer Sep 11 '12 at 15:38
  • It seems like at least `IsBusy` should be an event so that you can wait on it. Maybe the rest can use `Interlocked.CompareExchange` – parapura rajkumar Sep 11 '12 at 15:40
  • I can't use `BackgroundWorker` because it uses MTA model and I need STA to access third-party COM object (I know framework will do the required marshalling anyway if needed but I don't want that behavior for some reason). – TX_ Sep 11 '12 at 15:41
  • 2
    @TX_ Why don't you use `Task`s instead, along with [STA scheduler](http://blogs.msdn.com/b/pfxteam/archive/2010/04/07/9990421.aspx)? – svick Sep 11 '12 at 15:43
  • Possible duplicate of http://stackoverflow.com/questions/2074670/are-c-sharp-auto-implemented-static-properties-thread-safe. Default implementation for auto props is not thread-safe. Eric Lippert has a nice spec-based comment in this thread about that :) Ultimately, you need locking primitives. – David W Sep 11 '12 at 15:47
  • @svick, I'd like to use `Task`s but in this particular case I have to replace BackgroundWorker "seamlessly". Great suggestion though. – TX_ Sep 12 '12 at 08:25
  • Re the Edit: the problem in Tudors answer is not in the property but in the while loop. Note that it is not a problem in practice but only with such a very tight loop. Solve it inside the loop, eg with `Thread.MemoryBarrier();` – H H Sep 12 '12 at 17:04

4 Answers4

6

I came up with the following implementation. Please comment whether you think it is an optimal solution:

//========== Public properties ==================================================//

public bool CancellationPending { get { return _cancellationPending; } private set { _cancellationPending = value; } }

public bool IsBusy { get { return _isBusy; } private set { _isBusy = value; } }

public bool WorkerReportsProgress { get { return _workerReportsProgress; } set { _workerReportsProgress = value; } }

public bool WorkerSupportsCancellation { get { return _workerSupportsCancellation; } set { _workerSupportsCancellation = value; } }

//========== Private fields ==================================================//

private volatile bool _cancellationPending;
private volatile bool _isBusy;
private volatile bool _workerReportsProgress;
private volatile bool _workerSupportsCancellation;

Reasoning: atomicity is ensured by the fact that the fields are of type bool, so no need for locking. Making them volatile will ensure that any thread will read current value - not cached - in case another thread has modified it. I think this is the exact purpose (and only valid use) of volatile keyword, right?

TX_
  • 5,056
  • 3
  • 28
  • 40
  • Volatility can't be defined for properties. Underlying fields are what matters. – TX_ Oct 03 '12 at 10:22
  • The property shields the effect of volatile. Try to create a case where removing volatile makes a difference. – H H Oct 03 '12 at 10:48
  • 1
    `class Program { static bool _stop = false; static bool stop { get { return _stop; } set { _stop = value; } } public static void Main(string[] args) { new Thread(() => { Console.WriteLine("START"); bool dummy = false; while (!stop) dummy = !dummy; Console.WriteLine("END"); }).Start(); Thread.Sleep(1000); stop = true; Console.WriteLine("stop = true"); Console.WriteLine("WAITING..."); } }` – TX_ Oct 03 '12 at 12:34
  • 1
    Copy and paste it, then build the `release` build, and run without debugger. It will hang. Then change `static bool _stop = false;` with `static volatile bool _stop = false;`, do the same, and you'll see - it will no longer hang (as is desired). Now please undo your vote-down. – TX_ Oct 03 '12 at 12:37
  • Big waste of time. It doesn't hang w/o volatile. – H H Oct 03 '12 at 12:51
  • 4
    That's funny, because it is you who is wasting my time, bro. Even though I was 100% sure, your words made me suspicious and I decided to test once again. I wrote that small code that demonstrates the problem clearly. I'm using VS2012, though I have tested with VS2010 and the behavior was exactly same. Create new empty project in Visual C#. Copy/paste the code. Build the `release` build, this is important! And then run it **outside the debugger**, this is important as well! You will see that original code will hang after writing "WAITING...", and if you make `_stop` field volatile it will work. – TX_ Oct 03 '12 at 13:02
  • 2
    TX is right, the code hangs. I got the same exact issue on production where I relied on a bool property to make the processing loop stop. Now I switched completely to CancellationTokenSource, takes a lot of implementation details out of implementing a reliable cancellation. – Igor Malin Oct 26 '15 at 21:12
3
public bool CancellationPending { get; private set; }
public bool IsBusy { get; private set; }
public bool WorkerReportsProgress { get; set; }
public bool WorkerSupportsCancellation { get; set; }

So they might be accessed/modified by different threads

No, that would only apply to CancellationPending and IsBusy, not to the others.
And they are all booleans, guaranteed to be atomic. Atomicity is enough here.

All of the properties of the original Backgroundworker are documented as not thread-safe.
See near the bottom of this page.

H H
  • 263,252
  • 30
  • 330
  • 514
  • Did that. Again, you probably shouldn't do anything. See comment above. – H H Sep 12 '12 at 17:04
  • 1
    Even if they're atomic, marking the backing fields as `volatile` is necessary to prevent compiler optimizations which assume access from a single thread. – Mateen Ulhaq Feb 17 '16 at 02:31
  • @MateenUlhaq - you can't make properties volatile and you don't have to. The Bgw provides enough _fences_ to make this reliable. And see [this](http://blogs.msmvps.com/peterritchie/2012/01/25/avoid-the-volatile-modifier/) for example. – H H Feb 17 '16 at 06:16
1

Though I think that there are better options out there such as using Tasks with anothe scheduler as svick mentioned.

Should you want to continue on this path you should definitely use locking and not volatile fields as volatile does not do what you think. Oh and this guy said something about never making a volatile field...

Instead of volatile you can use your favourite synch primitive (lock, Mutex, Interlocked, ReaderWriterLockSlim, etc...) depending on the access characteristics.

Slugart
  • 4,535
  • 24
  • 32
0

A simple way is to have mutex objects for each property that you want to be thread safe. In the get and set properties, use a Monitor.Enter(declaredObjectMutext), and Monitor.Exit(declaredObjectMutex). Once done, this will make your properties thread-safe (all calls to get and set will be blocking calls until any other threads are finished).

Another option is to use the Interlocked Class, which allows for thread-safe modification of integers, and bools. If thats all you're using with your properties, its an easy solution.

greggorob64
  • 2,487
  • 2
  • 27
  • 55