Skip to content

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

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

Merged
jozkee merged 18 commits into from
Jul 22, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f515710
Relax LinkTarget so it always returns null when steps on an error
jozkee Jul 14, 2021
08233ba
Make polling use the symbolic link target's LastWriteTime
jozkee Jul 14, 2021
d780995
Fix for the case where the link can change its target
jozkee Jul 15, 2021
ac4a845
Add more test cases and exclude them from non-netcoreapp tfms
jozkee Jul 15, 2021
b4895ad
Fix project references in ref projects
jozkee Jul 15, 2021
ebb0326
Do not use UnsupportedOSPlatforms on test project in order to fix CI …
jozkee Jul 16, 2021
1164e33
Do not return link's LastWriteTime when target not exists
jozkee Jul 19, 2021
98b737a
Address feedback on tests and improve them to cover more scenarios.
jozkee Jul 19, 2021
75fcf96
Make the project unsupported in browser.
jozkee Jul 19, 2021
144335a
Fix duplicate reference to PlatformAttributes with IncludePlatformAtt…
jozkee Jul 19, 2021
9c50a3a
Disable default items for browser
jozkee Jul 20, 2021
b2f9bad
Undo unrelated changes to Strings.resx
jozkee Jul 20, 2021
d8d143a
Replace Thread.Sleep with Task.Delay, add assertion messages to try t…
jozkee Jul 20, 2021
c7e28d4
Replace HasChanged for RegisterChangeCallback in tests
jozkee Jul 21, 2021
5b85631
Add messages to asserts to attempt to debug CI issues
jozkee Jul 21, 2021
5c27cae
Add date format to assertion messages.
jozkee Jul 21, 2021
19dd9c3
Increase delay between writes to one second since OSX doesn't report …
jozkee Jul 21, 2021
8c1b3a2
Merge branch 'main' of https://github.com/dotnet/runtime into symlink…
jozkee Jul 21, 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
Prev Previous commit
Next Next commit
Do not return link's LastWriteTime when target not exists
  • Loading branch information
@jozkee
jozkee committed Jul 19, 2021
commit 1164e3355030a95c46099395e9806d0be2a182df
14 changes: 10 additions & 4 deletions ...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 @@ -29,19 +29,23 @@ public static bool IsExcluded(FileSystemInfo fileSystemInfo, ExclusionFilters fi
return false;
}

public static FileInfo ResolveFileLinkTarget(string filePath)
public static DateTime? GetFileLinkTargetLastWriteTimeUtc(string filePath)
{
#if NETCOREAPP
var fileInfo = new FileInfo(filePath);
if (fileInfo.Exists)
{
return ResolveFileLinkTarget(fileInfo);
return GetFileLinkTargetLastWriteTimeUtc(fileInfo);
}
#endif
return null;
}

public static FileInfo ResolveFileLinkTarget(FileInfo fileInfo)
// If file is a link and link target exists, return target's LastWriteTimeUtc.
// If file is a link, and link target does not exists, return DateTime.MinValue
// since the link's LastWriteTimeUtc doesn't convey anything for this scenario.
// If file is not a link, return null to inform the caller that file is not a link.
public static DateTime? GetFileLinkTargetLastWriteTimeUtc(FileInfo fileInfo)
{
#if NETCOREAPP
Debug.Assert(fileInfo.Exists);
Expand All @@ -50,8 +54,10 @@ public static FileInfo ResolveFileLinkTarget(FileInfo fileInfo)
FileSystemInfo targetInfo = fileInfo.ResolveLinkTarget(returnFinalTarget: true);
Copy link
Member

@adamsitnik adamsitnik Jul 20, 2021

Choose a reason for hiding this comment

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

@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?

Copy link
Member

@ericstj ericstj Jul 20, 2021

Choose a reason for hiding this comment

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

@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

Copy link
Member

@davidfowl davidfowl Jul 20, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@jozkee jozkee Jul 20, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@adamsitnik adamsitnik Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

Copy link
Member Author

@jozkee jozkee Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ericstj ericstj Jul 22, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jeffhandley jeffhandley Jul 23, 2021

Choose a reason for hiding this comment

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

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.

if (targetInfo.Exists)
{
return (FileInfo)targetInfo;
return targetInfo.LastWriteTimeUtc;
}

return DateTime.MinValue;
}
#endif

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ private DateTime GetLastWriteTimeUtc()
return DateTime.MinValue;
}

return FileSystemInfoHelper.ResolveFileLinkTarget(_fileInfo)?.LastWriteTimeUtc ?? _fileInfo.LastWriteTimeUtc;
return FileSystemInfoHelper.GetFileLinkTargetLastWriteTimeUtc(_fileInfo) ?? _fileInfo.LastWriteTimeUtc;
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingWildCardChangeToken.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ private bool CalculateChanges()
protected virtual DateTime GetLastWriteUtc(string path)
{
string filePath = Path.Combine(_directoryInfo.FullName, path);
return FileSystemInfoHelper.ResolveFileLinkTarget(filePath)?.LastWriteTimeUtc ?? File.GetLastWriteTimeUtc(filePath);
return FileSystemInfoHelper.GetFileLinkTargetLastWriteTimeUtc(filePath) ?? File.GetLastWriteTimeUtc(filePath);
}

private static bool ArrayEquals(byte[] previousHash, byte[] currentHash)
Expand Down