Skip to content
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

Add native lock speclet #7849

Merged
merged 6 commits into from
Jan 26, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions proposals/native-lock.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
# Native lock

## Summary
[summary]: #summary
jjonescz marked this conversation as resolved.
Show resolved Hide resolved

Special-case how `System.Threading.Lock` interacts with the `lock` keyword (calling its `EnterLockScope` method under the hood).
Add static analysis warnings to prevent accidental misuse of the type where possible.

## Motivation
[motivation]: #motivation

.NET 9 is introducing a new [`System.Threading.Lock` type](https://github.com/dotnet/runtime/issues/34812)
as a better alternative to existing monitor-based locking.
The presence of the `lock` keyword in C# might lead developers to think they can use it with this new type.
Doing so wouldn't lock according to the semantics of this type but would instead treat it as any other object and would use monitor-based locking.

## Detailed design
[design]: #detailed-design

Semantics of the lock statement ([§13.13](https://github.com/dotnet/csharpstandard/blob/9af5bdaa7af535f34fbb7923e5406e01db8489f7/standard/statements.md#1313-the-lock-statement))
are changed to special-case the `System.Threading.Lock` type:

> A `lock` statement of the form `lock (x) { ... }`
>
> 1. **where `x` is an expression of type `System.Threading.Lock`, is precisely equivalent to:**
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
> ```cs
> using (x.EnterLockScope())
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
> {
> ...
> }
> ```
> **and `System.Threading.Lock` must have the following shape:**
> ```cs
> namespace System.Threading
> {
> public sealed class Lock
> {
> public Scope EnterLockScope();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I updated the proposal in the Lock API review to rename this method back to EnterScope, as the Lock in the name is redundant given the type name and it's not being suggested for use as a general pattern anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not being suggested for use as a general pattern anymore

I think that's still considered as future work (when ref structs can implement interfaces). Wouldn't it be better to keep the name EnterLockScope to make the transition to the generic pattern smoother?

Copy link
Member

@kouvel kouvel Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was under the impression that it may or may not be expanded to a general lock pattern, and figured we might as well use an appropriate name for now. If a general pattern is exposed in the future it may also be more useful for it to be usable by other existing types, which may also involve new syntax or some such. At that time if it's necessary to add EnterLockScope again we could, and perhaps explicitly implement it if it's an interface member to hide it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I assume this can still change in API review. Do you know when the API will be approved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I don't think it's been scheduled yet, will add you when I find out

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec should reflect our best guess for what the API will be. If API review changes the name we will just update the spec.

Based on previous experiences though we should put the name @kouvel least wants in the spec. Cause inevitably whatever we pick in the initial spec always changes 😉

>
> public ref struct Scope
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
> {
> public void Dispose();
> }
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
> }
> }
> ```
> 2. where `x` is an expression of a *reference_type*, is precisely equivalent to: [...]

Note that the shape might not be fully checked (e.g., there will be no errors nor warnings if the `Lock` type is not `sealed`),
but the feature might not work as expected (e.g., there will be no warnings when converting `Lock` to a derived type,
since the feature assumes there are no derived types).

Additionally, new warnings are added to implicit reference conversions ([§10.2.8](https://github.com/dotnet/csharpstandard/blob/9af5bdaa7af535f34fbb7923e5406e01db8489f7/standard/conversions.md#1028-implicit-reference-conversions))
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
when upcasting the `System.Threading.Lock` type:

> The implicit reference conversions are:
>
> - From any *reference_type* to `object` and `dynamic`.
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
> - **A warning is reported when the *reference_type* is known to be `System.Threading.Lock`.**
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
> - From any *class_type* `S` to any *class_type* `T`, provided `S` is derived from `T`.
> - **A warning is reported when `S` is known to be `System.Threading.Lock`.**
> - From any *class_type* `S` to any *interface_type* `T`, provided `S` implements `T`.
> - **A warning is reported when `S` is known to be `System.Threading.Lock`.**
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
> - [...]

```cs
object l = new System.Threading.Lock(); // warning
lock (l) { } // monitor-based locking is used here
```

Note that this warning occurs even for equivalent explicit conversions.
To escape out of the warning and force use of monitor-based locking, one can use
- the usual warning suppression means (`#pragma warning disable`),
- `Monitor` APIs directly,
- indirect casting like `object AsObject<T>(T l) => (object)l;`.

## Alternatives
[alternatives]: #alternatives

- Support a general pattern that other types can also use to interact with the `lock` keyword.
This is a future work that might be implemented when `ref struct`s can participate in generics.
Discussed in [LDM 2023-12-04](https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-12-04.md#lock-statement-pattern).

- To avoid ambiguity between the existing monitor-based locking and the new `Lock` (or pattern in the future), we could:
- Introduce a new syntax instead of reusing the existing `lock` statement.
- Require the new lock types to be `struct`s (since the existing `lock` disallows value types).
There could be problems with default constructors and copying if the structs have lazy initialization.

- The codegen could be hardened against thread aborts (which are themselves obsoleted).

- We could warn also when `Lock` is passed as a type parameter, because locking on a type parameter always uses monitor-based locking:

```cs
M(new Lock()); // could warn here

void M<T>(T x) // (specifying `where T : Lock` makes no difference)
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
{
lock (x) { } // because this uses Monitor
}
```

However, that would cause warnings when storing `Lock`s in a list which is undesirable:

```cs
List<Lock> list = new();
list.Add(new Lock()); // would warn here
jjonescz marked this conversation as resolved.
Show resolved Hide resolved
```

## Unresolved questions
[unresolved]: #unresolved-questions

- Can we allow the new `lock` statement in async methods?
Since `await` is disallowed inside the `lock`, this would be safe.
Currently, since `lock` is lowered to `using` with a `ref struct` as the resource, this results in a compile-time error.
The workaround is to extract the `lock` into a separate non-async method.

## Design meetings

- [LDM 2023-05-01](https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-05-01.md#lock-statement-improvements): initial decision to support a `lock` pattern
- [LDM 2023-10-16](https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-10-16.md#lock-statement-pattern): triaged into the working set for .NET 9
- [LDM 2023-12-04](https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-12-04.md#lock-statement-pattern): rejected the general pattern, accepted only special-casing the `Lock` type + adding static analysis warnings