Document event implementation reasoning which covers race condition

[ite-jankl]

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