[API Proposal]: Disposable pattern in synchronization primitives

[pzaj2]

Background and motivation

The idea for this proposal came to me as I was helping to identify an issue a couple of weeks ago where a complex block of code would not release a semaphore in an edge case.

I have since then built a library to solve this issue (pending internal company review before being published to NuGet) and raised a change proposal with the #C language (https://github.com/dotnet/csharplang/discussions/6466).

The consensus around my initial proposal to add the support directly to the C# language was that it adds value but should instead be implemented in the BCL, so here is my proposal.

The premise is that synchronization mechanisms can freeze the whole system when implemented improperly and in many cases may be hard to debug/identify. I would therefore like to propose changing the API to make it safer to consume synchronization primitives [in my example, SemaphoreSlim]. Please keep in mind that ideally, all synchronization primitives should be aligned and implement the disposable pattern.

API Proposal

Please keep in mind that this proposal is merely to depict what I have in mind. Implementing it this way would be a substantial change and therefore, I believe the direction needs to be discussed.

namespace System.Threading;

internal class Guard: IDisposable {
    private readonly SemaphoreSlim _guarded;
    internal Guard(SemaphoreSlim guarded) {
        _guarded = guarded;
    }

    public void Dispose()
    {
        _guarded.Release();
    }
}

public class SemaphoreSlim : IDisposable {
    ... ctors and existing implementation [minus the modified methods]

    public IDisposable Wait() { // This would return an instance of `System.Threading.Guard`

    } 

    public Task<IDisposable> WaitAsync() { // This would return an instance of `System.Threading.Guard`

    } 

   ... remaining overloads of `Wait` and `WaitAsync`, also modified to return `IDisposable`
}

API Usage

public class SynchronizedProcessor {
    private static SemaphoreSlim _sync = new SemaphoreSlim(1, 1);

    ... ctors, etc.

    public void Process(params object[] args) {

        using (_sync.Wait()) {
            ProcessInternal(args);
        }
    }

    private void ProcessInternal(params object[] args) {
        ... operations requiring synchronization
    }
}

Alternative Designs

Since I appreciate that such a proposal is invasive, I'd be more than happy to consider alternatives. Some of the alternatives that come to my mind are:

• creating a new set of primitives [1:1 with existing primitives] that would follow this convention, implementing the disposable pattern
• creating a set of wrappers around existing primitives that would follow this convention, be referenced by the documentation & doc comments on the existing primitives as a safer alternative
• extending the existing primitives with a set of new methods that implement the disposable pattern (not sure about naming, for semaphores it could be Claim and ClaimAsync? Definitely open to suggestions)

Risks

To my knowledge, the only risk that may proposal introduces is breaking changes to SemaphoreSlim and the other synchronization primitives.

That said, the alternative designs/proposals eliminate this problem, although they may not be as "successful" as the main proposal as they do not enforce the disposable pattern. This could only work if enough effort is put towards promoting this solution as safer and more convenient throughout the documentation, in-code docs and contributions from the community in a form of articles, posts, etc.

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @mangod9 See info in area-owners.md if you want to be subscribed.

Issue Details

---

### Background and motivation The idea for this proposal came to me as I was helping to identify an issue a couple of weeks ago where a complex block of code would not release a semaphore in an edge case. I have since then built a library to solve this issue (pending internal company review before being published to NuGet) and raised a change proposal with the #C language (https://github.com/dotnet/csharplang/discussions/6466). The consensus around my initial proposal to add the support directly to the C# language was that it adds value but should instead be implemented in the BCL, so here is my proposal. The premise is that synchronization mechanisms can freeze the whole system when implemented improperly and in many cases may be hard to debug/identify. I would therefore like to propose changing the API to make it safer to consume synchronization primitives [in my example, `SemaphoreSlim`]. Please keep in mind that ideally, all synchronization primitives should be aligned and implement the disposable pattern. ### API Proposal Please keep in mind that this proposal is merely to depict what I have in mind. Implementing it this way would be a substantial change and therefore, I believe the direction needs to be discussed. ```csharp namespace System.Threading; internal class Guard: IDisposable { private readonly SemaphoreSlim _guarded; internal Guard(SemaphoreSlim guarded) { _guarded = guarded; } public void Dispose() { _guarded.Release(); } } ``` ```csharp public class SemaphoreSlim : IDisposable { ... ctors and existing implementation [minus the modified methods] public IDisposable Wait() { // This would return an instance of `System.Threading.Guard` } public Task WaitAsync() { // This would return an instance of `System.Threading.Guard` } ... remaining overloads of `Wait` and `WaitAsync`, also modified to return `IDisposable` } ``` ### API Usage ```csharp public class SynchronizedProcessor { private static SemaphoreSlim _sync = new SemaphoreSlim(1, 1); ... ctors, etc. public void Process(params object[] args) { using (_sync.Wait()) { ProcessInternal(args); } } private void ProcessInternal(params object[] args) { ... operations requiring synchronization } } ``` ### Alternative Designs Since I appreciate that such a proposal is invasive, I'd be more than happy to consider alternatives. Some of the alternatives that come to my mind are: - creating a new set of primitives [1:1 with existing primitives] that would follow this convention, implementing the disposable pattern - creating a set of wrappers around existing primitives that would follow this convention, be referenced by the documentation & doc comments on the existing primitives as a safer alternative - extending the existing primitives with a set of new methods that implement the disposable pattern (not sure about naming, for semaphores it could be `Claim` and `ClaimAsync`? Definitely open to suggestions) ### Risks To my knowledge, the only risk that may proposal introduces is breaking changes to `SemaphoreSlim` and the other synchronization primitives. That said, the alternative designs/proposals eliminate this problem, although they may not be as "successful" as the main proposal as they do not enforce the disposable pattern. This could only work if enough effort is put towards promoting this solution as safer and more convenient throughout the documentation, in-code docs and contributions from the community in a form of articles, posts, etc.

Author:	pzaj2
Assignees:	-
Labels:	`api-suggestion`, `area-System.Threading`, `untriaged`
Milestone:	-

[Joe4evr]

I like the idea, since I've also made IDisposable wrappers for synchronization primitives on occasion. But I have to point out:

   ... remaining overloads of `Wait` and `WaitAsync`, also modified to return `IDisposable`

This is a no-go immediately as changing return types is a breaking change, so that will never happen. Also, since C# doesn't support overloading by return type, another name would be needed. (🍝 WaitLease/WaitLeaseAsync?)

Additionally, there's a strong likelihood these methods should return a public struct so it won't cost an allocation to use.

[hez2010]

since C# doesn't support overloading by return type

C# doesn't support it, but IL does.

[pzaj2]

I like the idea, since I've also made IDisposable wrappers for synchronization primitives on occasion. But I have to point out:

   ... remaining overloads of `Wait` and `WaitAsync`, also modified to return `IDisposable`

This is a no-go immediately as changing return types is a breaking change, so that will never happen. Also, since C# doesn't support overloading by return type, another name would be needed. (🍝 WaitLease/WaitLeaseAsync?)

Additionally, there's a strong likelihood these methods should return a public struct so it won't cost an allocation to use.

I agree that breaking changes may be a no-go, hence my alternative proposals. WaitLease seems to make sense, definitely better than Claim if you ask me. Overloading by return type would be nice, for sure, but as you said - C# doesn't support it.

As for returning a public struct - my snippets are merely an example. I'm, however, curious as to how this would affect allocation or lack thereof. Any documentation/articles that explain that in more detail? I'm keen to see where this is coming from.

Thanks for the comment, it is definitely a step in the right direction from my point of view.

[svick]

As for returning a public struct - my snippets are merely an example. I'm, however, curious as to how this would affect allocation or lack thereof. Any documentation/articles that explain that in more detail? I'm keen to see where this is coming from.

In your snippet, Guard is a class, so it is allocated on the heap. If you create it every time WaitLease is called, then that's an extra allocation for every call. If you create it once per SemaphoreSlim, then that's still an extra allocation and it increases the size of SemaphoreSlim (because it needs to be stored in a field). Just changing Guard to a struct wouldn't help, because value types are boxed when converted to an interface like IDisposable and boxing requires an allocation.

But directly returning a public struct, without converting to IDisposable, does not make any allocations.

[bjornen77]

I agree, returning public struct will reduce allocations. Also returning a ValueTask instead of Task would make this completely allocation free.

public readonly struct Guard: IDisposable { }

public ValueTask<Guard> WaitLeaseAsync()  {  } 

[stephentoub]

Also returning a ValueTask instead of Task would make this completely allocation free

Only when the primitive could be acquired immediately / synchronously. SemaphoreSlim.WaitAsync itself will allocate a Task if the semaphore can't currently be acquired, and then the wrapper method returning a ValueTask would also need to either allocate its own continuation task (which is what would happen under the covers if that method were async ValueTask), or it would need to entail some kind of pooling, which might reduce allocation but has its own costs to consider. If we were to add a method like this, making it return ValueTask would be fine, I'm just pointing out it wouldn't be "completely allocation free".

But directly returning a public struct, without converting to IDisposable, does not make any allocations.

This is true, but it's also one of the reasons we haven't added such methods to these types in the past. Generally developers expect IDisposable.Dispose to be idempotent, but without allocating, we wouldn't be able to guarantee that, and in the case of a type like SemaphoreSlim, multiple disposes would then lead to multiple releases, which could silently lead to corruption (due to allowing more concurrency than was intended to be allowed).

That's not to say we can't add methods like those proposed, but if we added it with the lease as a class, we'd be allocating, and if we added it with the lease as a struct, we'd risk this kind of misuse. As such until now we've left it for developers to do in their own code bases and choose for themselves which poison they prefer.

[bjornen77]

I'm just pointing out it wouldn't be "completely allocation free".

Yes, this is true. I meant allocation free in best case scenario :)

multiple disposes would then lead to multiple releases,

Could this be solved in the guard struct by also having a bool field and use that boolean to make sure SemephoreSlim release is called only once using Interlocked exchange?

[stephentoub]

Could this be solved in the guard struct by also having a bool field and use that boolean to make sure SemephoreSlim release is called only once using Interlocked exchange?

That won't help if the struct is copied.

[bjornen77]

Also true.. There are lot of small things that could go wrong if not careful as you say :)

If this will be added I think that documentation on how to use this properly will be important.

[bjornen77]

The "life time" of the SemephoreSlim is also returned as it is referenced in the guard class/struct. Therefore it is necessary to keep that object alive(not disposed) as long as the guard class/struct is used. No problem if it is static, but otherwise. This might not be obvious by user.