Is it safe to share local variable between threads (via a callback closure)?

Question

I want to do something like the following - basically I am calling an async operation which will call a callback in another thread and I want to wait for it to complete "inline". My worry is that that changes variables shared across threads (bar & event) may not be synchronized due to being stored in registers for example. If they were member variables I could mark them volatile but volatile can’t be used on local variables created on the stack. I could use member variables but I think its cleaner not clutter my class by keeping it all local.

Bar bar = null;
ManualResetEvent event = new ManualResetEvent(false);

foo.AsyncOperation(new Action(()=>{    
    // This delegate will be called in another thread
    bar = ...
    event.Set();
}));

event.WaitOne(timeout);
// use bar

Note: unless you do something else between `AsyncOperation` and `WaitOne`, you might as well run it synchronously — Marc Gravell, Sep 02 '11 at 11:11
This question is related: http://stackoverflow.com/questions/6581848/memory-barrier-generators/6932271#6932271 — Brian Gideon, Sep 02 '11 at 14:57
@Marc - Good point but the API I am calling inherently asynchronous. Its based on messages sent over the network to a server. Within AsyncOperaton a message is set and the call back is notified when there is a reply. I use it asynchronously most of the time, but in this particular case I want to wait for the result . I include a time out in WaitOne in case the reply doesn’t come. — Shane, Sep 03 '11 at 01:04
@Shane cool - in that case the usage is fine; you might, however, be surprised how many times I've seen people do an async/join (immediately) inappropriately. — Marc Gravell, Sep 03 '11 at 06:43

xanatos · Accepted Answer · 2011-09-02T11:40:57.137

6

Yes, it will work correctly. Read here

http://www.albahari.com/threading/part4.aspx

The following implicitly generate full fences: Setting and waiting on a signaling construct

and in the signaling constructs the ManualResetEvent is included.

If you want to know what a full fence is, in the same page:

Full fences The simplest kind of memory barrier is a full memory barrier (full fence) which prevents any kind of instruction reordering or caching around that fence. Calling Thread.MemoryBarrier generates a full fence;

edited Sep 02 '11 at 11:40

answered Sep 02 '11 at 11:26

xanatos

109,618
12
197
280

Cool; if the `WaitOne` acts as a fence, it is safe. I do, however, with that MSDN would document this http://msdn.microsoft.com/en-us/library/58195swd.aspx – Marc Gravell Sep 02 '11 at 11:33
2

@Marc The msdn documentation about thread safety and fences is quite lacking, if I remember correctly, BUT http://msdn.microsoft.com/en-us/library/ms686355(v=vs.85).aspx `The following synchronization functions use the appropriate barriers to ensure memory ordering:` `Wait functions` and `Functions that signal synchronization objects` – xanatos Sep 02 '11 at 11:36
sorted, then; I'll remove my answer – Marc Gravell Sep 02 '11 at 11:39
@Xantos - Haven't had time to dig into this but I think you reasoning makes sense. So what are the fences here - the event constructor, Set and Wait? I am thinking reasoning based on lock free contructs is too tricky and that the intention of the code would be clearer if I lock while updating and reading bar in the two threads. The event object should make a suitable lock. I guess the closure capture mechanisim ensures the event would be seen as initialized in both threads? – Shane Sep 05 '11 at 14:57
@Shane I would say the `Set`, the `Wait`. I'm not sure the creation of the `ManualResetEvent` implicitly creates the memory barrier. It's more probably a "hard ordering" case (it's always created before it can be referenced in "code order" (so if it's created in line 1 and it's referenced in line 2, the two lines can't be swapped)). You could add a "useless" `lock` to make everything clearer, if you want. Clearly if you select the MRE as the "target" of the lock, both threads will see it. Lock free constructs gives migraine :-) – xanatos Sep 05 '11 at 15:12
@Shane The reason why the constructor of the MRE and the access to the MRE can't be reordered are: A) Otherwhise the `MRE.Set()` could fail with a NULL exception :-) It would be a *classical bug* and B) http://www.bluebytesoftware.com/blog/PermaLink,guid,efdf6840-c071-4c5b-addf-9c3205de7f1d.aspx `Rule 1: Data dependence among loads and stores is never violated.` The `load` is accessing the `event` to do the `Set` or `Wait`, the `Store` is setting the `event` variable with the result of the constuction of the MRE. This clearly is in "code order". – xanatos Sep 05 '11 at 15:21

Random Dev · Answer 2 · 2011-09-02T11:42:54.117

4

I think your code will work - the closure will lift then into the heap even if they were just stack-variables (the ManualReseetEvent will surely not be).

But why don't you put everything after event.WaitOne() just inside the continuation (the block were event.Set is called)? I think this should be the prefered way to handle this kind of situation and you won't run into trouble this way (you don't need the Bar in the outer block at all, and you can still use the MRE to check).

I would consider turning this into Operations using Task objects - this would solve all of this in one go (for example return a Task from your AsyncOperation). You can then wait for the result of the Task and use the returned Bar ...

class Foo
{ 
 // ...
 private Task<Bar> AsyncOperation(Task<Bar> initializeBar)
 {
   return initializeBar
          .ContinueWith(
            bar => { 
                     /* do your work with bar and return some new or same bar */ 
                     return bar;
                   });
 }
}

and use it like this:

var task = foo.AsyncOperation(Taks.Factory.StartNew(() => { /* create and return a bar */ }));
var theBar = task.Result; // <- this will wait for the task to finish
// use your bar

PS: The closure will basically wrap them just into a class-object ;) PPS: it's hard for me to test this code without your AsyncOperation but it should work modulo syntax errors by wrong spelling/typing I made

edited Sep 02 '11 at 11:42

answered Sep 02 '11 at 11:06

Random Dev

51,810
9
92
119

I'm not so sure - how is it *guaranteed* to work? the usual data/ordering rules only apply when single-threaded... – Marc Gravell Sep 02 '11 at 11:13
obviously you know a lot more about those stuff than I do - the question was if this code is put into some kind of register (and I assumed) to be reused/recreated in the other thread) and I think *this* will surely not happen. And I think I saw something very similar uesed in the async stuff F# uses and honestly: I haved use similiar stuff (using ManualResetEvents - locally defined to synchronise threads) quiete a lot and never run into trouble. But I'm no concurency-Wizzard - maybe you can point where the problem lies exactly instead of hinting at it? (no offens - really interested) – Random Dev Sep 02 '11 at 11:18
oh wait - you (of course) are thinking about the Bar - object I guess ... hmm - that might indeed spell trouble... – Random Dev Sep 02 '11 at 11:22
There is a ManualResetEvent.Set that doubles as MemoryBarrier, if I remember correctly – xanatos Sep 02 '11 at 11:24
@xanatos if the WaitOne() also functions as a memory-barrier, that would do it; the memory-barrier on the Set would not impact the code on the other thread (the one we care about) – Marc Gravell Sep 02 '11 at 11:31

Is it safe to share local variable between threads (via a callback closure)?

2 Answers2