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

PhysicalFileProvider: Remove second try on GetFileLinkTargetLastWriteTimeUtc #57136

Merged
merged 3 commits into from
Aug 11, 2021
Merged

PhysicalFileProvider: Remove second try on GetFileLinkTargetLastWriteTimeUtc #57136

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f07bb3b
return the value from 2nd try
adamsitnik Aug 10, 2021
cd370c8
Do not attempt a second try and just capture the FileNotFoundException
jozkee Aug 10, 2021
9827528
Update src/libraries/Microsoft.Extensions.FileProviders.Physical/src/…
danmoseley Aug 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ibraries/Microsoft.Extensions.FileProviders.Physical/src/Internal/FileSystemInfoHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public static bool IsExcluded(FileSystemInfo fileSystemInfo, ExclusionFilters fi
// Try one more time, if it fails again just give up.
danmoseley marked this conversation as resolved.
Show resolved Hide resolved
if (!isSecondTry)
{
GetFileLinkTargetLastWriteTimeUtc(fileInfo, isSecondTry: true);
return GetFileLinkTargetLastWriteTimeUtc(fileInfo, isSecondTry: true);
Copy link
Member

Choose a reason for hiding this comment

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

Why try a second time? I don't see any discussion about why. If the file no longer exists, why do we expect it to appear milliseconds (microseconds?) later? We don't retry in our IO operations in general, eg., File.Exists doesn't retry.

Also why FileNotFoundException specifically, since this can throw various kinds of IO exceptions? https://github.com/danmoseley/runtime/blob/986ea1324ad08d052c468b92c5c92b47c7b57bea/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs#L603

Copy link
Member

Choose a reason for hiding this comment

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

Why try a second time?

We call fileInfo.LinkTarget != null which does a syscall and verifies that the file is a link. Then we need to call fileInfo.ResolveLinkTarget in order to go to the link's target, this implies a second syscall to the path. Between both syscall, it may occur that the file no longer exists so we try one more time to make sure that the file is indeed not existing anymore. As an alternative, we could handle the exception without the "second try".

why FileNotFoundException specifically

That's the one exception that has been caught in CI, we could be more loose but I didn't want to do it unless there was evidence that supports doing so (i.e: broken tests).

Copy link
Member

@jozkee jozkee Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Copying what was written in #57128 (comment) to keep the conversation in one place:

danmoseley commented 1 hour ago •
@adamsitnik is it fragile to run this code on a timer (basically QUWI) and aim to catch only documented exceptions? Since if something wierd happens the app terminates. I wonder whether it should capture all exceptions, like a Task, and rethrow them on the requesting thread? Not sure what the usual pattern is in this code, or whether rethrowing on the original thread would terminate the app anyway.

Jozkee commented 6 minutes ago
@danmoseley FWIW I think this happens because Timer.Dispose() doesn't prevent queued timer callbacks from being called after disposed.

Callbacks can occur after the Dispose() method overload has been called, because the timer queues callbacks for execution by thread pool threads. You can use the Dispose(WaitHandle) method overload to wait until all callbacks have completed.

We could switch to Timer.Dispose(WaitHandle) in order to wait for all queued callbacks to finish before continuing disposing and that could prevent the FileNotFoundException, however, PhysicalFileProvider code was already kinda resilient to these errors when used with non-link files so maybe we should go for resiliency instead?

Copy link
Member

Choose a reason for hiding this comment

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

As an alternative, we could handle the exception without the "second try".

If I understand right, that would work just as well, and would avoid another 1-2 syscalls and another exception.

That's the one exception that has been caught in CI,

I see - fair enough - it makes sense to throw all other IO exceptions normally, but FNFE is special because of this race.

Copy link
Member

Choose a reason for hiding this comment

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

PhysicalFileProvider code was already kinda resilient to these errors when used with non-link files so maybe we should go for resiliency instead?

So we have made PFP potentially less resilient for existing code? On the face of it that sounds like something we should avoid but I don't have full context.

Copy link
Member

Choose a reason for hiding this comment

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

So we have made PFP potentially less resilient for existing code?

Actually, nevermind that last thing, I was thinking that File.GetLastWriteTimeUtc and FileSystmeInfo.LastWriteTimeUtc was more resilient to exceptions but they are not.

}
}

Expand Down