4

Given,

private object _x;

private object LoadAndSet(ref object x) {
   // lock established over read and update
   lock (somePrivateObjectNotUsedElsewhereThatIsIrrelvantToTheQuestion) {
       if (x == null)
           x = Load();
       return x;
   }
}

Invoked as,

public object Load() {
    return LoadAndSet(ref _x);
}
  • Are the Read / Writes to the Field (_x) "passed by reference" covered under the atomicity/visibility guarantees of the lock?

That is, is the first code equivalent to the following where the field is used directly? (The assignment occurs directly, instead of via a ref parameter.)

private object _x;

private object LoadAndSetFieldDirectly() {
   // lock established over read and update
   lock (somePrivateObjectNotUsedElsewhereThatIsIrrelvantToTheQuestion) {
       if (_x == null)
           _x = Load();
       return _x;
   }
}

public object Load() {
    return LoadAndSetFieldDirectly();
}

I suspect this to be true due to the use of ldind.ref and stind.ref in the method's MSIL; however, the question is begging authoritative documentation/information on the thread-safety (or lack of) when writing such ref code.

user2864740
  • 60,010
  • 15
  • 145
  • 220
  • 1
    Only if every other place that accesses _x equally locks on your 'this'. If they access it outside a lock or lock on some other object, then no. – Adam G Sep 18 '18 at 23:08
  • FYI: see the answer to https://stackoverflow.com/questions/251391/why-is-lockthis-bad. – jwdonahue Sep 18 '18 at 23:21
  • 1
    Perhaps you should contain your language and actually read what I wrote? I did not mention anything about why 'this' is a bad idea to lock upon. The problem I am pointing out is where someone locks on something else then accesses _x, or directly accesses _x without any lock whatsoever. – Adam G Sep 18 '18 at 23:59
  • 3
    I don't understand your second question. It's not legal to pass properties as `ref` in C#, so the question is basically "what would be different if something impossible happened?" That's not really a useful question to ask; impossible things are not observed to happen. – Eric Lippert Sep 19 '18 at 00:35
  • @EricLippert I've update the question for a non-ref baseline and removed the daft property bit.. – user2864740 Sep 19 '18 at 00:41
  • 3
    To not answer your question but rather to hopefully solve your problem: don't roll your own lazy initializer in the first place. Use `Lazy`. It was written by Joe Duffy; it is correct. – Eric Lippert Sep 19 '18 at 00:46
  • @EricLippert Very valid point, Lazy is used in most places :} Although a slight grievance with Lazy is does not [appear to] support call-loader-at-most-once-if-no-exception and don't-cache-exceptions together .. sometimes there are exceptions that ought not to be cached while only allowing a single loader invocation. Not saying this 'requirement' is the best design.. – user2864740 Sep 19 '18 at 00:48
  • " It was written by Joe Duffy; it is correct"... and very performant too! – Luca Cremonesi Sep 19 '18 at 14:52
  • @LucaCremonesi Regardless of performance, it still lacks a LazyThreadSafetyMode.ExecutionAndPublicationWithoutCapturingExceptions mode :} – user2864740 Sep 19 '18 at 16:34
  • @user2864740 - Would it be sufficient to create your own `LazyWithoutExceptionCache` type that uses a `Lazy` field and wraps the `valueFactory` you pass into it with a lambda that captures the exceptions and instead returns a default value? – John Rasch Sep 19 '18 at 21:20
  • @JohnRasch There are multiple alternative-Lazy implementations just for this situation (and we've actually such a type being used elsewhere). However, a wrapper to suppress the exception has different semantics - ie. imagine `remoteLazy = new Lazy(MakeExternalWebAPICall)`. If the first call fails (which it does occasionally due to "random network errors" eg), an exception *should* be thrown and will in turn 'fail' the calling context. However, desired outcome is that the next call to `remoteLazy`, ie. in a new calling context, will re-execute the value factory (and hopefully succeed!). – user2864740 Sep 19 '18 at 23:39
  • @JohnRasch Now, granted this isn't an "ideal" usage of Lazy, given all the other possibilities (eg. ConcurrentDictionary, and whatever other caching strategies) but the use of Lazy-wrapping-external-resource-calls in our [non-trivial code base] has led to several times of 'needing to force-restart services'. Yet, in most of these cases if the exception (or a dummy value) was *not* captured for the duration of Lazy, the problem would have 'fixed itself' in the next web-request or rabbitmq message processed.. – user2864740 Sep 19 '18 at 23:42
  • @JohnRasch Large parts of the code should 'not have been written the way it was', .. and many years of patchwork development on many lines of code is hard to [quickly] offset :} – user2864740 Sep 19 '18 at 23:53

1 Answers1

5

The semantics of lock(lockObject) { statements } are:

  • No two distinct threads are ever in a region of locked code that was locked with the same lockObject instance. If one thread is in the region then it will either go into an infinite loop, complete normally, or throw. (Or, for advanced players, goes into a wait state via Monitor.Wait; this is not intended to be a tutorial on the correct use of the monitor object.) A second thread will not enter the protected region until control leaves the region.
  • Reads of variables in or after the lock will not be moved "backwards in time" to before the lock.
  • Writes of variables in or before the lock will not be moved "forwards in time" to after the lock.

(This is a quick informal summary; for exact details of what the C# specification guarantees about reordering of reads and writes, see the specification.)

Those semantics are enforced by the runtime regardless of whether the variables accessed in the lock body are fields, locals, normal formal parameters, ref/out formal parameters, array elements or pointer dereferences.

That said, three things make me nervous about your code.

First, it's unnecessary and suboptimal re-invention of an existing mechanism. If you want to implement lazy initialization, use Lazy<T>. Why roll your own and risk getting it wrong, when you can use code written by experts who already have eked out every bit of performance from it?

Second, you have to ensure that every use of the field is under a lock, not just its initialization. Passing a field as a ref parameter makes an alias for the field, and now you have made the job of verifying that you got every usage of the field under the lock harder.

Third, it seems like there is an opportunity for unnecessary contention here. What if two different fields are both initialized by this same code on two different threads? Now they are contending for the lock when they do not need to be.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • Thanks for the answer - and *then* suggestions/feedback. This question came about by going through some old DCL code and *pondering* different approaches. The "final" form of the above example/experiment would then be `return _x ?? LoadAndSet(ref _x);` which would happily pay any lock-contention 'until some future undetermined time as the field may contain an assigned object', similar to DCL but split into a loader and wrapper. – user2864740 Sep 19 '18 at 01:10
  • Normally we do use Lazy, but in this case (and a few other cases) there is a requirement that the loader function is only called once on success, and that exceptions are not captured. AFAIK, LazyThreadSafetyMode.PublicationOnly will call the loader 'many times, accepting the first value' and LazyThreadSafetyMode.ExecutionAndPublication 'captures exceptions'. – user2864740 Sep 19 '18 at 01:12
  • The reason for the initial question and to use `ref _x` in the above - as opposed to `return _x ?? LoadAndSetFieldDirectly();` - is that then the *only* usages of `_x` (and the loader invocation) are within the wrapper. – user2864740 Sep 19 '18 at 01:15