-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proposal]: Lock statement pattern (VS 17.10, .NET 9) #7104
Comments
Dictionary<string, object> dic = new();
Lock lo = new();
lock(dic["my lock"] = lo) {}//this does not work.
class MyLock
{
public LockHolder EntersLockWithHolder() {...}
public struct LockHolder
{
public void Dispose() {...}
}
}
...
static readonly MyLock lo = new();
static void MyMethod()
{
lock(lo)
{
...
}
} Everything compiles even though the spelling This may also be problematic if This problem could be solved by introducing a custom attribute only to be found in .NET 8 BCL that must be present on such lock types. The compiler would then detect any misspelling, and backporting to earlier version would result in a compilation error.
|
If we consider implementing this feature, shouldn’t we consider its async counterpart which is lowered to |
I believe that should follow a completely different pattern as locks should have to opt-in to being "async"-friendly. |
I have to agree, I think there's enough danger of these newer-style locks being accidentally treated as monitor locks by the compiler in any case where the pattern just happens to not match, with no warning to the developer. I think that needs to be added to the list of "Drawbacks" for this proposal. Is the plan to also extend this to the myriad of other locks or synchronization primitives that exist in the BCL? |
Would it be feasible to introduce the ability of a type to forbid using [ForbidMonitor]
class Lock
{
public LockHolder EnterLockWithHolder();
public struct LockHolder
{
public void Dispose();
}
} And used like this: private readonly Lock _lock = new();
void Foo()
{
lock (_lock) // lowers to EnterLockWithHolder()
{
}
lock ((object)_lock) // lowers to Monitor.Enter, but throws at runtime
{
}
} |
How would that be enforced? If you upcast the |
@HaloFour As per the comment, it would be runtime-enforced. Presumably, if |
@HaloFour I meant that the compiler would use But I don't know if that could be implemented without making every existing use of |
Sorry, I see what you meant after another re-reading of the comment. It'd prevent a subtle break, but you could still end up with exceptions at runtime if you're not careful. I'd love to see more language support around locking types. It's always annoying me that none of the lock types in the BCL even offer support for |
Perhaps Also, though I have seen |
Hmm, my initial reaction is, not only we shouldn't add to |
Having thought about it, I would have liked in a "perfect world" to have |
Correct and that is an explicit design choice. This proposal breaks compat for any type which is being locked on today that just happens to fit the new locking pattern. Casting to lock(dic["my lock"] = lo) {}//this does not work. This particular case is not very realistic to me. In practice, it's very uncommon for the expression inside a |
Sounds like something which should be much more explicit. As described, it's one accidental behavior that has an accidental fallback; a pit of failure in a pit of failure. |
Wouldn't it go against the principle of least surprise? |
tbh, i would expect people to be more surprised by how things work today. Intending 'lock' to have the semantics appropraite for the lock-instance they're working with, rather than it having Monitor semantics. |
For a c# programmer who knows all about await, foreach, using, and their surface area semantics, but absolutely nothing about lock, it would indeed be extremely weird to realize lock is so different from these other constructs. But that's a very small (if at all existent) demographic. Most people are very much used to lock relying on Monitor and this is a major departure. |
As I mentioned the far more common case of locking is done against a variable: a local or field. lock (guard) { In that case the compat fix, casting to lock ((object)guard)
I don't agree this is an accidental fallback, it's a very deliberate one. This new behavior is entirely about API matching. Code that wants to take advantage of that needs to be careful to maintain the correct type. The alternative is to essentially provide a diagnostic whenever a lockable type is cast to a base type or interface. That seems quite extreme and prevents these types from having other useful behaviors. Customers who want to use the new locking pattern and are warry of this problem can simply implement it on a |
Except when the cast (or generic code) is far removed from the Admittedly, as with Monitor-based locks, I'd think it'd be somewhat uncommon for these instances to be passed around, so maybe the concern is somewhat minimal.
If the fallback is explicitly intended to be used as described then the diagnostic would only ever apply when passing the
Customers aren't writing their own lock types, they're relying on the myriad of types provided by the BCL, none of which are |
I think existing locks could be "updated" by providing an extension that returns a light wrapper that implements the new API. That would be required to work with lock (rwl.ReaderLock()) { }
lock (rwl.WriterLock()) { }
lock (rwl.UpgradeableReaderLock()) { } And perhaps even do the same thing with lock (obj.MonitorLock()) { } |
Yes, although that suffers from the problem in that if you forget to bring that extension method into scope you'll accidentally fallback to Monitor again. Feels like you'd want analyzers that would also detect Monitor locks against common lock classes to warn of such.
I kind of wish we could deprecate the current behavior with |
Just add them directly as members to those types, then. The analyzer should warn against any lock that uses the old style Monitor, whether those are implemented as extensions or type members either way.
Absolutely!
Why not? It seems like most people here are worried about compatibility issues, not that they don't want the pattern updated. I think an analyzer warning should fix that. Even if a |
That'd be up to the BCL team, and that would potentially change the meaning of existing code.
If the type already implemented the lock pattern then the fallback to Monitor would no longer be a risk.
Right, that due to the existing Monitor behavior that any accidental miss of the lock pattern would still be valid code that would compile, leading to subtle bugs at runtime. I'd be game for a more explicit approach, such as the one that you're describing, which would discourage the current behavior. |
Another thing, could the using (var key = _lock.EnterLockWithHolder())
{
// do something with key
} Maybe something like this? lock (_lock; var key)
{
// do something with key
} |
Disagree. As I mentioned, this diagnostic would fire when you cast a lock instance into an interface it implemented. I would not classify that as somewhere it probably shouldn't go. The author implemented the interface, it should be a valid target. Further such a diagnostic can't even be completely accurate. It won't catch cases like the following: object SneakyCast<T>(T t) => (object)t;
It's fairly trivial to wrap the BCL types in a |
The customers sophisticated enough to know to do that are not the customers I would be worried about. |
I think even if the incident rate of misusing |
@jaredpar Why would you warn on a cast? Just warn when the
@TahirAhmadov I would be fine with a new keyword. But |
@TahirAhmadov No, I'd say we definitely don't want the new type's tl;dr - if the namespace is in scope:
if the namespace is not in scope:
|
OK I think I see what you mean. It's a little difficult to wrap my head around it because it's a little complicated :) |
@TahirAhmadov I guess it is kind of like a language dialect, but done in a way that the behavior doesn't change, only the warning, and built purely on existing language constructs. It'd certainly be possible to do with an analyzer, but that presumes that every developer is aware of the analyzer and knows to turn it on; my approach will kind of automatically activate the analyzer based on the heuristic of having the new namespace in scope, which is a pretty good proxy for "code that's actively using the new locking system", and turns out to give quite a lot of fine grained control of where the analyzer should be active and how severe the errors should be both globally and on a per-file basis, without needing to add all that flexibility into the analyzer or remember to activate the analyzer in the first place. |
https://sharplab.io/#gist:1dee0bd0e21c175c239d6cdf9658a40f |
|
The type in question here is a |
Basically a lock that looks like this I guess : let mutable lock = 0
let myFunction () =
if Interlocked.CompareExchange(&lock, 1, lock) = 0 then
....
Interlocked.CompareExchange(&lock, 0, lock)
Why do we use ref struct for locks they live in a thread only |
When is this expected to come out of preview? |
In C# 13 and .NET 9. |
To be specific, the new The actual proposal - a lock pattern - is not coming, at least not in C# 13 |
Is there any reason why we still use the I assume when |
Yes. The lock statement clearly and concisely indicates the intent. It is a region of mutual exclusion. A using block does not indicate that.
We do not think so. The language has supported locking since v1. It is a widely used construct. We see no reason to move away from that
The language supported both |
A using (lock.BeginMutualExclusionSection())
{
...
} Would be even more explicit and obvious to me than a custom But I guess this aspect is still subjective, so fair enough.
Fair enough.
Just because something is supported since X, doesn't mean it is inherently good.
Just because something is widely used doesn't mean it is good or that it should never be reconsidered.
Again, fair enough... to me it's a shame, as it makes the language unnecessarily more complex/less orthogonal. It introduces a special case for something that clearly didn't need it.
Oh, my bad on this one. I thought the using block was introduced after C# 1. I stand corrected. In that case, looks like we just missed an opportunity to avoid introducing the |
That's more complex and less clear to me than just the
I disagree. This is a good part of hte langauge that people have been using for 25 years. Removing support is just adding friction and complexity. Foolish consistencies, hobgoblins, and all that.
We didn't miss an opportunity. The possibility of using |
We didn't need There are lots of special one-off lang features that are just sugar over other things. The vast majority of hte language is actually just that. Yes, we could go the route of other languages and whittle that away to a core kernel that you use for everything. But we actually view that as a negative. We think this features and sugar are the value and niceness that makes our language more pleasant and more desirable for many users. |
I would also point out that |
Isn't it just a matter of convention? Some will prefer a more technical focus "I want |
I don't quite understand the meaning of such a construction. Essentially, the compiler will just replace using with lock, but a lot of additional information is needed. |
Lock statement pattern
Lock
object feature roslyn#71716(This proposal comes from @kouvel. I've populated this issue primarily with text he wrote in a separate document and augmented it with a few more details.)
Summary
Enable types to define custom behaviors for entering and exiting a lock when an instance of the type is used with the C# “lock” keyword.
Motivation
.NET 9 is likely to introduce a new dedicated System.Threading.Lock type. Along with other custom locks, the presence of the
lock
keyword in C# might lead developers to think they can use it in conjunction with this new type, but doing so won't actually lock according to the semantics of the lock type and would instead treat it as any arbitrary object for use withMonitor
.Detailed design
Example
A type would expose the following to match the proposed pattern:
EnterLockScope()
would enter the lock andDispose()
would exit the lock. The behaviors of entering and exiting the lock are defined by the type.The
ILockPattern
interface is a marker interface that indicates that usage of values of this type with thelock
keyword would override the normal code generation for arbitrary objects. Instead, the compiler would lower thelock
to use the lock pattern, e.g.:would be lowered to the equivalent of:
Lock pattern and behavior details
Consider a type
L
(Lock
in this example) that may be used with thelock
keyword. IfL
matches the lock pattern, it would meet all of the following criteria:L
implements interfaceILockPattern
L
has an accessible instance methodS EnterLockScope()
that returns a value of typeS
(Lock.Scope
in this example). The method must be at least as visible asL
. Extension methods don't qualify for the pattern.S
qualifies for use with theusing
keywordA marker interface
ILockPattern
is used to opt into the behaviors below, including through inheritance, and so thatS
may be defined by the user (for instance, as aref struct
). For a typeL
that implements interfaceILockPattern
:L
does not fully match the lock pattern, it would result in an errorS
may not be used in the context, it would result in an errorL
is implicitly or explicitly casted to another type that does not match the lock pattern (including generic types), it would result in a warningthis
to a base type when calling an inherited methodMonitor
with values of typeL
that may be masked under a different type and used with thelock
keyword, such as with casts to base types, interfaces, etc.L
is used with thelock
keyword, or a value of typeS
is used with theusing
keyword, and the block contains anawait
, it would result in an errorMonitor
andLock
have thread affinity.SpinLock
is optionally thread-affinitized. It can opt into the pattern, but then usage of it with thelock
orusing
keywords would still disallow awaits inside the block statically.SpinLock example
System.Threading.SpinLock
(a struct) could expose such a holder:and then similarly be usable with
lock
whereas today as a struct it's not. When a variable of a struct type that matches the lock pattern is used with thelock
keyword, it would be used by reference. Note the reference toSpinLock
in the previous section.Drawbacks
lock(objectsOfThatType)
in existing code, and in ways that might go unnoticed due to the general unobservability of locks (when things are working). This also means if System.Threading.Lock is introduced before the language support, it likely couldn't later participate.L1
that exposes the type is updated to opt into the lock pattern, another libraryL2
that referencesL1
and was compiled for the previous version ofL1
would have different locking behavior until it is recompiled with the updated version ofL1
.using
instead oflock
and writing out the name of the enter method manually (i.e. writing out the lowered code in the first example).ILockPattern
itself doesn't match the lock pattern, though an interface could be created to match the lock pattern.Alternatives
AttributeUsageAttribute.Inherited = true
could be used for an attribute, it doesn't seem to work with interfaces.using lock(x)
or other alternatives instead, which would eliminate any possibility of misusing values of the type withMonitor
.ref
keyword for certain uses, e.g. in the SpinLock example.Unresolved questions
Design meetings
https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-05-01.md#lock-statement-improvements
https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-10-16.md#lock-statement-pattern
https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-12-04.md#lock-statement-pattern
The text was updated successfully, but these errors were encountered: