HashSet.Add generates null pointer violation

[vsfeedback]

This issue has been moved from a ticket on Developer Community.

---

The following generates the exception, started to happen 12th of may 2020, I did not notice any updates, assumed that this is a side effect that hapens the day before an update as you get all types of funny bugs before an update. Now today I got the update and the issue still occurs.

    public static Action<bool?, string>? Callback { get; set; }
    private static HashSet<string> _licensed = new HashSet<string>();

    /// <summary>
    /// Determines whether the specified assembly is licensed.
    /// </summary>
    /// <param name="assembly">The assembly.</param>
    private static void TestIsLicensed(object? state)
    {
        if (state is Assembly assembly && !string.IsNullOrEmpty(assembly.FullName))
        {
            //the issue will coour in the Add
            if (_licensed.Add(assembly.FullName))
            {
                bool? responce = _lm.Send(assembly);
                Callback?.Invoke(responce, "Server response");
            }
            else
            {
                Callback?.Invoke(null, "Was already registered");
            }
        }
        else
        {
            Callback?.Invoke(false, "Assembly missing");
        }
    }

---

Original Comments

Visual Studio Feedback System on 5/13/2020, 03:23 AM:

We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.

Tarek Mahmoud Sayed [MSFT] on 5/20/2020, 02:48 PM:

We couldn’t reproduce the issue with the code you have provided. could you please provide a complete project reproduce the issue? Also, do you have the stack trace of the thrown null reference exception?

Peter Pann on 5/21/2020, 00:33 AM:

I am not sure, I can't send you the project and not sure if I can create a project that will re-generate the same exception. What else can I send you that will help you understand the issue ?

Tarek Mahmoud Sayed [MSFT] on 5/21/2020, 08:33 AM:

you mentioned in the code that the problem happen in the line

_licensed.Add(assembly.FullName)

could you set a breakpoint under the debugger on that line and before the exception is thrown and then re-check the values of objects _licensed, assembly and assembly.FullName.To know which object is really causing the null reference exception. Please ensure the exception is thrown after checking these values. Sending the full stack also will help.

If this will not help, maybe the next step will be sending a debugger dump when the exception is thrown.

Peter Pann on 5/21/2020, 09:57 AM:

I did, none of the values is null, not _licensed. not assemply and not assembly.FullName, I think you have the watch window with the values

Tarek Mahmoud Sayed [MSFT] on 5/21/2020, 10:10 AM:

Do you have the full stack of the exception?

Peter Pann on 5/21/2020, 10:46 AM:

I only have what was included in the bug report. I have refactored the code and no just loop over a list<string> and if not found I add it, if found I return false.

I removed HassSet<string>.Add and not it works

Tarek Mahmoud Sayed [MSFT] on 5/21/2020, 10:56 AM:

sorry I missed the files you have attached to the report. I can see them now. I'll try to take a look.

Tarek Mahmoud Sayed [MSFT] on 5/21/2020, 00:54 PM:

I looked more and I am seeing this issue may occur only in the case of race condition. does your code can execute the HashSet.Add in multiple threads same time? if you are not sure, we may try having your code back to the state which reproduces the issue and then modified by taking the lock before adding to the set. something like:

lock(_licensed) // just for trial  
{
     if (_licensed.Add(assembly.FullName)) 
     {
          bool? responce = _lm.Send(assembly);    
          Callback?.Invoke(responce, "Server response");                  
     }                  
     else                  
     {                           
          Callback?.Invoke(null, "Was already registered");             
     }     
} 

Note that HashSet is not thread-safe.

---

Original Solutions

(no solutions)

[ghost]

Tagging subscribers to this area: @eiriktsarpalis
Notify danmosemsft if you want to be subscribed.

[tarekgh]

This issue reported against net core 3.1. I am seeing there is only one way we can end up throwing null reference exception in the line indicated by the reporter when having a race condition. here is how this can happen:

having simple HashSet like:

HashSet<string> hs = new HashSet<string>();

Then imagine you have 2 threads t1 and t2 calling `hs.Add("something");' in exact same time. t1 will execute the line

runtime/src/libraries/System.Collections/src/System/Collections/Generic/HashSet.cs

Line 1327 in 6e30997

if (_buckets == null)

and it finds _buckets is null and then it will jump executing Initialize method, which reaches and execute the line

runtime/src/libraries/System.Collections/src/System/Collections/Generic/HashSet.cs

Line 1263 in 6e30997

_buckets = new int[size];

which initialize _buckets but t1 will get suspended before executing the next like which initialize _slots. That is means _buckets != null and _slots == null. now t2 will start executing the line

runtime/src/libraries/System.Collections/src/System/Collections/Generic/HashSet.cs

Line 1327 in 6e30997

if (_buckets == null)

which will see _buckets != null and continue executing inside this method assuming _slots shouldn't be null which is not the case. Then it will throw null reference exception first time accessing _slots.

This could be by design as HashSet is not thread-safe and apps should handle the synchronization when using it. I just opened this issue so the owner can investigate more to know if it is possible we throw null reference exception for any different reason or we have any bug.

[eiriktsarpalis]

Thanks for the investigation. I'd probably agree with the position that this should be expected behaviour for objects that are not thread safe. I also find it unlikely that this not related to unsynchronized access given this is a fairly standard pattern, and the issue goes away as soon a lock gets used. I'd therefore be in favor of closing, cc @layomia