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.

Sign up for GitHub

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

Jump to bottom

Directory.Delete: prefer DirectoryNotFoundException over UnauthorizedAccess IOException. #62396

Merged
merged 2 commits into from
Dec 16, 2021
Merged

Directory.Delete: prefer DirectoryNotFoundException over UnauthorizedAccess IOException. #62396

merged 2 commits into from
Dec 16, 2021

Conversation

tmds
Copy link
Member

@tmds tmds commented Dec 4, 2021

Fixes #62034.
Regression in #59520.

@danmoseley @carlossanlop @adamsitnik ptal.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 4, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO and removed community-contribution Indicates that the PR has been added by a community member labels Dec 4, 2021
@ghost
Copy link

ghost commented Dec 4, 2021

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #62034.
Regression in #59520.

@danmoseley @carlossanlop @adamsitnik ptal.

Author: tmds
Assignees: -
Labels:

area-System.IO
Milestone: -

@tmds
Copy link
Member Author

tmds commented Dec 4, 2021

Can you trigger the outerloop/elevated testing?

carlossanlop
carlossanlop approved these changes Dec 4, 2021
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your help, @tmds.

src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs Outdated
Comment on lines 499 to 507
case Interop.Error.EROFS:
// Prefer throwing DirectoryNotFoundException over UnauthorizedAccess IOException.
if (topLevel && !DirectoryExists(fullPath, out Interop.ErrorInfo existErr) && existErr.Error == Interop.Error.ENOENT)
{
throw Interop.GetExceptionForIoErrno(Interop.Error.ENOENT.Info(), fullPath, isDirectory: true);
}
throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, fullPath));
case Interop.Error.EISDIR:
throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, fullPath)); // match Win32 exception
throw new IOException(SR.Format(SR.UnauthorizedAccess_IODenied_Path, fullPath));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting case. The fix looks good but I have a few questions.

So you found that when EROFS is caught, there is a special case where the actual problem was that the top level directory didn't exist. Correct? Do we know what is the actual reason why EROFS is being returned and not something else?

Do you mind changing that comment to something that would describe that we have two different possible cases here? That would be more meaningful.

In the original issue, Dan left a comment asking if the elevated test was not being executed in the PR CI. Can you please verify that the test was indeed executed in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you found that when EROFS is caught, there is a special case where the actual problem was that the top level directory didn't exist. Correct? Do we know what is the actual reason why EROFS is being returned and not something else?

Previously we were always checking if the directory existed before trying to remove it.

One of the test verified that DirectoryNotFoundException was thrown in favor of the UnauthorizedAccess_IODenied_Path (on EROFS). And I regressed it in #59520.

In the original issue, Dan left a comment asking if the elevated test was not being executed in the PR CI. Can you please verify that the test was indeed executed in this PR?

I'll verify the test passes.

@carlossanlop
Copy link
Member

carlossanlop commented Dec 4, 2021 •
edited
Loading

Can you trigger the outerloop/elevated testing?

@safern I've never done this. Do you know how to trigger it?

Edit: Nevermind. Found the instructions 😃

@carlossanlop
Copy link
Member

/azp list

@azure-pipelines

This comment has been minimized.

Sign in to view
@carlossanlop
Copy link
Member

/azp runtime-libraries-coreclr outerloop

@azure-pipelines

This comment has been minimized.

Sign in to view
@carlossanlop
Copy link
Member

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik adamsitnik added this to the 7.0.0 milestone Dec 6, 2021
@danmoseley
Copy link
Member

@safern I see this failure --

 Check failure on line 221 in .dotnet/sdk/6.0.100/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets 

@azure-pipelinesazure-pipelines
/ runtime-dev-innerloop
.dotnet/sdk/6.0.100/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets#L221
.dotnet/sdk/6.0.100/Sdks/NuGet.Build.Tasks.Pack/build/NuGet.Build.Tasks.Pack.targets(221,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) The process cannot access the file '/__w/1/s/artifacts/packages/Debug/Shipping/runtime.native.System.IO.Ports.7.0.0-ci.nupkg' because it is being used by another processc

@safern safern mentioned this pull request Dec 7, 2021
Closed
@safern
Copy link
Member

safern commented Dec 7, 2021

Thanks, @danmoseley, I just logged: #62471 and retried the leg. I'll try to repro locally and fix that issue.

@danmoseley danmoseley mentioned this pull request Dec 7, 2021
Merged
@tmds
Copy link
Member Author

tmds commented Dec 10, 2021

@carlossanlop @danmoseley @jozkee I'll be making some changes. No need to do another review yet.

@tmds tmds force-pushed the rmdir_access_notfound branch from 8318ce5 to 6a4ebc7 Compare December 12, 2021 00:26
@tmds
Copy link
Member Author

tmds commented Dec 12, 2021

This is up for review. I verified Unix_NotFoundDirectory_ReadOnlyVolume (and other privileged+outerloop) passes.

adamsitnik
adamsitnik approved these changes Dec 16, 2021
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @tmds!

@adamsitnik adamsitnik merged commit a4f169c into dotnet:main Dec 16, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 15, 2022
@jeffhandley jeffhandley added the community-contribution Indicates that the PR has been added by a community member label Aug 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danmoseley danmoseley danmoseley left review comments

@jozkee jozkee jozkee left review comments

@carlossanlop carlossanlop carlossanlop approved these changes

@adamsitnik adamsitnik adamsitnik approved these changes

Assignees
No one assigned
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

Test failure System.IO.Tests.Directory_Delete_str_bool.Unix_NotFoundDirectory_ReadOnlyVolume
7 participants
@tmds @carlossanlop @danmoseley @safern @adamsitnik @jozkee @jeffhandley