Update list of Exceptions for Directory.GetParent

[mriehm]

Update list of Exceptions for Directory.GetParent

Summary

Removed IOException, UnauthorizedAccessException, and DirectoryNotFoundException from the list of Exceptions for Directory.GetParent(), since they cannot actually get thrown by Directory.GetParent().

Also, added SecurityException and NotSupportedException since they can get thrown by Directory.GetParent(), but were not listed in the documentation before.

Details

Listing those Exceptions that I am now removing was confusing because they implied that Directory.GetParent() performs IO operations, which has performance and reliability implications, but Directory.GetParent() does not actually perform IO operations.

I confirmed that these Exceptions are not thrown by testing the function, and also by examining the referencesource code. According to the source, Directory.GetParent() is exactly equal to calling Path.GetFullPath(), followed by Path.GetDirectoryName(), followed by new DirectoryInfo(), none of which list in their documentation the three Exceptions that I am removing.
Conversely, SecurityException and NotSupportedException are listed in the documentation for Path.GetFullPath(), but were not listed for Directory.GetParent, so I am now adding them to Directory.GetParent.

Suggested Reviewers

[BillWagner]

Thanks for submitting this fix @mriehm We appreciate it.

I've reviewed the changes, and I would be ready to approve it. I'm checking with @JeremyKuhne or @pjanotti to review these as well for technical accuracy across all platforms.

Once I hear back from the one of them, I'll merge this.

I think both of them are out for the holidays, so it could be a bit of a wait. I'm trying to find someone else that can take a look in the meantime.

Thanks again.

[mairaw]

Friendly ping after the holidays @JeremyKuhne and @pjanotti. Can one of you review this PR?

[pjanotti]

That seems correct, notice that the set of exceptions is different between CoreFX and desktop. I believe there is no SecurityException in this case on CoreFX. @JeremyKuhne is more familiar with the code.

[JeremyKuhne]

As @pjanotti called out, the SecurityException is only potentially thrown on desktop (CAS).

All of the others can still be thrown. The fact that they aren't documented in other places is a bug. Anything that calls GetFullPath can end up calling our generic Win32 error handler. On desktop: http://referencesource.microsoft.com/#mscorlib/system/io/__error.cs,135 from http://referencesource.microsoft.com/#mscorlib/system/io/path.cs,916

[JeremyKuhne]

(Sorry about the long delay- I was on an extended vacation this year.)

[mairaw]

No worries @JeremyKuhne. Thanks for the review guys!

So this is what we should do in this PR:

• Keep the exceptions that are being removed (IOException, UnauthorizedAccessException, DirectoryNotFoundException)
• Note that SecurityException is only thrown for .NET Framework

Correct @JeremyKuhne?

@mriehm let us know if you wanna make the changes. Otherwise, we can help. Thanks!

I'll create an issue for us to take a look at the exceptions for the other places then.

[JeremyKuhne]

@mairaw Yes, that is correct. As a general rule, as most everything calls GetFullPath() you're going to go through the WinIOError I linked to (there is an equivalent API in Core). That means FileNotFound, DirectoryNotFound, UnauthorizedAccess, IOException, PathTooLong, DriveNotFound, and OperationCancelled are always possible. GetFullPathName doesn't document what errors it gives back.

I don't know if there is a good place to put a general "catching IO errors" blurb. If you want to be truly safe, you need to catch:

• System.IO.IOException (includes all of the DirectoryNotFound, FileNotFound, etc. Generic IO errors get IOException itself)
• System.UnauthorizedAccessException
• System.OperationCancelledException
• System.ArgumentException (for bad path formats, argument null, etc.)
• System.Security.SecurityException (CAS: only on desktop and only in limited trust)

[mriehm]

I suppose that it is true that GetFullPathName doesn't document all potential errors, so we can't say for sure that those Exceptions won't get thrown. On the other hand, I'm not sure that it is necessarily more helpful to over-document than under-document the list of Exceptions. I think that my preference would be only to document those for which we can also provide a description of the situation in which that Exception would occur, so I can think of how I should write my code to avoid it or recover from it. If it's an error that I have no idea what could have caused it (e.g. OperationCancelledException from Directory.GetParent()), then to me it is no different from a generic Exception and I will catch it in a top-level Exception handler to log it to investigate later.

Or for instance, since DirectoryNotFoundException is currently documented for Directory.GetParent(), then I might write my code assuming that any path returned from Directory.GetParent() is guaranteed to exist. However, testing Directory.GetParent() shows that is not the case (at least for the specific scenario I tested on my particular machine & OS). Also, the GetFullPathName documentation does state "This function does not verify that the resulting path and file name are valid, or that they see an existing file on the associated volume."

My original motivation for this PR is that I develop a web server which performs a lot of network file operations. I would like to be able to tell which of the APIs that I use perform IO/network operations so that I can improve performance by reducing the frequency of those, and also focus on improving my error handling around them. Based on the current documentation for Directory.GetParent(), it initially seemed like one of the APIs that I should focus on, but upon testing, that does not seem to be the case. However, perhaps I should start using Path.GetDirectoryName() instead, just to be safe and avoid the black box of GetFullPathName.

For those reasons, I don't think that I'm going to propose changes to document Exceptions for other APIs that I don't know for sure can actually throw them. However, I will understand if you do not approve this PR based on the perspective that we don't know for sure that Directory.GetParent() will never throw these Exceptions.

Thanks for your time.

[rpetrusha]

Thanks for submitting this PR, @mriehm. Although I'm going to close it, it was really important in helping us to identify a number of issues with exception information in the file I/O documentation, and you've really made a very valuable contribution in improving the docs that far transcends a single PR. In particular:

• We're often missing full exception information for methods that call into GetFullPath.
• Our description of PathTooLongException is no longer accurate.
• SecurityException is thrown by these methods in the .NET Framework only.

I've opened PR #4283 to update exception information for Directory.GetParent. I've also opened two issues, 4284 and 4285, to address exception information in other file I/O members.

@