1

Today I encountered a weird issue in our multi-threaded .NET application.

I was writing a public static event accessor like so:

private static readonly object myLock = new object();
private static Action<MyType, Guid, Guid> _handler;
public static event Action<MyType, Guid, Guid> MyEvent
{
   add { lock (myLock) _handler += value; }
   remove { lock (myLock) _handler -= value; }
}

This piece of code lives in a a non static class.

The issue I face is that once I use the add accessor (at a rather early point of the execution of the application): MyClass.MyEvent += myMethod;, I get a null reference exception in the lock method call in the add accessor. Through debugging I found out that this is because, for some reason, the private static lock object is (still) null at the point the code reaches the lock statement.

This is baffling to me. My understanding is that all static members should be initialized at the point of the first access to the class? I later did realize, that the code seems to work if I move the lock object to the top of the class, meaning it gets initialized at an way earlier point. This works, but I would like to keep all relevant code in one place, so I don't like this solution.

FYI, the file I am working in has almost 5000 lines of code, and the snippet above is close to the end. I have no idea if that makes any difference though...

A theory of ours has been that the problem has to do with the fact that we are working with several threads. More precise than that, we have not figured out. It feels a bit silly that this would be the cause of or problem, as the reason we want to use a static event accessor with locks is to handle multi-thread access...

UPDATE

So it turns out it now suddenly just works. As it should.

I honestly have no idea what the problem nor the fix was. I simply went on with my day and implemented a few other static event fields as well as subscribing to these from another file. Having recompiled the project it now works as it should. I did not edit the order of the statements in the file btw.

Thank you all for all your suggestions, thoughts and information. I realise the manner of this implementation (public static event) may not be optimal, but it was the route chosen for this implementation for the moment.

Matheos
  • 207
  • 2
  • 12
  • 1
    Also note that event add/remove operations are already thread-safe. There's no need to use an explicit backing field and a lock here – canton7 Mar 07 '22 at 13:27
  • 2
    I think we need a [mcve] here. The fact that you said moving the lock earlier in the class fixes it, implies that the event subscription is happening because of a static field initialisation which happens between the declaration of the lock object and the event. But your snippet doesn't show this. – canton7 Mar 07 '22 at 13:29
  • I fixed the code. I had just messed up when renaming variables for my example. My understanding was that I needed a solution like this, in order to implement thread safe capability. That the compiler generated accessors are not thread safe? – Matheos Mar 07 '22 at 13:30
  • 1
    It might make sense to post the stack trace of the exception. It could give some hints – Daniel Hilgarth Mar 07 '22 at 13:31
  • @canton7 I cannot really start posting a 5000 line long file which is part of our non open source code base. The subscription happens in a private constructor of another class. This class implements the singleton patter, meaning the private constructor is called from a static Property. Does this information help? – Matheos Mar 07 '22 at 13:35
  • 1
    Right, hence the "minimal" part. Please read the link for details – canton7 Mar 07 '22 at 13:44
  • [Possible repro](https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEBMBGAsAKHQGYACLEgYQIG8CT6zT1sA2EgEQ5IF4SYBTAO4cAFAEoA3HQbEyrEgCd+EBAHsYAGwCeJVcABW/AMYAXEhtVGA1gHkDxs7wHC9h0+Kn4GjOW34A3fhgzAFFA4IAJCBgEDX4FEhDpelovbwYVBBJqc0srEQtrOzcTMWySAF9K5PSlMFVA8sL85uKHMpyqipru/F6CWXJ2GhrZdnEa1PSGCgA6EJJkXhFVOBJ+Mu4APnKKz29eiqA==) – canton7 Mar 07 '22 at 13:49
  • 1
    @canton7 I think we've been bitten by similar bugs in the past and wear the same scars :) – Marc Gravell Mar 07 '22 at 13:52
  • 2
    The problem is in code you're not showing. There is probably a cyclic dependency in initializers that causes the lock object to be used during its own class' static construction. Look at the exception stack trace. – Matt Timmermans Mar 07 '22 at 14:03

1 Answers1

4

From the comments,

"That the compiler generated accessors are not thread safe?" - compiler-generated "field-like events" (i.e.

public static event Action<MyType, Guid, Guid> MyEvent;

are guaranteed by the specification to be thread-safe (the compiler currently uses a looped interlocked exchange, but lock has also been used in the past).

So: you don't actually need anything here. That said, static events are usually a bad idea and can lead to memory leaks. But: we also have a field initialization problem; now, as you suggest, the static field initializer here:

private static object myLock = new object();

should absolutely be ready when it is needed; the runtime guarantees static initializers are executed, so this should be fine; you could try adding readonly to make sure that you don't have code that is accessing it incorrectly, but I have to agree: this code should work fine. A full (but minimal) repro would probably be necessary to investigate it. The other thing (in addition to readonly) that I would be interested in looking for would be other field initializers, that might cause this event to get touched, for example:

private static Foo foo = new Foo();
private static object myLock = new object();

If the Foo() constructor calls back into the event, for example:

class Foo {
    public Foo() {
        YourType.MyEvent += SomeHandler;
    }
}

then the field initializer will not have finished yet, and will not have reached the myLock assignment. The order of static field initializers is preserved, noting that if you have multiple partial class files, then that order is itself not defined. In that scenario, making sure they are ordered correctly should fix it:

// IMPORTANT: make sure this stays first
private static object myLock = new object();
private static Foo foo = new Foo();
Marc Gravell
  • 1,026,079
  • 266
  • 2,566
  • 2,900
  • Woops sorry I forgot the readonly keyword. Yes I do use it already in fact, so adding/removing it does not change anything unfortunately. Thanks for the proposed scenario example. However, my class is not partial nor should it contain the scenario explained. I try to access the MyEvent event from another class' private constructor which in turn is called on demand to create a singleton instance of that class. This is done through a getter Property: MyOtherClass.Instance. – Matheos Mar 07 '22 at 14:02
  • Just for fun, [this is the current thread-safe implementation of the `add` and `remove` handlers for an event](https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEBMBGAsAKHQGYACLEgYQIG8CT6zT1sA2EgUwDd2YAXEgKLc+ACQgwEAG3YAnQQG4CAXwJA) – canton7 Mar 07 '22 at 15:02
  • @canton7 what troubles me is that editing the handler is thread-safe, but reading it is not volatile. If you invoke the event from multiple threads, do you have to use `Volatile.Read` manually? ([example](https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEBMBGAsAKHQAYACdbAOgBUALAJwFMIFYBzAbgPQGYzMSAwgQDeBEuLK9yANhIMAbgxgAXEgFFFKgBIQYCADYM66zvgmSy2WegAsJAErQAzgw1LlACgCUYiaLPmEmoA/BQAkjDyAPYA1gweMACu+vpwJEkpXqbmAL6+4jyW1naOUC5uKgDKEABm8T4B4v6BEgBqUfoQylCGFPZMCB6MNepeoRHRcQnJqekzWfkkefjLQA==)) I might post a new question about this. – Theodor Zoulias Mar 07 '22 at 17:54
  • @TheodorZoulias it doesn't really matter, honestly; even if a read came from cache (which is going to be super transient), it is going to be competing with data from another thread: so ultimately it would be indistinguishable from simply having happened in a slightly different ordering - the key point being: when you already have multiple threads fighting, *there is no reliable order here*, so a read of either is valid and acceptable. In other words: it isn't a problem that needs a solution. The "symptom" would be simply another valid and expected output, and would not be provable as a "fail" – Marc Gravell Mar 08 '22 at 08:08
  • @Matheos if it was me, I'd put a break-point in the `add` accessor, and look at the stack-trace; I would expect to find that we are in fact running in some tortured path *inside* the static initializer on the same thread – Marc Gravell Mar 08 '22 at 08:10
  • @canton7 hmm, you might be right, but I am not so sure. If I had to write such code today, I would definitely use the `Volatile.Read` in order to be on the safe side. Look at this question for example: [Difference between Interlocked.Exchange and Volatile.Write?](https://stackoverflow.com/questions/12425738/difference-between-interlocked-exchange-and-volatile-write) Nobody said there "Hey, use neither of those and everything will be OK". If omitting the `Volatile.Read` is OK, shouldn't be also OK to omit the `Volatile.Write`? – Theodor Zoulias Mar 08 '22 at 08:50
  • @TheodorZoulias Relevant: https://codeblog.jonskeet.uk/2015/01/30/clean-event-handlers-invocation-with-c-6/ . – canton7 Mar 08 '22 at 08:53
  • @canton7 from [Jon Skeet's article](https://codeblog.jonskeet.uk/2015/01/30/clean-event-handlers-invocation-with-c-6/): *`Volatile.Read(ref Foo)?.Invoke(this, EventArgs.Empty);` "that makes me nervous in terms of whether I’ve missed something."* -- Jon is nervous about whether `Volatile.Read` is enough. I guess he would be even more nervous by omitting the memory barrier entirely! – Theodor Zoulias Mar 08 '22 at 08:59
  • @TheodorZoulias again, though; in this scenario, *it doesn't matter*; the interlocked swap is required for correctness to avoid lost writes (i.e. lost adds/removes) when changing the value, but there simply *is no sensible definition of 'correct' for reads* here; when you have multiple threads writing and reading the event field, it is impossible and irrelevant to determine between the different outcomes, because *they're all legitimate outcomes* from different hypothetical thread timings, which are all non-deterministic *anyway*. The only thing we need to rule out is torn reads: CLI does that – Marc Gravell Mar 08 '22 at 09:06
  • The only thing I'd be worried about is if you're raising the event in a loop, and the JIT / processor decides to elide re-reading the field on every loop iteration. I know that can happen for normal field reads, but I'm currently failing to reproduce it in sharplab... – canton7 Mar 08 '22 at 09:07
  • *"The only thing I'd be worried about is if you're raising the event in a loop, and the JIT / processor decides to elide re-reading the field on every loop iteration."* -- @canton7 that's not a small thing! Btw here is another interesting reading: [C# Events and Thread Safety](https://stackoverflow.com/questions/786383/c-sharp-events-and-thread-safety), where the "obsession" about making the event accessors (add/remove) thread-safe is characterized as cargo cult programming. With which I tend to agree. Events are unsuitable for multithreaded scenarios anyway IMHO. – Theodor Zoulias Mar 08 '22 at 09:15
  • @TheodorZoulias In this scenario, you're necessarily unsubscribing from the event on a different thread to the one the event is being raised on. There's always going to be a race here: the event raise could always be started *just before* you manage to unsubscribe from it, so the actual event handler invocation could always happen *just after* you unsubscribe. That's just what happens with threading. Delays reading/writing the delegate field just make an existing race a teeny bit worse, but since you need to be safe to this race *anyway*, it doesn't matter. – canton7 Mar 08 '22 at 09:27