Make polling use the symbolic link target's LastWriteTime #55664

jozkee · 2021-07-14T18:28:06Z

Builds on top of #55668

Adding NetCoreAppCurrent to Microsoft.Extensions.FileProviders.Physical and to all the projects it depends on, in order to use the Symbolic Link APIs added in .NET 6.
cc @ericstj @jeffhandley

The fix consists on avoid using the link's LastWriteTimeUtc with the one of its target.

ghost · 2021-07-14T18:28:15Z

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

Issue Details

Fixes #36091

Adding NetCoreAppCurrent to Microsoft.Extensions.FileProviders.Physical and to all the projects it depends on, in order to use the Symbolic Link APIs added in .NET 6.
cc @ericstj @jeffhandley

Author:	Jozkee
Assignees:	Jozkee
Labels:	`area-System.IO`
Milestone:	6.0.0

ericstj · 2021-07-14T18:47:16Z

src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs

@@ -38,7 +38,7 @@ public class PollingFileChangeToken : IPollingChangeToken
        /// <param name="fileInfo">The <see cref="System.IO.FileInfo"/> to poll</param>
        public PollingFileChangeToken(FileInfo fileInfo)
        {
-            _fileInfo = fileInfo;
+            _fileInfo = FileSystemInfoHelper.ResolveFileLinkTarget(fileInfo) ?? fileInfo;


Will this raise new exceptions if the target of the file is not reachable? Perhaps we can add a test case for that: watching a normal file, watching a non-existent file, watching a symlinked file, watching a symlink with non-existent target.

It shouldn't, Exists and LinkTarget != null should guard for that.

runtime/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/Internal/FileSystemInfoHelper.cs

Lines 42 to 49 in 08233ba

if (fileInfo.Exists && fileInfo.LinkTarget != null)

{

FileSystemInfo targetInfo = fileInfo.ResolveLinkTarget(returnFinalTarget: true);

if (targetInfo.Exists)

{

return (FileInfo)targetInfo;

}

}

However since these APIs are not atomic, it could be that the link gets deleted between LinkTarget and ResolveLinkTarget. I don't know if there's an atomic way to do this.

I think it's worse than just not being atomic, since you capture the FileInfo for the lifetime of the token, which might be long.

Here's another scenario. Suppose that the symlink initially points at one target, but then is changed to point at a new target and that new target has a new modified time? I think that's actually the scenario in this issue. If you captured the FileInfo from the initial target you'd miss that change since you'd be checking the original target.

What happens if the symlinks chain goes through an area that you can't read?

touch bar mkdir foo cd foo ln -s ../bar . cd .. ln -s baz foo/bar sha256sum bar baz chmod a-x foo sha256sum bar baz

In our world, baz.LinkTarget should resolve (to foo/bar). Does foo/bar then report Exists is false since it can't be read (the directory is no longer allowed to be traversed into)?

In our world, baz.LinkTarget should resolve (to foo/bar). Does foo/bar then report Exists is false since it can't be read (the directory is no longer allowed to be traversed into)?

@bartonjs Yes, that's what I would expect. Therefore this should print false, unless that the FileInfo .ctor does some validation on inaccessible paths:

var targetInfo = File.ResolveLinkTarget("baz", returnFinaltarget: true); Console.Write(targetInfo.Exists);

Thank you for addressing the link target feedback. Please consider adding test cases for the deleted link target cases, as they seem unique to this addition as it has to handle non-existent link targets, and link targets that get deleted / added.

If the link target does not exists, we are returning the LastWriteTimeUtc from the link itself but I think that doesn't tell us anything, I will change it to return DateTime.MinValue instead (same thing we do when the file does not exists).
Let me know if you think that could be problematic.

I also added the requested tests: 98b737a.

src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingWildCardChangeToken.cs

...nsions.FileProviders.Abstractions/ref/Microsoft.Extensions.FileProviders.Abstractions.csproj

safern · 2021-07-15T16:22:31Z

src/libraries/Microsoft.Extensions.FileProviders.Physical/Directory.Build.props

@@ -2,5 +2,6 @@
  <Import Project="..\Directory.Build.props" />
  <PropertyGroup>
    <IsAspNetCoreApp>true</IsAspNetCoreApp>
+    <UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>


How is this unsupported on browser? Is the whole assembly not supported on browser?

this project references System.IO.FileSystem.Watcher and that is not supported in browser:

runtime/src/libraries/System.IO.FileSystem.Watcher/Directory.Build.props

Line 6 in 7aaffcb

<UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>

I see, so then should we add a runtime assembly for Browser that throws PNSE?

cc: @marek-safar @steveisok

/cc @lewing

I see, so then should we add a runtime assembly for Browser that throws PNSE?

Yes which is what this setting does

This setting doesn't generate a PNSE automatically, this setting is just to add an assembly attribute for the platform analyzer. Unless something changed since last time I did it, in order to generate PNSE we need to add a -Browser tfm and then set the flag to generate a pnse when TargetsBrowser == true. So then it seems like we should do that.

Thanks for catching this, I've addressed @safern's comment in 75fcf96, please let me know if something else (related to mark the project as unsupported in Browser) needs to be done.

...s/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.csproj

…issue

src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/Directory.Build.props

ericstj · 2021-07-16T21:58:38Z

...es/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.netcoreapp.cs

+            // Act 2 - Change link target to file 2.
+            token = provider.Watch(filter); // Once HasChanged is true, the value will always be true. Get a new change token.
+            Assert.False(token.HasChanged);
+            File.Delete(linkPath);


Is it possible to reset the target without deleting the link?

Through File.CreateSymbolicLink, it is not. It may be possible to modify the target of an existing link by using FSCTL_SET_REPARSE_POINT, see https://docs.microsoft.com/en-us/windows/win32/fileio/reparse-point-operations.

For Unix, I know that you can use the -f switch in ln -sf bar foo to force the link to be created even if it already exists, but I guess that's just deleting the old link.

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ln.html says that -f in ln is functionally equivalent to "call unlink", which we call File.Delete.

...es/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.netcoreapp.cs

ericstj · 2021-07-16T22:04:48Z

src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs

@@ -38,7 +38,7 @@ public class PollingFileChangeToken : IPollingChangeToken
        /// <param name="fileInfo">The <see cref="System.IO.FileInfo"/> to poll</param>
        public PollingFileChangeToken(FileInfo fileInfo)
        {
-            _fileInfo = fileInfo;
+            _fileInfo = FileSystemInfoHelper.ResolveFileLinkTarget(fileInfo) ?? fileInfo;


Thank you for addressing the link target feedback. Please consider adding test cases for the deleted link target cases, as they seem unique to this addition as it has to handle non-existent link targets, and link targets that get deleted / added.

…ributes=false

…o debug CI failures and increase delay between writes

adamsitnik · 2021-07-20T08:53:58Z

src/libraries/Microsoft.Extensions.FileProviders.Physical/src/Internal/FileSystemInfoHelper.cs

+            Debug.Assert(fileInfo.Exists);
+            if (fileInfo.LinkTarget != null)
+            {
+                FileSystemInfo targetInfo = fileInfo.ResolveLinkTarget(returnFinalTarget: true);


@jozkee have you considered keeping all these libs .NET Standard 2.0 and referencing the code that defines ResolveLinkTarget (and other things if needed)? We are already doing something like this for Microsoft.IO.Redist which targets net471. I am just wondering how much effort it would require to keep it NS2.0.

@jeffhandley @ericstj how long do we plan to keep targeting netstandard2.0 and net461 for the Microsoft.Extensions* libs?

@jeffhandley Jeff Handley FTE @ericstj Eric St. John FTE how long do we plan to keep targeting netstandard2.0 and net461 for the Microsoft.Extensions* libs?

At the moment we have no plans for dropping netstandard2.0 (and therefor net461) from the extensions. Perhaps it's something that could be considered in the future if this limitation hinders development considerably. They were intentionally omitted from aspnet/Announcements#324. cc @davidfowl @DamianEdwards @maryamariyan

Yes, we're keeping ns2.0 and net461 until further notice.

We can do a different (and more efficient) approach to get the target's Last write time, see @ericstj's suggestion #55664 (comment).
But (not) saving a couple of allocations is not a big issue considering that the performance penalty is relatively low compared to other pieces of the code that allocate much more.

We can do a different (and more efficient) approach to get the target's Last write time

@jozkee I wanted to know whether it would be possible to make this fix work for all TFMs (6 + NS2 + net461), because currently, it's going to work only for .NET 6.

It is, but we can defer fixing this on other TFMs if no one is asking for it.
Do you think we can take a dependency on MS.IO.Redist here to utilize the same set of Symbolic Link APIs in order to fix this in ns2.0?
cc @jeffhandley.

I think to fix on older TFMs it needs more than just MS.IO.Redist. I believe the implementation of the IO API you are using here (or would add) would depend on the system-native PALs which we don't consider part of the public API. Having a package depend on those is fragile at the least, and might not even work if the versions of those don't have the exports you need on older frameworks. Adding private copies of the system-native PALs (similar to what System.IO.Ports does) is expensive too. Something to consider around OOB'ing our IO API.

Ahh, good info @ericstj. So far, we don't have any requests to have this fix apply to previous versions; I'm OK with this being net6.0+ until such requests come in.

src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.cs

…milliseconds

jozkee · 2021-07-21T19:36:28Z

Judging by the Helix log, it appears to be that LastWriteTimeUtc in OSX doesn't report the milliseconds. I have now increased the delay between writes to one second. All tests should pass now.
We can track the milliseconds issue in #55951.

…s_polling

adamsitnik

LGTM, thank you for the fix @jozkee!

tmds · 2021-07-22T18:40:51Z

fwiw, I think we failed to acknowledge the root cause of #36091 is FileInfo does not return LastWriteTimeUtc for the target (but for the link). Instead of trying to improve that, specific APIs are now patched to handle symbolic links to get to the target LastWriteTimeUtc.
#52908 is a better solution imo (and is more efficient on Linux).

jozkee · 2021-07-22T18:54:28Z

#52908 is a better solution imo (and is more efficient on Linux).

After working on fixing this I agree on that, we should definitely give it a try.

jozkee added 2 commits July 14, 2021 10:55

Relax LinkTarget so it always returns null when steps on an error

f515710

Make polling use the symbolic link target's LastWriteTime

08233ba

jozkee added the area-System.IO label Jul 14, 2021

jozkee added this to the 6.0.0 milestone Jul 14, 2021

jozkee requested review from jeffhandley, carlossanlop, adamsitnik, ericstj, bartonjs and safern July 14, 2021 18:28

jozkee self-assigned this Jul 14, 2021

jozkee mentioned this pull request Jul 14, 2021

Relax LinkTarget so it always returns null when steps on an error #55668

Merged

ericstj reviewed Jul 14, 2021

View reviewed changes

safern reviewed Jul 15, 2021

View reviewed changes

...nsions.FileProviders.Abstractions/ref/Microsoft.Extensions.FileProviders.Abstractions.csproj Outdated Show resolved Hide resolved

safern reviewed Jul 15, 2021

View reviewed changes

...s/Microsoft.Extensions.FileSystemGlobbing/ref/Microsoft.Extensions.FileSystemGlobbing.csproj Outdated Show resolved Hide resolved

jozkee added 4 commits July 15, 2021 16:08

Fix for the case where the link can change its target

d780995

Add more test cases and exclude them from non-netcoreapp tfms

ac4a845

Fix project references in ref projects

b4895ad

Do not use UnsupportedOSPlatforms on test project in order to fix CI …

ebb0326

…issue

safern reviewed Jul 16, 2021

View reviewed changes

src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/Directory.Build.props Outdated Show resolved Hide resolved

ericstj reviewed Jul 16, 2021

View reviewed changes

jozkee added 6 commits July 19, 2021 14:52

Do not return link's LastWriteTime when target not exists

1164e33

Address feedback on tests and improve them to cover more scenarios.

98b737a

Make the project unsupported in browser.

75fcf96

Fix duplicate reference to PlatformAttributes with IncludePlatformAtt…

144335a

…ributes=false

Disable default items for browser

9c50a3a

Undo unrelated changes to Strings.resx

b2f9bad

jozkee requested a review from ericstj July 20, 2021 00:36

jozkee requested a review from safern July 20, 2021 00:36

Replace Thread.Sleep with Task.Delay, add assertion messages to try t…

d8d143a

…o debug CI failures and increase delay between writes

adamsitnik reviewed Jul 20, 2021

View reviewed changes

Replace HasChanged for RegisterChangeCallback in tests

c7e28d4

jozkee requested a review from adamsitnik July 21, 2021 00:49

jozkee added 3 commits July 21, 2021 08:47

Add messages to asserts to attempt to debug CI issues

5b85631

Add date format to assertion messages.

5c27cae

Increase delay between writes to one second since OSX doesn't report …

19dd9c3

…milliseconds

Merge branch 'main' of https://github.com/dotnet/runtime into symlink…

8c1b3a2

…s_polling

adamsitnik approved these changes Jul 22, 2021

View reviewed changes

jozkee merged commit 101e68a into dotnet:main Jul 22, 2021

jozkee deleted the symlinks_polling branch July 22, 2021 18:11

eerhardt mentioned this pull request Jul 22, 2021

Most of Microsoft.Extensions.FileProviders.Physical should be supported on Browser #56178

Closed

jozkee mentioned this pull request Jul 22, 2021

Microsoft.Extensions.FileProviders.Physical.Tests disabled on Unix systems #56190

Closed

This was referenced Jul 28, 2021

[WinHttpHandler] Long running test: ResponseHeadersRead_SynchronizationContextNotUsedByHandler on Win7/Win8 #54034

Closed

Massive long running tests on Linux #56477

Closed

System.Net.Security test run killed on Mono Linux #56478

Closed

jozkee mentioned this pull request Aug 2, 2021

dotnet watch does not work on macOS Catalina #54586

Closed

ghost locked as resolved and limited conversation to collaborators Aug 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make polling use the symbolic link target's LastWriteTime #55664

Make polling use the symbolic link target's LastWriteTime #55664

jozkee commented Jul 14, 2021 •

edited

Loading

ghost commented Jul 14, 2021

ericstj Jul 14, 2021

jozkee Jul 14, 2021

ericstj Jul 14, 2021

bartonjs Jul 15, 2021

jozkee Jul 15, 2021 •

edited

Loading

ericstj Jul 16, 2021

jozkee Jul 19, 2021 •

edited

Loading

safern Jul 15, 2021

jozkee Jul 15, 2021

safern Jul 15, 2021

steveisok Jul 15, 2021

marek-safar Jul 16, 2021

safern Jul 16, 2021 •

edited

Loading

jozkee Jul 19, 2021

ericstj Jul 16, 2021

jozkee Jul 16, 2021 •

edited

Loading

bartonjs Jul 20, 2021

ericstj Jul 16, 2021

adamsitnik Jul 20, 2021

ericstj Jul 20, 2021

davidfowl Jul 20, 2021

jozkee Jul 20, 2021

adamsitnik Jul 21, 2021

jozkee Jul 21, 2021 •

edited

Loading

ericstj Jul 22, 2021

jeffhandley Jul 23, 2021

jozkee commented Jul 21, 2021

adamsitnik left a comment

tmds commented Jul 22, 2021

jozkee commented Jul 22, 2021

	if (fileInfo.Exists && fileInfo.LinkTarget != null)
	{
	FileSystemInfo targetInfo = fileInfo.ResolveLinkTarget(returnFinalTarget: true);
	if (targetInfo.Exists)
	{
	return (FileInfo)targetInfo;
	}
	}

Make polling use the symbolic link target's LastWriteTime #55664

Make polling use the symbolic link target's LastWriteTime #55664

Conversation

jozkee commented Jul 14, 2021 • edited Loading

ghost commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jozkee commented Jul 21, 2021

adamsitnik left a comment

Choose a reason for hiding this comment

tmds commented Jul 22, 2021

jozkee commented Jul 22, 2021

jozkee commented Jul 14, 2021 •

edited

Loading

jozkee Jul 15, 2021 •

edited

Loading

jozkee Jul 19, 2021 •

edited

Loading

safern Jul 16, 2021 •

edited

Loading

jozkee Jul 16, 2021 •

edited

Loading

jozkee Jul 21, 2021 •

edited

Loading