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

Do not hold SpinLock in fields marked as readonly #33773

Open
terrajobst opened this issue Mar 19, 2020 · 6 comments
Open

Do not hold SpinLock in fields marked as readonly #33773

terrajobst opened this issue Mar 19, 2020 · 6 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Threading code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@terrajobst
Copy link
Member

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

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Threading code-analyzer Marks an issue that suggests a Roslyn analyzer labels Mar 19, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 19, 2020
@stephentoub
Copy link
Member

Closely related / duplicative: dotnet/roslyn-analyzers#2811

@jeffhandley jeffhandley added this to the 5.0 milestone Mar 20, 2020
@jeffhandley jeffhandley added the code-fixer Marks an issue that suggests a Roslyn code fixer label Mar 21, 2020
@jeffhandley
Copy link
Member

Estimates:

  • Analyzer: Small
  • Fixer: Small

@terrajobst terrajobst removed the untriaged New issue has not been triaged by the area owner label Mar 21, 2020
@jeffhandley jeffhandley added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Mar 23, 2020
@terrajobst terrajobst removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 23, 2020
@terrajobst
Copy link
Member Author

Seems related to #33772

@carlossanlop
Copy link
Member

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

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

@sharwell
Copy link
Member

@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
Copy link

JC3 commented May 23, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Threading code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

No branches or pull requests

7 participants