-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[wasm] Mark System.Threading.Thread APIs unsupported on Browser #41892
[wasm] Mark System.Threading.Thread APIs unsupported on Browser #41892
Conversation
Tagging subscribers to this area: @safern, @ViktorHofer |
src/libraries/System.Private.CoreLib/src/System/Threading/Thread.cs
Outdated
Show resolved
Hide resolved
@@ -189,6 +190,7 @@ public void Abort(object? stateInfo) | |||
throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadAbort); | |||
} | |||
|
|||
[UnsupportedOSPlatform("browser")] | |||
public static void ResetAbort() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this throws PNSE everywhere, is there a way we can mark it as such?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if its better to stick with the convention of marking it Obsolete. I wasn't sure if a docs issue should be linked in the parameters of Obsolete
. It looks like it was brought up in dotnet/docs#3825.
Another option that was brought up was to mark this as [UnsupportedOSPlatform("none")]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeffhandley @buyaa-n What do y'all think about [UnsupportedOSPlatform("none")]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the analyzer that would be treated like unsupported on a platform named "none", there is no generic naming for all platforms CC @terrajobst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Obsolete]
would be a better approach than to fake that with an [UnsupportedOSPlatform]
or [SupportedOSPlatform]
attribute. I recommend we omit this annotation here and file a 6.0.0 issue to mark this API as Obsolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at a stale page a moment ago. I think we'll have to wait to mark this as Obsolete
until 6.0.0 since it's technically a breaking change. We'll want to consider using a custom diagnostic id for the obsoletion too and add it to the list of obsoletions, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Jeff. We shouldn't annotate this API. We should obsolete, but not for .NET 5.
Besides |
Contributes to #41087
Omitted CompressedStack because it pertains to Serialization infrastructure.
SetApartmentState is windows specific
Abort, Suspend, Resume are obsolete