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 polling use the symbolic link target's LastWriteTime #55664

Merged
merged 18 commits into from
Jul 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
<TargetFrameworks>$(NetCoreAppCurrent);netstandard2.0;net461</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<Compile Include="Microsoft.Extensions.FileProviders.Abstractions.cs" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\ref\Microsoft.Extensions.Primitives.csproj" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">
<ProjectReference Include="$(LibrariesProjectRoot)System.Runtime\ref\System.Runtime.csproj" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<RootNamespace>Microsoft.Extensions.FileProviders</RootNamespace>
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
<TargetFrameworks>$(NetCoreAppCurrent);netstandard2.0;net461</TargetFrameworks>
<EnableDefaultItems>true</EnableDefaultItems>
<PackageDescription>Abstractions of files and directories.

Expand All @@ -21,4 +21,9 @@ Microsoft.Extensions.FileProviders.IFileProvider</PackageDescription>
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\src\Microsoft.Extensions.Primitives.csproj" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Reference Include="System.Runtime" />
<Reference Include="System.Linq" />
<Reference Include="System.Linq.Expressions" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
<Import Project="..\Directory.Build.props" />
<PropertyGroup>
<IsAspNetCoreApp>true</IsAspNetCoreApp>
<UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

<UnsupportedOSPlatforms>browser</UnsupportedOSPlatforms>

Copy link
Member

Choose a reason for hiding this comment

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

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

cc: @marek-safar @steveisok

Copy link
Member

Choose a reason for hiding this comment

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

/cc @lewing

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Yes which is what this setting does

Copy link
Member

@safern safern Jul 16, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

</PropertyGroup>
</Project>
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
<TargetFrameworks>$(NetCoreAppCurrent);netstandard2.0;net461</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<Compile Include="Microsoft.Extensions.FileProviders.Physical.cs" />
Expand All @@ -10,4 +10,7 @@
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.FileSystemGlobbing\ref\Microsoft.Extensions.FileSystemGlobbing.csproj" />
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\ref\Microsoft.Extensions.Primitives.csproj" />
</ItemGroup>
<ItemGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">
<ProjectReference Include="$(LibrariesProjectRoot)System.IO.FileSystem.Watcher\ref\System.IO.FileSystem.Watcher.csproj" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using System.IO;

namespace Microsoft.Extensions.FileProviders.Physical
Expand All @@ -27,5 +28,40 @@ public static bool IsExcluded(FileSystemInfo fileSystemInfo, ExclusionFilters fi

return false;
}

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

// 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);
if (fileInfo.LinkTarget != null)
{
FileSystemInfo targetInfo = fileInfo.ResolveLinkTarget(returnFinalTarget: true);
Copy link
Member

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

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

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

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

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

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

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

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 targetInfo.LastWriteTimeUtc;
}

return DateTime.MinValue;
}
#endif

return null;
}
}
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<RootNamespace>Microsoft.Extensions.FileProviders</RootNamespace>
<TargetFrameworks>netstandard2.0;net461</TargetFrameworks>
<TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppCurrent)-Browser;netstandard2.0;net461</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<EnableDefaultItems>true</EnableDefaultItems>
<PackageDescription>File provider for physical files for Microsoft.Extensions.FileProviders.</PackageDescription>
</PropertyGroup>

<PropertyGroup Condition="'$(TargetsBrowser)' == 'true'">
<EnableDefaultItems>false</EnableDefaultItems>
<GeneratePlatformNotSupportedAssemblyMessage>SR.FileProvidersPhysical_PlatformNotSupported</GeneratePlatformNotSupportedAssemblyMessage>
</PropertyGroup>

<ItemGroup>
<Compile Include="$(CommonPath)Extensions\EmptyDisposable.cs"
Expand All @@ -26,4 +31,15 @@
<PackageReference Include="System.Security.Cryptography.Algorithms" Version="$(SystemSecurityCryptographyAlgorithmsVersion)" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Reference Include="System.Collections.Concurrent" />
<Reference Include="System.ComponentModel.Primitives" />
<Reference Include="System.IO.FileSystem.Watcher" />
<Reference Include="System.Linq" />
<Reference Include="System.Linq.Expressions" />
<Reference Include="System.Runtime" />
<Reference Include="System.Security.Cryptography.Algorithms" />
<Reference Include="System.Security.Cryptography.Primitives" />
<Reference Include="System.Threading" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,13 @@ public PollingFileChangeToken(FileInfo fileInfo)
private DateTime GetLastWriteTimeUtc()
{
_fileInfo.Refresh();
return _fileInfo.Exists ? _fileInfo.LastWriteTimeUtc : DateTime.MinValue;

if (!_fileInfo.Exists)
{
return DateTime.MinValue;
}

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

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ private bool CalculateChanges()
/// <returns>The <see cref="DateTime"/> that the file was last modified.</returns>
protected virtual DateTime GetLastWriteUtc(string path)
{
return File.GetLastWriteTimeUtc(Path.Combine(_directoryInfo.FullName, path));
string filePath = Path.Combine(_directoryInfo.FullName, path);
return FileSystemInfoHelper.GetFileLinkTargetLastWriteTimeUtc(filePath) ?? File.GetLastWriteTimeUtc(filePath);
}

private static bool ArrayEquals(byte[] previousHash, byte[] currentHash)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,7 @@
<data name="UnexpectedFileSystemInfo" xml:space="preserve">
<value>Unexpected type of FileSystemInfo</value>
</data>
<data name="FileProvidersPhysical_PlatformNotSupported" xml:space="preserve">
<value>Microsoft.Extensions.FileProviders.Physical is not supported on this platform.</value>
</data>
</root>
Original file line number Diff line number Diff line change
@@ -1,17 +1,22 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<RootNamespace>Microsoft.Extensions.FileProviders.Physical</RootNamespace>
<TargetFrameworks>$(NetCoreAppCurrent);net461</TargetFrameworks>
<EnableDefaultItems>true</EnableDefaultItems>
<IgnoreForCI Condition="'$(TargetOS)' == 'Browser'">true</IgnoreForCI>
<IncludePlatformAttributes>false</IncludePlatformAttributes>
</PropertyGroup>

<ItemGroup>
<Compile Include="$(CommonTestPath)System\Threading\Tasks\TaskTimeoutExtensions.cs"
Link="Common\System\Threading\Tasks\TaskTimeoutExtensions.cs" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFramework)' != '$(NetCoreAppCurrent)'">
<Compile Remove="PhysicalFileProviderTests.netcoreapp.cs" />
</ItemGroup>

<ItemGroup>
<PackageReference Include="Moq" Version="$(MoqVersion)" />
<ProjectReference Include="..\src\Microsoft.Extensions.FileProviders.Physical.csproj" SkipUseReferenceAssembly="true" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

namespace Microsoft.Extensions.FileProviders
{
public class PhysicalFileProviderTests
public partial class PhysicalFileProviderTests
{
private const int WaitTimeForTokenToFire = 500;
private const int WaitTimeForTokenCallback = 10000;
Expand Down Expand Up @@ -1512,6 +1512,58 @@ public void UsePollingFileWatcher_FileWatcherNotNull_ReturnsFalse()
}
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public async Task UsePollingFileWatcher_UseActivePolling_HasChanged(bool useWildcard)
{
// Arrange
using var root = new DisposableFileSystem();
string fileName = Path.GetRandomFileName();
string filePath = Path.Combine(root.RootPath, fileName);
File.WriteAllText(filePath, "v1.1");

using var provider = new PhysicalFileProvider(root.RootPath) { UsePollingFileWatcher = true, UseActivePolling = true };
IChangeToken token = provider.Watch(useWildcard ? "*" : fileName);

var tcs = new TaskCompletionSource<object>();
token.RegisterChangeCallback(_ => { tcs.TrySetResult(null); }, null);

// Act
await Task.Delay(1000); // Wait a second before writing again, see https://github.com/dotnet/runtime/issues/55951.
File.WriteAllText(filePath, "v1.2");

// Assert
jozkee marked this conversation as resolved.
Show resolved Hide resolved
Assert.True(tcs.Task.Wait(TimeSpan.FromSeconds(30)),
$"Change event was not raised - current time: {DateTime.UtcNow:O}, file LastWriteTimeUtc: {File.GetLastWriteTimeUtc(filePath):O}");
}

[Theory]
[InlineData(false)]
[InlineData(true)]
public void UsePollingFileWatcher_UseActivePolling_HasChanged_FileDeleted(bool useWildcard)
{
// Arrange
using var root = new DisposableFileSystem();
string fileName = Path.GetRandomFileName();
string filePath = Path.Combine(root.RootPath, fileName);
File.WriteAllText(filePath, "v1.1");

string filter = useWildcard ? "*" : fileName;
using var provider = new PhysicalFileProvider(root.RootPath) { UsePollingFileWatcher = true, UseActivePolling = true };
IChangeToken token = provider.Watch(filter);

var tcs = new TaskCompletionSource<object>();
token.RegisterChangeCallback(_ => { tcs.TrySetResult(null); }, null);

// Act
File.Delete(filePath);

// Assert
Assert.True(tcs.Task.Wait(TimeSpan.FromSeconds(30)),
$"Change event was not raised - current time: {DateTime.UtcNow:O}, file Exists: {File.Exists(filePath)}.");
}

[Fact]
public void CreateFileWatcher_CreatesWatcherWithPollingAndActiveFlags()
{
Expand Down
Loading