Update native File.Exists #9223

JaynieBai · 2023-09-18T09:35:02Z

Context

The root of the problem here is that our custom Windows-only implementation of File.Exists behaves differently than the regular one in BCL

Changes Made

Rewrite the WindowsFileSystem.FileExists implementation like this:

#if NETFRAMEWORK
            return Microsoft.IO.File.Exists(path);
#else
            return File.Exists(path);
#endif

Testing

ProjectItemSpecTooLong()

Notes

AR-May · 2023-10-26T14:25:14Z

@JaynieBai Could you comment on this PR and fill the description?

JaynieBai · 2023-11-01T01:52:47Z

/azp run

azure-pipelines · 2023-11-01T01:52:57Z

Azure Pipelines successfully started running 1 pipeline(s).

f-alizada

Overall looking good!
One comment left probably nit

src/Build.UnitTests/BackEnd/MSBuild_Tests.cs

AR-May · 2023-11-20T09:19:50Z

src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs

@@ -314,14 +314,14 @@ public async Task<bool> ExecuteInternal()
            {
                ITaskItem project = Projects[i];

-                string projectPath = FileUtilities.AttemptToShortenPath(project.ItemSpec);
+                string projectPath = FileUtilities.GetFullPathNoThrow(project.ItemSpec);


It seems like we shorten the path always after this change. I guess the explanation for the change is "So, when project path includes multiple reductant "..\" not longer than the max path, it will report MSBuild.ProjectFileNotFound.". I am a bit confused: if the path is not too long, how "MSBuild.ProjectFileNotFound" is thrown? I would think that the file would be found in this case.

When project path includes multiple reductant ".." not longer than the max path. The code here FileSystems.Default.FileExists(projectPath) will return false.

msbuild/src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs

Lines 343 to 392 in b59f07e

if (FileSystems.Default.FileExists(projectPath) || (skipNonExistProjects == SkipNonExistentProjectsBehavior.Build))

{

if (FileUtilities.IsVCProjFilename(projectPath))

{

Log.LogErrorWithCodeFromResources("MSBuild.ProjectUpgradeNeededToVcxProj", project.ItemSpec);

success = false;

continue;

}

// If we are building in parallel we want to only make one call to

// ExecuteTargets once we verified that all projects exist

if (!BuildInParallel)

{

singleProject[0] = project;

bool executeResult = await ExecuteTargets(

singleProject,

propertiesTable,

undefinePropertiesArray,

targetLists,

StopOnFirstFailure,

RebaseOutputs,

BuildEngine3,

Log,

_targetOutputs,

UnloadProjectsOnCompletion,

ToolsVersion,

SkipNonexistentTargets);

if (!executeResult)

{

success = false;

}

}

else

{

skipProjects[i] = false;

}

}

else

{

if (skipNonExistProjects == SkipNonExistentProjectsBehavior.Skip)

{

Log.LogMessageFromResources(MessageImportance.High, "MSBuild.ProjectFileNotFoundMessage", project.ItemSpec);

}

else

{

ErrorUtilities.VerifyThrow(skipNonExistProjects == SkipNonExistentProjectsBehavior.Error, "skipNonexistentProjects has unexpected value {0}", skipNonExistProjects);

Log.LogErrorWithCodeFromResources("MSBuild.ProjectFileNotFound", project.ItemSpec);

success = false;

}

Is this discussion still relevant?

…i/issue4247

src/Tasks.UnitTests/MSBuild_Tests.cs

…i/issue4247

…into jennybai/issue4247

ladipro · 2024-04-09T12:17:19Z

It looks like the root of the problem here is that our custom Windows-only implementation of File.Exists behaves differently than the regular one in BCL. Here it is:

msbuild/src/Framework/NativeMethods.cs

Lines 1725 to 1731 in 053feb0

    
           [SupportedOSPlatform("windows")] 
        
           internal static bool FileExistsWindows(string fullPath) 
        
           { 
        
               WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA(); 
        
               bool success = GetFileAttributesEx(fullPath, 0, ref data); 
        
               return success && (data.fileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0; 
        
           }

BCL API accepts paths like ..\..\..\..\..\..\..\..\..\..\..\..\..\mydir\myfile while ours does not. I think we should make sure that the fix is done at the right layer (or if it needs to be done at all). What are our guarantees when it comes to such paths? Are there other places where we currently have a similar Windows-only problem? Does the proposed fix of always normalizing paths with GetFullPathNoThrow have a negative perf impact on Linux and Mac where we likely don't need it?

@JaynieBai, can you please check why exactly FileSystems.Default.FileExists(projectPath) is failing without the proposed fix? Is it because the argument is longer than MAX_PATH, because it contains unbalanced ..'s, or because it contains any ..'s at all?

JaynieBai · 2024-04-12T07:56:25Z

It looks like the root of the problem here is that our custom Windows-only implementation of File.Exists behaves differently than the regular one in BCL. Here it is:

msbuild/src/Framework/NativeMethods.cs

Lines 1725 to 1731 in 053feb0

[SupportedOSPlatform("windows")]

internal static bool FileExistsWindows(string fullPath)

{

WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA();

bool success = GetFileAttributesEx(fullPath, 0, ref data);

return success && (data.fileAttributes & FILE_ATTRIBUTE_DIRECTORY) == 0;

}

BCL API accepts paths like ..\..\..\..\..\..\..\..\..\..\..\..\..\mydir\myfile while ours does not. I think we should make sure that the fix is done at the right layer (or if it needs to be done at all). What are our guarantees when it comes to such paths? Are there other places where we currently have a similar Windows-only problem? Does the proposed fix of always normalizing paths with GetFullPathNoThrow have a negative perf impact on Linux and Mac where we likely don't need it?

@JaynieBai, can you please check why exactly FileSystems.Default.FileExists(projectPath) is failing without the proposed fix? Is it because the argument is longer than MAX_PATH, because it contains unbalanced ..'s, or because it contains any ..'s at all?

I Find FileSystems.Default.FileExists calls the unmanage function GetFileAttributesEx. So write the following code and find when file is prefixed with different length of "..\". The outputs of File.Exists and GetFileAttributesEx are different. Not sure the reason now. @f-alizada Could you have a look?

static void Main(string[] args)
{
    int[] numbers = new int[] {7, 8, 50, 57, 101, 250 };
    for(int j=0 ; j<numbers.Length; j++)
    {
        string file = null;
        for (int i = 0; i < numbers[j]; i++)
        {
            file += "..\\";
        }
        file += "Users\\file.tmp";
        var test = File.Exists(file);
        Console.WriteLine("FileLength:" + file.Length);
        Console.WriteLine("File.Exists Output:" + test);
        WIN32_FILE_ATTRIBUTE_DATA data = new WIN32_FILE_ATTRIBUTE_DATA();
        var result = GetFileAttributesEx(file, 0, ref data);
        Console.WriteLine("FileSystems.Default.FileExists Output: " + result);
        if (!result)
        {
            int error = Marshal.GetLastWin32Error();
            Console.WriteLine($"Error {error} occurred while getting file attributes.");
        }
    }
}

[DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
[return: MarshalAs(UnmanagedType.Bool)]
internal static extern bool GetFileAttributesEx(String name, int fileInfoLevel, ref WIN32_FILE_ATTRIBUTE_DATA lpFileInformation);

/// <summary>
/// Contains information about a file or directory; used by GetFileAttributesEx.
/// </summary>
[StructLayout(LayoutKind.Sequential)]
public struct WIN32_FILE_ATTRIBUTE_DATA
{
    internal int fileAttributes;
    internal uint ftCreationTimeLow;
    internal uint ftCreationTimeHigh;
    internal uint ftLastAccessTimeLow;
    internal uint ftLastAccessTimeHigh;
    internal uint ftLastWriteTimeLow;
    internal uint ftLastWriteTimeHigh;
    internal uint fileSizeHigh;
    internal uint fileSizeLow;
}

.NET Framework Output

FileLength:35
File.Exists Output:False
FileSystems.Default.FileExists Output: False
Error 3 occurred while getting file attributes.
FileLength:38
File.Exists Output:True
FileSystems.Default.FileExists Output: True
FileLength:164
File.Exists Output:True
FileSystems.Default.FileExists Output: True
FileLength:185
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 3 occurred while getting file attributes.
FileLength:317
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 3 occurred while getting file attributes.
FileLength:764
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 3 occurred while getting file attributes.

.Net Output

FileLength:35
File.Exists Output:False
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.
FileLength:38
File.Exists Output:False
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.
FileLength:164
File.Exists Output:True
FileSystems.Default.FileExists Output: True
FileLength:185
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.
FileLength:317
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.
FileLength:764
File.Exists Output:True
FileSystems.Default.FileExists Output: False
Error 0 occurred while getting file attributes.

AR-May · 2024-06-12T18:40:32Z

I agree with @ladipro here that we need to make sure a change happens on a correct abstraction layer and isolated only to the failing scenario, given the perf concerns.

@rainersigwald do you have some knowledge why we have a custom Windows-only implementation of File.Exists here?

JanKrivanek · 2024-07-09T11:57:12Z

Synced with @AR-May on this - our conclusion: The root of the problem is in our custom implementation of FileExists (FileExistsWindows). The suggested course of action (@JaynieBai):

Compare performance of standard File.Exists and our custom FileExistsWindows (you can use BenchmarkDotNet for this) - and see if there is a significant difference (report the results here)
If there are no significant differences - let's replace our custom impl. with the standard impl
If there is a difference - ping the team - we'll need to have different (more specific) bug created for fixing the FileExistsWindows

MichalPavlik · 2024-08-29T09:04:42Z

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.4037/23H2/2023Update/SunValley3)
AMD Ryzen 7 5700X, 1 CPU, 16 logical and 8 physical cores
[Host] : .NET Framework 4.8.1 (4.8.9261.0), X64 RyuJIT VectorSize=256
DefaultJob : .NET Framework 4.8.1 (4.8.9261.0), X64 RyuJIT VectorSize=256

Method	Mean	Error	StdDev	Allocated
Native	59.18 us	1.025 us	1.052 us	-
Managed	61.09 us	0.804 us	0.752 us	96 B

No big difference in time, but managed implementation allocates.

JanKrivanek · 2024-09-02T12:51:29Z

Thanks @MichalPavlik! We should probably file a bug with Runtime team.

I'm wondering how does this allocation influence overall build perf. @JaynieBai - can you run OrchardCore build with current version (custom FileExists impl) vs version where we'd switch to File.Exists?
If the change is not measurable I'd still vote to get off of our custom implementation and adopt the File.Exists

MichalPavlik · 2024-09-03T10:43:56Z

@JanKrivanek, it was a little bit misleading as I forgot to test Microsoft.IO.Redist implementation.

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22631.4037/23H2/2023Update/SunValley3)
AMD Ryzen 7 5700X, 1 CPU, 16 logical and 8 physical cores
[Host] : .NET Framework 4.8.1 (4.8.9261.0), X64 RyuJIT VectorSize=256
DefaultJob : .NET Framework 4.8.1 (4.8.9261.0), X64 RyuJIT VectorSize=256

Method	Mean	Error	StdDev	Allocated
Native	58.10 us	0.648 us	0.574 us	-
Managed	60.87 us	0.870 us	0.814 us	96 B
Managed_IORedist	59.39 us	0.984 us	0.920 us	-

This one doesn't allocate :)
And I can share also result from my VM. Although it runs on virtual disk drive, maybe the ReFS/DevDrive has some impact :)

Method	Mean	Error	StdDev	Allocated
Native	26.13 us	0.483 us	0.452 us	-
Managed	28.64 us	0.206 us	0.172 us	96 B
Managed_IORedist	27.41 us	0.525 us	0.605 us	-

MichalPavlik · 2024-09-05T08:38:00Z

@JanKrivanek, @JaynieBai Replacing the FileExistsWindows should resolve #9986. Any chance to use MS.IO.Redist version for file existence check? Frankly, I would replace all P/Invokes in WindowsFileSystem, but as new PR for different issue :)

JanKrivanek · 2024-09-05T12:12:52Z

I have no strong preference between the 2 (MS.IO.Redist vs System.IO) - but I'm definitely in favor of getting rid the custom implementation.

MichalPavlik · 2024-09-06T07:03:16Z

@JaynieBai, please rewrite the WindowsFileSystem.FileExists implementation with something like this:

#if NETFRAMEWORK
            return Microsoft.IO.File.Exists(path);
#else
            return File.Exists(path);
#endif

This simple focused change is enough for now. We will talk about the rest of P/Invokes later.

JaynieBai · 2024-09-11T08:32:33Z

@MichalPavlik Microsoft.Build.UnitTests.XMakeAppTests.ConfigurationInvalid failed since not find the project file when build with copied MSBuild in the temp folder.
But actually, the project file is there, and build with RunnerUtilities.ExecBootstrapedMSBuild from the bootstrap MSBuild.exe , it succeeds.

MichalPavlik · 2024-09-18T17:12:13Z

This was tricky. The reason why is MS.IO.Redist returning false in this case is due to IOException swallowing inside the MS.IO.File.Exists method. When the method is called inside of a MSBuild copy with modified app.config, an exception is thrown:

System.IO.FileLoadException: 'Could not load file or assembly 'System.Memory, Version=4.0.1.1, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)'

This exception is silently ignored as FileLoadException is a subtype of IOException. The current version of System.Memory assembly distributed with MSBuild is 4.0.1.2. Adding bindingRedirect to the configContent resolves the issue, but it's a workaround that we would need to maintain. Or we would have to extract the assemblyBinding from existing app.config and inject to the generated one.

@rainersigwald Is this test critical enough to have such a complicated functional test? If so, then we have these options:

Make the test more complex by adding current assemblyBinding to the created app.config
Try to test this differently - using new temporary AppDomain (with modified config) if possible, or unit test if possible
Using old System.IO.File.Exists (which allocates 96 bytes) without modifying the test
Something else :)

I would personally prefer to find a way how to simplify the test if we really need this one. These kind of tests can fail on magnitude of reasons, and investigation is costly.

Edit: Bug report created - dotnet/maintenance-packages#117

MichalPavlik · 2024-09-19T08:23:26Z

No opinions so far. Let's use the simplest way :) @JaynieBai, please replace the configContent assignment with this:

XElement configRuntimeElement = XDocument.Load(RunnerUtilities.PathToCurrentlyRunningMsBuildExe + ".config").Root.Element("runtime");

string configContent = $@"<?xml version =""1.0""?>
                            <configuration>
                                <configSections>
                                    <section name=""msbuildToolsets"" type=""Microsoft.Build.Evaluation.ToolsetConfigurationSection, Microsoft.Build, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a"" />
                                    <foo/>
                                </configSections>
                                <startup>
                                    <supportedRuntime version=""v4.0""/>
                                </startup>
                                <foo/>
                                <msbuildToolsets default=""X"">
                                <foo/>
                                    <toolset toolsVersion=""X"">
                                        <foo/>
                                    <property name=""MSBuildBinPath"" value=""Y""/>
                                    <foo/>
                                    </toolset>
                                <foo/>
                                </msbuildToolsets>
                                {configRuntimeElement}
                            </configuration>";

It adds all assembly redirects to the config file and the Exists method will work as expected.

rainersigwald · 2024-09-19T15:03:28Z

Ah sorry. I like that plan @MichalPavlik.

Enable test ProjectItemSpecTooLong

12642d5

Get full path

f7a2b3f

JaynieBai marked this pull request as ready for review November 1, 2023 05:46

rainersigwald requested review from f-alizada and AR-May November 1, 2023 15:51

f-alizada reviewed Nov 6, 2023

View reviewed changes

src/Build.UnitTests/BackEnd/MSBuild_Tests.cs Show resolved Hide resolved

AR-May reviewed Nov 20, 2023

View reviewed changes

rokonec assigned JaynieBai Dec 6, 2023

JaynieBai added 2 commits December 29, 2023 10:41

Merge branch 'main' of https://github.com/dotnet/msbuild into jennyba…

e154785

…i/issue4247

Get the rootlength from tempProject

ea825e0

JanKrivanek requested a review from f-alizada March 27, 2024 12:26

f-alizada approved these changes Mar 28, 2024

View reviewed changes

f-alizada reviewed Mar 28, 2024

View reviewed changes

src/Tasks.UnitTests/MSBuild_Tests.cs Show resolved Hide resolved

Merge branch 'main' of https://github.com/dotnet/msbuild into jennyba…

eddaf2c

…i/issue4247

JaynieBai changed the title ~~Enable test ProjectItemSpecTooLong~~ Shorten the path always when actually just passing the long paths Mar 29, 2024

JaynieBai changed the title ~~Shorten the path always when actually just passing the long paths~~ Shorten the path always when actually passing the long paths Mar 29, 2024

JaynieBai added 4 commits March 29, 2024 15:23

Update the test exception

786c148

Update the test description

19bde2c

Merge branch 'jennybai/issue4247' of https://github.com/dotnet/msbuild …

07abf93

…into jennybai/issue4247

Update test description

057b285

f-alizada requested a review from AR-May April 9, 2024 10:26

JaynieBai closed this Apr 12, 2024

JaynieBai reopened this Apr 12, 2024

MichalPavlik mentioned this pull request Sep 2, 2024

Unexpected warning/error MSB3202 #9986

Open

JaynieBai added 3 commits September 10, 2024 17:38

Merge branch 'main' into jennybai/issue4247

cee1569

Update WindowsFileSystem.FileExists

51237ed

revert test description

cf2962c

MichalPavlik self-assigned this Sep 19, 2024

Adds all assembly redirects to the config file

8315ad1

JaynieBai changed the title ~~Shorten the path always when actually passing the long paths~~ Update native File.Exists Sep 20, 2024

MichalPavlik approved these changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update native File.Exists #9223

Update native File.Exists #9223

JaynieBai commented Sep 18, 2023 •

edited

Loading

AR-May commented Oct 26, 2023

JaynieBai commented Nov 1, 2023

azure-pipelines bot commented Nov 1, 2023

f-alizada left a comment

AR-May Nov 20, 2023

JaynieBai Dec 29, 2023

MichalPavlik Sep 20, 2024

ladipro commented Apr 9, 2024

JaynieBai commented Apr 12, 2024 •

edited

Loading

AR-May commented Jun 12, 2024

JanKrivanek commented Jul 9, 2024

MichalPavlik commented Aug 29, 2024 •

edited

Loading

JanKrivanek commented Sep 2, 2024

MichalPavlik commented Sep 3, 2024 •

edited

Loading

MichalPavlik commented Sep 5, 2024 •

edited

Loading

JanKrivanek commented Sep 5, 2024

MichalPavlik commented Sep 6, 2024 •

edited

Loading

JaynieBai commented Sep 11, 2024

MichalPavlik commented Sep 18, 2024 •

edited

Loading

MichalPavlik commented Sep 19, 2024

rainersigwald commented Sep 19, 2024

	if (FileSystems.Default.FileExists(projectPath) \|\| (skipNonExistProjects == SkipNonExistentProjectsBehavior.Build))
	{
	if (FileUtilities.IsVCProjFilename(projectPath))
	{
	Log.LogErrorWithCodeFromResources("MSBuild.ProjectUpgradeNeededToVcxProj", project.ItemSpec);
	success = false;
	continue;
	}

	// If we are building in parallel we want to only make one call to
	// ExecuteTargets once we verified that all projects exist
	if (!BuildInParallel)
	{
	singleProject[0] = project;
	bool executeResult = await ExecuteTargets(
	singleProject,
	propertiesTable,
	undefinePropertiesArray,
	targetLists,
	StopOnFirstFailure,
	RebaseOutputs,
	BuildEngine3,
	Log,
	_targetOutputs,
	UnloadProjectsOnCompletion,
	ToolsVersion,
	SkipNonexistentTargets);

	if (!executeResult)
	{
	success = false;
	}
	}
	else
	{
	skipProjects[i] = false;
	}
	}
	else
	{
	if (skipNonExistProjects == SkipNonExistentProjectsBehavior.Skip)
	{
	Log.LogMessageFromResources(MessageImportance.High, "MSBuild.ProjectFileNotFoundMessage", project.ItemSpec);
	}
	else
	{
	ErrorUtilities.VerifyThrow(skipNonExistProjects == SkipNonExistentProjectsBehavior.Error, "skipNonexistentProjects has unexpected value {0}", skipNonExistProjects);
	Log.LogErrorWithCodeFromResources("MSBuild.ProjectFileNotFound", project.ItemSpec);
	success = false;
	}

Update native File.Exists #9223

Are you sure you want to change the base?

Update native File.Exists #9223

Conversation

JaynieBai commented Sep 18, 2023 • edited Loading

Context

Changes Made

Testing

Notes

AR-May commented Oct 26, 2023

JaynieBai commented Nov 1, 2023

azure-pipelines bot commented Nov 1, 2023

f-alizada left a comment

Choose a reason for hiding this comment

AR-May Nov 20, 2023

Choose a reason for hiding this comment

JaynieBai Dec 29, 2023

Choose a reason for hiding this comment

MichalPavlik Sep 20, 2024

Choose a reason for hiding this comment

ladipro commented Apr 9, 2024

JaynieBai commented Apr 12, 2024 • edited Loading

AR-May commented Jun 12, 2024

JanKrivanek commented Jul 9, 2024

MichalPavlik commented Aug 29, 2024 • edited Loading

JanKrivanek commented Sep 2, 2024

MichalPavlik commented Sep 3, 2024 • edited Loading

MichalPavlik commented Sep 5, 2024 • edited Loading

JanKrivanek commented Sep 5, 2024

MichalPavlik commented Sep 6, 2024 • edited Loading

JaynieBai commented Sep 11, 2024

MichalPavlik commented Sep 18, 2024 • edited Loading

MichalPavlik commented Sep 19, 2024

rainersigwald commented Sep 19, 2024

JaynieBai commented Sep 18, 2023 •

edited

Loading

JaynieBai commented Apr 12, 2024 •

edited

Loading

MichalPavlik commented Aug 29, 2024 •

edited

Loading

MichalPavlik commented Sep 3, 2024 •

edited

Loading

MichalPavlik commented Sep 5, 2024 •

edited

Loading

MichalPavlik commented Sep 6, 2024 •

edited

Loading

MichalPavlik commented Sep 18, 2024 •

edited

Loading