Missing synchronization for a variable but it is seemingly not an issue. Why?

[kiminuo]

Hey guys,

I was hoping to show by the following xUnit test that shouldStop variable has to be synchronized (that one should use lock (obj) { ... }). However, the test always passes (in both Debug and Release configurations; both were run in Visual Studio for Windows on an Intel x64 machine):

using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Xunit;
using Xunit.Abstractions;

namespace MyProject.UnitTests;

public class LockingTests
{
	public ITestOutputHelper TestOutputHelper { get; }

	public WhitelistTests(ITestOutputHelper testOutputHelper)
	{
		TestOutputHelper = testOutputHelper;
	}

        [Fact]
        public async Task AskingForTroubleAsync()
        {
	        int iterations = 500_000;
	        long errors = 0;
        
	        for (int i = 0; i < iterations; i++)
	        {
		        using CancellationTokenSource cts = new(1_000);
        
		        long set = 0;
		        bool shouldStop = false; // THIS VARIABLE IS NOT SYNCHRONIZED IN ANY WAY.
        
		        Task<bool> task1 = Task.Run(() =>
		        {
			        while (true)
			        {
				        if ((Interlocked.Read(ref set) == 1 && !shouldStop))
				        {
					        Interlocked.Increment(ref errors);
					        return Task.FromResult(false);
				        }
        
				        cts.Token.ThrowIfCancellationRequested();
        
				        if (shouldStop) { break; }
			        }
        
			        return Task.FromResult(true);
		        });
        
		        shouldStop = true;
		        Interlocked.Increment(ref set);
        
		        bool result = await task1.WaitAsync(cts.Token).ConfigureAwait(true);
        
		        if (!result)
		        {
			        TestOutputHelper.WriteLine($"Failed in iteration #{i}");
		        }
	        }
        
	        long totalErrors = Interlocked.Read(ref errors);
        
	        if (totalErrors > 0)
	        {
		        Assert.Fail($"The error rate was {totalErrors}/{iterations}, i.e. {(double)totalErrors / iterations * 100}%.");
	        }
        }
}

Is the test passing because

a) it's wrong in some way
b) the hardware provides stronger memory guarantees (I vaguely remember I read about it some time ago).
c) there is some memory barrier that ruins my test
d) something else
e) my understanding of synchronization is wrong :)

?

Basically, my goal was to achieve what this sample https://stackoverflow.com/questions/133270/illustrating-usage-of-the-volatile-keyword-in-c-sharp/1284007#1284007 (not tested) achieves but async code (Tasks). To say it explicitly, I would like to show that there is a "missed write" (i.e. one thread writes a value something and a reading thread misses that change).

Any feedback is welcome!
Thank you

PS: The goal of the unit test is just to persuade people that one should lock access to class fields when those class fields are accessed from mutliple threads as then no unwanted surprises arise.

[Clockwork-Muse]

For your purposes, probably "a": the task you queue (despite the name, Task.Run() doesn't always start everything immediately) likely doesn't start running until after shouldStop gets set to true. This doesn't necessarily prevent cache issues, but it doesn't help your case either.
Additionally, if the main thread manages to get to bool result = await task1.WaitAsync(cts.Token).ConfigureAwait(true); before the queued task gets launched on a separate thread, the async mechanism can use the existing thread, meaning that you aren't going to have cache issues. Recall that Tasks are backed by a shared thread pool (and other more complicated things).

To fix these issues, you need to have a barrier that prevents the main thread from setting shouldStop until after the internal task has been confirmed to have started.

(that one should use lock (obj) { ... })

lock and async don't always play well together. You can use lock inside an async method in some areas, but most practical uses of async code end up sidestepping it.

PS: The goal of the unit test is just to persuade people that one should lock access to class fields when those class fields are accessed from mutliple threads as then no unwanted surprises arise.

.... multithreading is a non-trivial topic. As stated, that's probably not sufficiently specified advice. For instance, while lock does help with cache issues, that's not it's primary or intended use. Instead, lock is primarily intended to ensure atomic updates to objects (multiple fields updated at the "same time" - ie, adding an item to an array and incrementing the count of held items).
(Strictly speaking the underlying thing for dealing with cache coherence are barriers/fences, but it's usually just way easier and safer to use locks)
For simplistic advice about threading it's probably better to say to avoid cross-thread/task communication altogether