Static lock object is not initialized in time-CodePudding

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...

CodePudding user response：

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();

CodePudding user response：

So it turns out it now just works, as expected...

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.