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

Make PhysicalFileProvider polling more resilient to IO exceptions #72462

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

jozkee
Copy link
Member

@jozkee jozkee commented Jul 19, 2022

Fixes #71003
Fixes #56810

@jozkee jozkee added this to the 7.0.0 milestone Jul 19, 2022
@jozkee jozkee self-assigned this Jul 19, 2022
@ghost
Copy link

ghost commented Jul 19, 2022

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

Issue Details

Fixes #71003
Fixes #56810

Author: Jozkee
Assignees: Jozkee
Labels:

area-Extensions-FileSystem

Milestone: 7.0.0

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

Should we catch any exception that could be IO related? Or is the API we are calling only going to throw these two?

Reasoning: the app has little or no control over whether examining arbitrary files on disk leads to an IO exception. It could be a corrupted disk, for a start. The app can't filter these out and can't catch them - it will just terminate. Is that a better experience than catching them?

If we catch all IO exceptions, you can use this as a guide:
https://github.com/dotnet/msbuild/blob/c34eb484f1ddf0f9e1940c620b1eff169433f5a1/src/Shared/ExceptionHandling.cs#L130-L145

@jozkee
Copy link
Member Author

jozkee commented Jul 19, 2022

I'm certain the API won't throw these ones:

e is NotSupportedException
|| (e is ArgumentException && !(e is ArgumentNullException))
|| e is SecurityException

All exceptions come form Win32Marshal.GetExceptionForWin32Error. which can return Unauthorized, several exceptions derived from IOException, and OperationCanceledException, which I don't think happens for this case.
https://source.dot.net/#System.Private.CoreLib/Win32Marshal.cs,8cf456a56c1e487e

@danmoseley
Copy link
Member

sounds good to me.

@jozkee
Copy link
Member Author

jozkee commented Jul 19, 2022

One CI error is #70969, the other one is System.Net.Http test failures (couldn't find a related issue).

@jozkee jozkee merged commit a54a823 into dotnet:main Jul 19, 2022
@jozkee jozkee deleted the fileprovider-71003 branch July 19, 2022 18:33
@ghost ghost locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants