Docs: Document event implementation reasoning which covers race condition

[ite-klass]

The Events section describes how to implement the event ThresholdReached.

The method OnThresholdReached creates a variable and assigns it the event value.

The code makes you think that you could simplify the call from

EventHandler handler = ThresholdReached;
handler?.Invoke(this, e);

to

ThresholdReached?.Invoke(this, e);

However, the docs page How to: Raise Base Class Events in Derived Classes (C# Programming Guide) adds a comment to this statement (albeit it is hidden within quite a lot of code in the example code):

// Make a temporary copy of the event to avoid possibility of
// a race condition if the last subscriber unsubscribes
// immediately after the null check and before the event is raised.

I think it would be very helpful to educate users here why the copy is necessary/recommended. I think it should also link to the other page so users can learn why the protected virtual method is used to raise events.

The fact that the assignment operator = makes event copies rather than references was unexpected to me, which would also be a helpful clarification here.

---

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

• ID: caf84f7a-743b-867a-2ac5-2ab815f10c45
• Version Independent ID: 9b1d1a8a-5151-8e4d-2365-7c859e1bacd8
• Content: Handling and Raising Events
• Content Source: docs/standard/events/index.md
• Product: dotnet
• Technology: dotnet-standard
• GitHub Login: @rpetrusha
• Microsoft Alias: ronpet

[Thraka]

Thank you for opening this issue. I'll add it to the backlog. You can always submit the fix yourself by editing the original article. Click on the Content Source link at the bottom of your original comment. To learn how to edit, see the Editing files in a repository article from GitHub.

Thanks again!

[Thraka]

@BillWagner Something I've never really considered..

Should all event calls just be upgraded to a discard pattern to protect against this?

(_ = ThresholdReached)?.Invoke(this, EventArgs.Empty);

[BillWagner]

@Thraka You don't need the discard pattern, you can use ThresholdReached:

ThresholdReached?.Invoke(this, EventArgs.Empty);

The reason for the temporary is to avoid a race condition. Imagine a caller removes the only attached handler between testing for null, and dereferencing:

if (ThresholdReached != null)
{
    // imagine another thread removes the only handler here.
    ThresholdReached(this, EventArgs.Empty); // NullReferenceException thrown here.
}

The ?. guarantees that ThresholdReached is evaluated only once. In effect, it makes the temporary for you.

Yes, we should update these samples, and the associated text. I don't know a good way to find all of them.

[pkulikov]

@Thraka it looks that the compiler does all the magic:
https://sharplab.io/#v2:D4AQDABCCMDcCwAoEBmKAmCBhCBvJEhUaIALBALIAUAlHgUQL4OEvEaUCeWANgIYBnAW3yIi49gFMAbpIB2AFwgBRWYoASfOQBMekgE4QAygHsAtpIUALAJZyA5poAOT+ZO0IxE1l+9P9NtJ8CpJQ5M6ucrRs4qLe3qYW1naOfC5u2gD8AHQAknLSJgDWklTJAgA0KmoKAIL69gLZymZOCpw0nvEQzL49SIxAA==

In particular, the line

SomethingHappened?.Invoke(this, EventArgs.Empty);

is compiled into:

EventHandler somethingHappened = this.m_SomethingHappened;
if (somethingHappened != null)
{
    somethingHappened(this, EventArgs.Empty);
}

[Thraka]

Awesome. Thanks for clarifying 😄

So in summary, the fix for this issue is to change the code to ThresholdReached?.Invoke

[ite-klass]

Alright, thanks for the information and clarification.

I guess I can make the adjustments and submit a PR.

[ite-klass]

Should the docs examples make use of the ?. operator to train users to use it, or should the examples not use it to be compatible to older C# versions?

[Thraka]

It should be used. We're moving to the more modern C# practices.

[EnricoMassone]

@ite-klass please pay attention to one important point that you wrote at the bottom of your question.

You wrote:

The fact that the assignment operator = makes event copies rather than references was unexpected to me, which would also be a helpful clarification here.

Indeed the assignment operator (=) is not making any copy of the ShapeChanged object: there is only one object in memory and both the event ShapeChanged and the local variable handler contains a reference to that object. The reason behind this is that EventHandler<ShapeEventArgs> is a delegate and delegates are reference types.

Making a "copy" of the event (I'm using the quotes here because, as explained above, this is not an object copy but indeed a reference copy) is an effective way of preventing a race condition because when you unsubscribe an event (using the operator -=) you are indeed creating a new object in memory (the object which I'm talking about contains the underlying delegate for the event) (the same holds true when you subscribe to an event using the operator +=). This behavior is a consequence of the fact that delegate instances are immutable, so each time you operate on a delegate instance you will create a new object in memory.

Back to the documentation code example, this implies that if another thread unsubscribes the only existing subscriber then the event ShapeChanged becomes a null reference, but our code is safe because is operating on the local variable handler which contains a non null reference (which is the value contained inside the event ShapeChanged before the other thread unsubscribed the only existing subscriber).

The following code demonstrates that delegates are reference types and that adding a target to or removing a target from a delegate instance create a fresh new object in memory.

class Program
  {
    static void Main(string[] args)
    {
      Func<int> foo = new Func<int>(() => 6);

      // Delegates are reference types
      var temp1 = foo;
      Console.WriteLine(ReferenceEquals(foo, temp1)); // prints true

      // Adding a target to a delegate creates a new object
      foo += GetInt;
      Console.WriteLine(ReferenceEquals(foo, temp1)); // prints false

      // Creates another copy before removing target from delegate
      var temp2 = foo;
      Console.WriteLine(ReferenceEquals(foo, temp2)); // prints true

      // Removing a target from a delegate creates a new object
      foo -= GetInt;
      Console.WriteLine(ReferenceEquals(foo, temp2)); // prints false

      Console.ReadLine();
    }

    static int GetInt()
    {
      return 11;
    }
  }

For a more complete in depth explanation of these topics refer to this article from Jon Skeet