C# reducing/eliminating multithreaded race conditions (GuardedBy+) - Feature Request

[DavesApps]

C# reducing/eliminating multithreaded race conditions related to unprotected variable access – (via keywords/visibility and unprotected access protection) (Feature request)
Making multithreading less error prone: Reducing/Eliminating and detecting unprotected variable access (avoiding many common race conditions)

Proposal for new C# functionality.

Multithreaded code is easy to get wrong. A couple of the main concerns are:

1. Race conditions due to unprotected variable access are easy to introduce/ hard to detect
2. Deadlocks due to lock ordering
3. Livelocks, lock contention, etc.

This proposal addresses areas related to problem #1. It does this by helping to make multithreading decisions about which synchronization objects are protecting which code elements more visible. Currently when using locks developers reading others code need to infer what variables are to be protected and by which synchronization objects by looking at all of the variable accessed in a code block between acquiring and release of a lock. If we can make this more visible in code it will be easier to tell what variables/objects should be protected and by which synchronization objects. Also this proposal has the ability to detect when unprotected access to variables/objects is attempted to catch when these problems happen.
This proposal should be viewed as just a starting point for the discussion and not the end all be all of syntax and functionality (syntax/functionality should likely be improved and may need to handle cases that were not thought of at this point).

Goals

• Make it easy to visualize protected and unprotected variable access
• Detect unprotected variable access
• Improving the ease of use for synchronization objects

Keyword #1: guardedby (ISyncobject 1, [ISyncobject 2], …) [auto]

Sync object = A special thread synchronization class – Similar variations like, monitor, semaphore, readerwriterlock, etc.
Use for a class, Method, Property, or field.
Summary: Adding to a class, method, property or field tells the compiler to check to see if the lock for the synchronization object was acquired for this thread. If the sync object is not locked when a protected method, property, field is encountered an UnguardedAreaException() will be thrown.

The guardedby keyword takes an interface such that could be coded by the developer if desired a cusom guard object that could take multiple locks based on conditions. This allows great flexibility so that more complex locking scenarios can be checked. For example sometimes a variable is protected by one or more locks (sometimes reader writer locks use multiple locks), so a custom ISyncObject could support checking if the appropriate locks were held for that guard or not. Possible concern: It also opens up some significant danger from an improperly implemented locking mechanism.

Possible ISyncObject interface:

Interface ISyncObject
{
    Bool IsAcquired { get; }
    void Acquire();
   void AcquireWriter();  //not supported on all syncobjects or could just proxy to acquire() perhaps?
   Bool Release();
}

Optional auto keyword for guardedby
(Not entirely sure if this option should go in but something to consider as it could offer similar but better functionality than is currently provided by .net sync attributes)
The optional auto keyword can be used so that instead of requiring the developer to manually apply locking around all of the guarded variables, the variables will automatically acquire the lock if not acquired yet and release the lock at the end of the Method. If multiple variables use the same guard within the same method the lock is only acquired and released once for that guard/sync object.
Concerns: With this option it somewhat goes against the goals by hiding what is happening in locking, but could make ease of use better for simple multithreading cases.

Example (Class):

class MyClass guardedby(_monitor)
{
    ISyncObject _monitor=new SyncMonitor();
   //…
}

Example(Method):

class MyClass 
{
    ISyncObject _monitor=new SyncMonitor();
    void MyMethod() guardedby (_monitor) { //…
   }
   //…
}

Example (field):

class MyClass 
{
    ISyncObject _monitor=new SyncMonitor();
    Private guardedby(_monitor) String mystring=””;
 
 }

Keyword #2: locking ([writer] sync object1, sync object2, …) { … }

Or
locking ([writer] guarded object1, guarded object2, …) { … } //Maybe?

Summary: This keyword will acquire the lock for all of the syncobject’s specified. It also hold a reference. At the end of the code block the lock will be released. Optionally a writer keyword can be added and if the syncobject supports writer locks it will acquire a writer lock. While within the code block no other area in the same thread will be able to release the lock until the end of the code block.
NOTE: Currently the only synchronization object that supports lock keyword is the Monitor, which is a shame since having the ability to lock(object) { } and lock and release while handling exceptions for the other syncobjects would save a lot of typing and potential mistakes. Supporting this new variation adds that capability for ALL supported synchronization objects.
The second variation of locking allows locking directly on the guarded objects themselves, this avoids the developer having to go figure out which syncobject protects which variables/properties and will just acquire the locks automatically. If an unprotected variable is requested a compiler error will occur. This is mostly useful only when a small number of variables are used in a method, as if the variable list was large it would probably be easier to just find the related syncobject. Perhaps a better answer would just be have intellisense suggest the syncobjects when the “locking(“ keyword is typed.
If when calling locking(…) the lock is already acquired for the thread it is optimized to not try to lock again, reducing the worry about overhead by locking in multiple places. Currently the way developers avoid the overhead is by calling the lock in a higher level function and calling functions that don’t lock themselves, and then unlocking at the end. By offering the optimization check for the thread developers can be free to lock and protect every function without the worry about significant overhead. (That old style method can be used still and will offer a slight performance benefit but might not be worth the effort if this proposal is implemented).

Its possible that the same “lock” keyword currently being used in .NET could be used in place of “locking” or some other keyword. It could just operate on the new sync objects differently.

class MyClass 
{
    ISyncObject _monitor=new SyncMonitor();
    Private guardedby(_monitor) String _mystring=””;
   Private guardedby(_monitor)  int _count=0;

  void MyMethod(string value)  {
   locking(_monitor)
   {
      _mystring=value;
   }
  
// a slightly more succinct variation, locks for the entire function call
  void MyMethod2(string value)  locking(_monitor)  {
      _mystring=value;
 }

// a variation that would require the caller to acquire the syncobject lock before calling.  If the caller did 
// not lock the syncobject it would throw an exception
  void InternalMyMethod3(string value)   {
      _mystring=value; // exception thrown if _monitor not locked, otherwise sets value
 }

// this example locks instead of on the syncobject on the variables that are guarded themselves
// This alleviates trying to figure out which objects are guardedby which syncobjects and lets the compiler handle that for you by supplying the protected variables to the locking call rather than the syncobjects
  void MyMethod3(string value,int cValue)  locking(_mystring,_count)  {
      _mystring=value;
      _count=cValue;
 }


 }

Keyword #3

unguarded([Property or field 1], [Property or field 2] [, …])
{
// unguarded code
}
Summary: The unguarded keyword allows access to guarded properties or fields without acquiring the lock, and it will not trigger the exception. This allows for setting values efficiently within the constructor of classes. It also allows efficient access for situations known not to be problematic (i.e. a variable that never changes.) Only the specified properties or fields are unguarded and only for the current thread, accesses from other threads can still trigger the exception if read or written to when the guard is not acquired.

Possible option:Perhaps it could be decided that constructors are always safe to automatically unguard variables during construction. (This should be thought about as having the compiler hide this goes against one of the goals to make understanding guarded or unguarded areas of the code more visible – an alternative might be allow the keyword to be applied to the Method level which would include constructors, so that it could be explicitly applied)

Public Class MyClass guardedby(_monitor)  /// guards all fields and properties
{
   ISyncObject _monitor=new MonitorSyncObject();
   Int _count=0;  //guarded by _monitor
   String _mystring=””; //guarded by _monitor
   Unguarded _myval=0; //NOT guarded by _monitor

   Public MyClass()
   {
     Unguarded(count, mystring)
     {
        _count=1;   //no exception thrown here even though lock not acquired
        _mystring=”test”; //no exception thrown here even though lock not acquired
     }
     _myval=1;  // no exception even though not locked as variable declared ungaurded
   }
}

Addendum

What is a SyncObject?
Syncobject could be nothing more than a wrapper around existing synchronization classes like Monitor, Semaphore, ReaderWriterLock, etc. The SyncObject would implement an interface with so that all syncobjects could be acquired and released in the same way. There would be a regular interface and a reader/writer interface since readerwriter locks need the ability to differentiate a writer lock.
SyncObject would implement the ISyncObject interface mentioned earlier.

Runtime Detection of unprotected access
This potentially could be a compiler define flag so that it could be compiled in or out based on build to alleviate any overhead concerns people might have about using this option. This would allow either enabling in just a debug build or if desired leave on for production release builds as the overhead should not be high, and in many cases inconsistent object states by unprotected variable access is worth stopping the program for since it could result in unstable or undesired program operation.

How is this proposal different than earlier papers, or articles?
Most of the earlier implementations and material I’ve been able to find either had one of the problems of: Limited use/Not flexible, too much overhead (in either syntax, runtime or both). Take for example the ability .NET has to synchronize object access with an attribute on the class. This allowed protecting the class at every entry point, but didn’t allow variations of synchronization object use and the flexibility to have one lock be held to execute multiple operations/methods within one lock/unlock code area. This adds significant overhead and doesn’t easily allow for an aggregate object to use a single synchronization object to lock everything.
This proposal offers great flexibility, makes multithreading code decisions very visible for easier code review/debugging, and can detect unprotected variable access. Other proposals also didn’t suggest unguarded areas in a similar way to allow for additional flexibility when needed while still making that design decision very visible, plus limiting the scope of the unguarded area.

Counter argument: Shouldn’t runtime unprotected access checks be a tool and not in the code?
Response: Sure it could be a tool, but .net already does things like runtime array bounds checking and checking if not on the UI thread when making UI calls. So the proposal here doesn’t really go much further than doing something similar to what those functions already do.
Also a tool doesn’t offer the benefit of making it obvious in the code which variables are protected by which synchronization objects, that will make it easier for developers to declare protected items and review them, plus ensure they abide by them.
Another argument is performance aspect. I believe this can be implemented in a way that is very lightweight down to the equivalent of checking a thread local Boolean perhaps for determining if a lock is being held. Also providing the option to compile out these checks for a build could be provided to alleviate any runtime concerns, and allowing for just debug or custom builds to be used to check for unprotected variable access.
It’s also possible that these new keywords could help a tool better do compile time static checking/validation of locks being held before using guarded values, so these changes could further benefit a tool or even compile time analysis.

[svick]

Could you format code in your post by enclosing it in triple backticks (```)? It makes it much easier to read.

[benaadams]

Something like

https://www.microsoft.com/en-us/research/wp-content/uploads/2017/07/snowflake-extended.pdf

/cc @mjp41

[svick]

1. Your proposal focuses almost exclusively on runtime checking. Why? I think this is something that can be checked at compile time, which would make it safer (detects errors everywhere, not just in code that is run) and cheaper (no runtime overhead).

2. I think the auto keyword is a terrible idea. I think locking is important and should be visible in code. With the proposed implicit locking, I think people would often lock too little (since locking would happen on the level of method, but any methods that call those methods would not be atomic) or too much (e.g. if I access a guarded field at the start and end of a long-running method).

3. The proposal that locking should check whether the lock is already acquired and do nothing if it is, means that all locks would effectively be recursive. Recursive locks have their advantages (which is why lock is recursive), but they also have disadvantages. I'm not sure forcing all synchronization to be recursive is a good idea.

4. If you wanted to perform compile-time checking (which, like I said before, I think is a better idea than runtime checking), you wouldn't need new syntax or external tools. You could take advantage of existing capabilities, namely analyzers and attributes. For example, one of your examples could look something like this:

   class MyClass
   {
       object _monitor = new object();
       [GuardedBy(nameof(_monitor))]
       private String _mystring = "";
       [GuardedBy(nameof(_monitor))]
       private int _count = 0;
   
       void MyMethod(string value)
       {
           lock (_monitor)
           {
               _mystring = value;
           }
       }
   }

   You would then have an analyzer that checks that all accesses to guarded variables are properly locked. The analyzer could be able to recognize other synchronization primitives too, possibly including something like your ISyncObject interface.

[DavesApps]

@svick thanks for taking the time to read it :)

#1 I like the idea of compile time checking, I just wasn't sure it would be possible for the compiler to analyze all the possible code paths involved since many could be impacted by runtime decisions. I think there definitely could be some compile time checking, I'm just not sure if it could handle all cases? But I agree the compiler can do some of this checking at the very least. It's possible if a more stringent requirement for locking is used. (Like if the only way to lock is to use the locking keyword followed by { ... } it may be possible to completely detect unprotected access conditions at compile time. It loses some flexibility that way, but something to think about.)

#2 Yes I agree the auto keyword as mentioned hiding the visibility would be bad. Perhaps as mentioned having the locking keyword auto detect if the lock is already acquired would reduce the overhead enough so that it could be applied to every method needed (where normally folks would resort to holding one lock and not locking in methods called.).

#3 Yes it is a good point is that it allows multiple entry for the same lock if acquired on the same thread. Perhaps based on the SyncObject used, and options on it, it could decide if it works that way or not.

#4 your example definitely could be compile time checked. I think when the examples get bigger that checking gets harder... for example if MyMethod called other methods within the lock.. and what if Monitor wasn't a monitor, and was instead a semaphore and released a semaphore (then accessed variables)... in one of the methods called. Would the compiler be able to catch that? (Because it might be based on a runtime condition. ) It would be nice if there could be some compile time checking like that though.

My original thinking was around using attributes as well. I think they could work though they might not have the flexibility without adding some other keywords (especially unguarded for example). So in for a pound in for the rest maybe at that point perhaps go with keywords? (Unguarded attributes might work if they could be applied at statement level.. I don't think I've ever tried an attribute on a statement?...)

[DavesApps]

@benaadams thanks, interesting I have to read through that a bit more (only got through the first couple pages) but I see in the beginning the parts like the Shield that I think sound similar to the guard. I almost considered shield as an alternate name to guard when doing this writeup.

I also prototyped having guarded variables in thinking through this idea and they look kind of similar to shield. The proposal can be mocked up using templated classes for sure. Keywords/language support of something like this might make it a little prettier, less wordy.

[jamesqo]

Disclaimer: I didn't read most of the proposal. However, I saw @svick's comment suggesting that the GuardedBy feature could be an attribute instead of a keyword, which I think is a great idea. I personally think it would be better to enforce it in Roslyn itself than in a third-party analyzer, however, since the use case is so common.

I also wanted to point out that Java has an annotation exactly like this already, with IDE support for enforcement: https://developer.android.com/reference/android/support/annotation/GuardedBy.html. It sounds very reasonable to give C# this GuardedBy attribute, too.

[DavesApps]

To summarize:

• I don't believe that just using attributes alone and static analysis/compile time analysis would allow for catching ALL cases of unprotected access, unless significant restrictions were put in place on how synchronization objects are used. Having runtime checking will catch probably 100% of unprotected access. I believe compile time checking could only catch some percentage of them (but a much smaller percentage if flexibility is allowed). (Does anyone think all cases could be caught compile time?) It might make sense to do both compile time to catch a smaller percentage of items and runtime checking to catch all the rest.

• Also attributes alone probably need more flexibility for keywords like unguarded keyword as mentioned to allow for setting variables in constructors or other locations without triggering false positives.

[DavesApps]

See this ACM article showing one of the leading causes of bugs found for C# is race conditions:

https://cacm.acm.org/magazines/2017/10/221326-a-large-scale-study-of-programming-languages-and-code-quality-in-github/fulltext

It's possible this proposal could help reduce this type of bug.

[DavesApps]

I thought of an additional idea that could be added to this. It is very difficult to relay the intentions (in code) of how methods or internals of a class should be used in relation to multiple threads. One of these intentions that is hard to encode is the nature of a variable used within the context of a synchronization (i.e. within a lock). For example you might be trying to remove an item from an array in a thread safe way but need to find it first, so find an index in array within a lock matching the item you want to delete. That index is only useful within the context of the current synchronization (lock) as if you exited the lock and tried to remove that index you could end up with corruption/race conditions. I had the idea of a thread synchronization context variable. The variable would only be usable within the current synchronization context, to avoid cases where someone might try to use a value outside of the context at some future point (while maintaining the code.). It basically encodes the intention of the variable to make it clear it's use. A simple way to do this might be something like a generic ThreadSyncContextVar<int>(syncobject), so trying to use it when the sync object hasn't been acquired or it was set as part of a previous call of the synchronization object would throw an exception. Perhaps the compiler could even detect inappropriate usages in some cases as well. Perhaps this could be done cleaner with some keywords (I don't think attributes work on local variables) or some other way perhaps.

`

ThreadSyncContextVar<int> index=new ThreadSyncContextVar(mysyncobject);
locking(mysyncobject)
{
index=myarray.FindIndex(finditem);
}
myarray.RemoveIndex(index); // would throw an exception as the index value is used outside of the synchronization context where it was assigned.
<
`

[DavesApps]

Herb Sutter makes arguments for very similar functionality in C++ at CppCon 2018 https://www.reddit.com/r/programming/comments/9jnph6/cppcon_2018_herb_sutter_thoughts_on_a_more/

C++ has some different implementation methods available to it perhaps than csharp but still makes sense in some way.

In the video around about 00:56:13 to 1:05:00.

[DavesApps]

Here is a demo of what can be done, though this could be done much better by the compiler to potentially even detect compile time improper usage.

https://github.com/DavesApps/ThreadGuard/

[DavesApps]

It is a good question. What type of detections of improper usage might be possible with Roslyn? All of them or would it be limited?

At runtime it can detect all usages without being acquired.

[CyrusNajmabadi]

Any checks you would want found by the compiler at compile time are things you could check for with your analyzer at compile time :-)

The compiler is not special here. :-)

[DavesApps]

For example the runtimes checks can easily tell when used if a lock has been acquired right then and there. But in the analyzer is that detectable statically everywhere? Seems it would be easy if all usage was in a single method. But if lock acquire happened in a different method(s) than the variable usage static detection sounds more difficult.

[CyrusNajmabadi]

I"m not sure what you're then asking for C# to do here. If this cannot be done statically, then how would the compiler help here?

[DavesApps]

In the example I provided I needed to use classes to wrap variables for detection. In theory if there was a guardedby keyword maybe the compiler could wrap any variable use itself (basically syntactic sugar keyword to create the wrapper classes (or no class at all) for runtime detection - so existing standard types could be used as is.) That might be beneficial rather than doing these as wrapper classes manually by each developer. As far as detection statically I was hoping some folks here had some thoughts on how much if any static detection might be possible (or is leaving that to runtime the best)?
Or alternatively could perhaps always require a re-entrant pattern of requesting the same lock in any method so it's easy for the compiler/roslyn to detect... but that would add the burden of requiring some idiom in each method where a shared variable is used.

Open to any other ideas.

[DavesApps]

I didn't realize when I suggested this it has some similarities to the Rust thread safety model. Is there any consideration of some variation of this/Rust thread safety for C#?