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

Methods that throw PlatformNotSupportedException are not documented #3825

Closed
DoctaJonez opened this issue Nov 29, 2017 · 12 comments
Closed

Methods that throw PlatformNotSupportedException are not documented #3825

DoctaJonez opened this issue Nov 29, 2017 · 12 comments
Labels
⌚ Not Triaged Not triaged

Comments

@DoctaJonez
Copy link

DoctaJonez commented Nov 29, 2017

I've been working on a project, and have been attempting to make it compatible with .Net Framework and .Net Core. When attempting to run it in .Net Core I found that some methods in the .Net API are not supported.

One such example is Thread.Abort. When you try to call Thread.Abort in a .Net Core 2.0 app, you get the following message:

System.PlatformNotSupportedException: 'Thread abort is not supported on this platform.'

You can see the Thread.Abort documentation for .Net Core 2.0 here: https://docs.microsoft.com/en-gb/dotnet/api/system.threading.thread.abort?view=netcore-2.0

As you can see, the documentation is not accurate, there is no mention of this method not being supported.

I would like to update the documentation to accurately reflect the methods that are not supported in .Net Core. I've downloaded the .Net Core source code and found the following methods in Thread.cs that aren't supported:

    public void Abort()
    {
        throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadAbort);
    }

    public void Abort(object stateInfo)
    {
        throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadAbort);
    }

    public static void ResetAbort()
    {
        throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadAbort);
    }

    [ObsoleteAttribute("Thread.Suspend has been deprecated.  Please use other classes in System.Threading, such as Monitor, Mutex, Event, and Semaphore, to synchronize Threads or protect resources.  http://go.microsoft.com/fwlink/?linkid=14202", false)]
    public void Suspend()
    {
        throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadSuspend);
    }

    [ObsoleteAttribute("Thread.Resume has been deprecated.  Please use other classes in System.Threading, such as Monitor, Mutex, Event, and Semaphore, to synchronize Threads or protect resources.  http://go.microsoft.com/fwlink/?linkid=14202", false)]
    public void Resume()
    {
        throw new PlatformNotSupportedException(SR.PlatformNotSupported_ThreadSuspend);
    }

This is my first contribution to the dotnet docs, so I looked at the guidelines, and step 1 says to create an issue here, so we can discuss the changes and agree how to proceed.

I was thinking of starting by updating the docs for the Thread class, to state the Abort, ResetAbort, Suspend and Resume methods are not supported in .Net Core, and perhaps specify that they throw PlatformNotSupportedException. How would I do this, only when the documentation is being viewed for .Net Core 2.0? Is there a guide on how to make platform specific changes to the documentation?

After I've successfully update the docs for the Thread class, I was planning on finding all other callsites for PlatformNotSupportedException and work on updating the rest of the documentation accordingly.

Thanks for any help you can offer.

@DoctaJonez
Copy link
Author

An example of what I'd like to do would be to change this:

Abort()
Raises a ThreadAbortException in the thread on which it is invoked, to begin the process of terminating the thread. Calling this method usually terminates the thread.

C#
public void Abort ();

Exceptions
SecurityException
The caller does not have the required permission.
ThreadStateException
The thread that is being aborted is currently suspended.

To something like this:

Abort()
This method is not supported and will throw System.PlatformNotSupportedException.

C#
public void Abort ();

Exceptions
PlatformNotSupportedException
This method is not supported.

@rpetrusha
Copy link
Contributor

rpetrusha commented Nov 29, 2017

@DoctaJonez, there is an existing issue, #2934, about PlatformNotSupportedException in general, but it would be great if you could document the exceptions for the Thread class.

All .NET implementations, including .NET Core and the .NET Framework, now share a single documentation set. That means that the member descriptions cannot be changed to note the exception. Instead, the exceptions should be noted in the Exceptions section of the member documentation, with a description of the appropriate exception condition. Since the documentation is in ECMAXML format, this involves adding a tag like the following after the Remarks section in the member documentation, something like this:

<exception cref="T:System.PlatformNotSupportedException">.NET Core only: This member is not supported.</exception>

The only question offhand is whether the exception is thrown on all platforms supported by .NET Core or only on some (particularly Linux and MacOS). The source code suggests it's thrown on all, but I'm not completely certain. @jkotas or @weshaggard, can you confirm?

Once we confirm whether the exception is thrown on all platforms supported by .NET Core or only on individual platforms, please let us know whether you're still interested in documenting the exceptions for the Thread class.

//cc @jkotas @weshaggard

@mairaw
Copy link
Contributor

mairaw commented Nov 29, 2017

I think you're right @rpetrusha. The wiki says that it throws for all platforms:
https://github.com/dotnet/corefx/wiki/ApiCompat#systemthreading

/cc @pjanotti

@jkotas
Copy link
Member

jkotas commented Nov 29, 2017

The wiki says that it throws for all platform

Right, Thread.Abort throws PNSE on all .NET Core platforms.

BTW: The platform-compat tool
has auto-generated per-platform data about methods that PNSE in .NET Core: https://raw.githubusercontent.com/dotnet/platform-compat/master/etc/exceptions.csv

cc @terrajobst

@DoctaJonez
Copy link
Author

@rpetrusha thank you for the reply. Yes I'm happy to document the exceptions for the Thread class. I'm happy to work through the compatibility spreadsheet and document all of the methods that are unsupported on all 3 platforms (Win, Mac, Linux), as these will be the most straightforward to do.

How would we document the others that are only supported on selected platforms?

Like this?

<exception cref="T:System.PlatformNotSupportedException">.NET Core on MacOS and Linux only: This member is not supported.</exception>

It seems a bit clunky, is there a better way of doing it?

@DoctaJonez
Copy link
Author

@rpetrusha I'd also like to know if there's a better way of documenting that the method isn't supported rather than just putting an exception into the docs with an addendum. It's a little too subtle for my liking.

If we haven't got the capability of showing appropriate documentation for the selected .Net version, why does the user have the option of filtering by .Net version on the API site? It gives the false impression that this documentation is relevant to the selected .Net version.

image

https://docs.microsoft.com/en-gb/dotnet/api/system.threading.thread.abort?view=netcore-2.0

@rpetrusha
Copy link
Contributor

rpetrusha commented Nov 30, 2017

The basic problem, @DoctaJonez, is that while the method isn't supported, it is present; the version selector determines whether a type or member is present on a particular .NET version, but it has no way of knowing what the member implementation is. It may be that this is something that the API browser can eventually address, especially since there are now analyzers that detect methods that throw PlatformNotImplementException. But for the foreseeable future, the exception information will have to indicate the individual platforms that throw.

I'd phrase the exception message somewhat differently:

<exception cref="T:System.PlatformNotSupportedException">.NET Core only: This member is not supported on the macOS and Linux platforms.</exception>

//cc @dend

edit by @mairaw: updated MacOS to macOS

@DoctaJonez
Copy link
Author

@rpetrusha thank you for the explanation.

I will work on a pull request to add exceptions as per your example. I will start with a small one for Thread, and then look at other types after getting the Thread PR approved.

@DoctaJonez
Copy link
Author

@rpetrusha, I have created the following pull request with the changes for the Thread class.

#3852

@TylerBrinkley
Copy link
Contributor

HttpClientHandler.SslProtocols and HttpClientHandler.CheckCertificateRevocationList both throw PlatformNotSupportedException in .NET 4.7.1 and thus should be documented as such.

@mairaw
Copy link
Contributor

mairaw commented Mar 22, 2018

@TylerBrinkley can you open a new issue for us to investigate this? Or if you prefer to just go ahead and open a PR, we welcome contributions. Thanks!

@nareshravlani
Copy link

check out my answer here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌚ Not Triaged Not triaged
Projects
None yet
Development

No branches or pull requests

8 participants