PhysicalFileProvider: Remove second try on GetFileLinkTargetLastWriteTimeUtc

[adamsitnik]

https://github.com/dotnet/runtime/pull/56915/files#r686113979

[danmoseley]

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

[jozkee]

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).

[jozkee]

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?

[danmoseley]

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.

[danmoseley]

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.

[jozkee]

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.

[danmoseley]

Thanks, I think this is cleaner.

[danmoseley]

@tarekgh seems several test failures related to your earlier work? https://github.com/dotnet/runtime/pull/57136/checks?check_run_id=3294583322

[jozkee]

I just filed an issue for Microsoft.Extensions.Hosting.Tests.ConsoleLifetimeExitTests #57184.
@danmoseley seems to me that all the CI failures are unrelated. Will merge now.