search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.25k stars 4.73k forks source link

Do not hold SpinLock in fields marked as readonly #33773

Open terrajobst opened 4 years ago

terrajobst commented 4 years ago

SpinLock is a mutable struct, meant only for advanced scenarios. Accidentally making a SpinLock field readonly can result in silent but significant problems, as any mutations to the instance (e.g. Enter, Exit) will be done on a compiler-generated copy and thus be ignored, making the lock an expensive nop. (It might make sense to extend this analyzer to additional mutable struct types where storing them in a readonly field is likely a bug, e.g. GCHandle.)

Category: Reliability

stephentoub commented 4 years ago

Closely related / duplicative: https://github.com/dotnet/roslyn-analyzers/issues/2811

jeffhandley commented 4 years ago

Estimates:

terrajobst commented 4 years ago

Seems related to #33772

carlossanlop commented 3 years ago

This analyzer has a long discussion here already: https://github.com/dotnet/roslyn-analyzers/issues/2811 which @mavasani already approved. There's even an PR out trying to get it fixed: https://github.com/dotnet/roslyn-analyzers/pull/2831

Do we want this issue to go through the dotnet/runtime API review process anyway?

sharwell commented 3 years ago

@carlossanlop I think a review would be good. The proposed rule only indirectly addresses the underlying problem, and we have a large number of related cases in #50389.

JC3 commented 3 years ago

Along those lines, storing SpinLock and other such values in Nullable, e.g.:

SpinLock? example;

That'd be covered implicitly with non-read-only constraints, but should probably have a modified error description so that a warning / failure message makes reasonable sense.