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

InMemoryDirectoryInfo is unexpectedly dependent on working directory due to its use of GetFullPath. #50648

Closed
BruceForstall opened this issue Apr 2, 2021 · 16 comments · Fixed by #50660 or #94528
Labels
arch-arm64 area-Extensions-FileSystem JitStress CLR JIT issues involving JIT internal stress modes os-windows
Milestone

Comments

@BruceForstall
Copy link
Member

From the runtime-coreclr libraries-jitstress pipeline:

https://dev.azure.com/dnceng/public/_build/results?buildId=1069450&view=ms.vss-test-web.build-test-results-tab

This test fails all Windows arm64 runs (including JITMinOpts=1), with:

COMPlus_JITMinOpts=1
COMPlus_TieredCompilation=0
D:\h\w\BAA10A61\w\A0300974\e>call RunTests.cmd --runtime-path D:\h\w\BAA10A61\p 
----- start Fri 04/02/2021  1:01:32.06 ===============  To repro directly: ===================================================== 
pushd D:\h\w\BAA10A61\w\A0300974\e\
"D:\h\w\BAA10A61\p\dotnet.exe" exec --runtimeconfig Microsoft.Extensions.FileSystemGlobbing.Tests.runtimeconfig.json --depsfile Microsoft.Extensions.FileSystemGlobbing.Tests.deps.json xunit.console.dll Microsoft.Extensions.FileSystemGlobbing.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing 
popd
===========================================================================================================

D:\h\w\BAA10A61\w\A0300974\e>"D:\h\w\BAA10A61\p\dotnet.exe" exec --runtimeconfig Microsoft.Extensions.FileSystemGlobbing.Tests.runtimeconfig.json --depsfile Microsoft.Extensions.FileSystemGlobbing.Tests.deps.json xunit.console.dll Microsoft.Extensions.FileSystemGlobbing.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: Microsoft.Extensions.FileSystemGlobbing.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  Microsoft.Extensions.FileSystemGlobbing.Tests (found 97 test cases)
  Starting:    Microsoft.Extensions.FileSystemGlobbing.Tests (parallel test collections = on, max threads = 8)
    Microsoft.Extensions.FileSystemGlobbing.Tests.FunctionalTests.VerifyInMemoryDirectoryInfo_IsNotEmpty [FAIL]
      Assert.Equal() Failure
      Expected: 1
      Actual:   0
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FunctionalTests.cs(858,0): at Microsoft.Extensions.FileSystemGlobbing.Tests.FunctionalTests.VerifyInMemoryDirectoryInfo_IsNotEmpty()
  Finished:    Microsoft.Extensions.FileSystemGlobbing.Tests
=== TEST EXECUTION SUMMARY ===
   Microsoft.Extensions.FileSystemGlobbing.Tests  Total: 343, Errors: 0, Failed: 1, Skipped: 0, Time: 7.984s
----- end Fri 04/02/2021  1:01:42.74 ----- exit code 1 ----------------------------------------------------------
@BruceForstall BruceForstall added arch-arm64 os-windows JitStress CLR JIT issues involving JIT internal stress modes area-Extensions-FileSystem labels Apr 2, 2021
@BruceForstall BruceForstall added this to the 6.0.0 milestone Apr 2, 2021
@ghost
Copy link

ghost commented Apr 2, 2021

Tagging subscribers to this area: @maryamariyan, @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

From the runtime-coreclr libraries-jitstress pipeline:

https://dev.azure.com/dnceng/public/_build/results?buildId=1069450&view=ms.vss-test-web.build-test-results-tab

This test fails all Windows arm64 runs (including JITMinOpts=1), with:

COMPlus_JITMinOpts=1
COMPlus_TieredCompilation=0
D:\h\w\BAA10A61\w\A0300974\e>call RunTests.cmd --runtime-path D:\h\w\BAA10A61\p 
----- start Fri 04/02/2021  1:01:32.06 ===============  To repro directly: ===================================================== 
pushd D:\h\w\BAA10A61\w\A0300974\e\
"D:\h\w\BAA10A61\p\dotnet.exe" exec --runtimeconfig Microsoft.Extensions.FileSystemGlobbing.Tests.runtimeconfig.json --depsfile Microsoft.Extensions.FileSystemGlobbing.Tests.deps.json xunit.console.dll Microsoft.Extensions.FileSystemGlobbing.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing 
popd
===========================================================================================================

D:\h\w\BAA10A61\w\A0300974\e>"D:\h\w\BAA10A61\p\dotnet.exe" exec --runtimeconfig Microsoft.Extensions.FileSystemGlobbing.Tests.runtimeconfig.json --depsfile Microsoft.Extensions.FileSystemGlobbing.Tests.deps.json xunit.console.dll Microsoft.Extensions.FileSystemGlobbing.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: Microsoft.Extensions.FileSystemGlobbing.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  Microsoft.Extensions.FileSystemGlobbing.Tests (found 97 test cases)
  Starting:    Microsoft.Extensions.FileSystemGlobbing.Tests (parallel test collections = on, max threads = 8)
    Microsoft.Extensions.FileSystemGlobbing.Tests.FunctionalTests.VerifyInMemoryDirectoryInfo_IsNotEmpty [FAIL]
      Assert.Equal() Failure
      Expected: 1
      Actual:   0
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FunctionalTests.cs(858,0): at Microsoft.Extensions.FileSystemGlobbing.Tests.FunctionalTests.VerifyInMemoryDirectoryInfo_IsNotEmpty()
  Finished:    Microsoft.Extensions.FileSystemGlobbing.Tests
=== TEST EXECUTION SUMMARY ===
   Microsoft.Extensions.FileSystemGlobbing.Tests  Total: 343, Errors: 0, Failed: 1, Skipped: 0, Time: 7.984s
----- end Fri 04/02/2021  1:01:42.74 ----- exit code 1 ----------------------------------------------------------
Author: BruceForstall
Assignees: -
Labels:

JitStress, arch-arm64, area-Extensions-FileSystem, os-windows

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 2, 2021
@BruceForstall BruceForstall added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-Extensions-FileSystem labels Apr 2, 2021
@BruceForstall
Copy link
Member Author

Marking codegen for now

@BruceForstall BruceForstall added area-Extensions-FileSystem and removed area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Apr 2, 2021
@ghost
Copy link

ghost commented Apr 2, 2021

Tagging subscribers to this area: @maryamariyan, @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

From the runtime-coreclr libraries-jitstress pipeline:

https://dev.azure.com/dnceng/public/_build/results?buildId=1069450&view=ms.vss-test-web.build-test-results-tab

This test fails all Windows arm64 runs (including JITMinOpts=1), with:

COMPlus_JITMinOpts=1
COMPlus_TieredCompilation=0
D:\h\w\BAA10A61\w\A0300974\e>call RunTests.cmd --runtime-path D:\h\w\BAA10A61\p 
----- start Fri 04/02/2021  1:01:32.06 ===============  To repro directly: ===================================================== 
pushd D:\h\w\BAA10A61\w\A0300974\e\
"D:\h\w\BAA10A61\p\dotnet.exe" exec --runtimeconfig Microsoft.Extensions.FileSystemGlobbing.Tests.runtimeconfig.json --depsfile Microsoft.Extensions.FileSystemGlobbing.Tests.deps.json xunit.console.dll Microsoft.Extensions.FileSystemGlobbing.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing 
popd
===========================================================================================================

D:\h\w\BAA10A61\w\A0300974\e>"D:\h\w\BAA10A61\p\dotnet.exe" exec --runtimeconfig Microsoft.Extensions.FileSystemGlobbing.Tests.runtimeconfig.json --depsfile Microsoft.Extensions.FileSystemGlobbing.Tests.deps.json xunit.console.dll Microsoft.Extensions.FileSystemGlobbing.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: Microsoft.Extensions.FileSystemGlobbing.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  Microsoft.Extensions.FileSystemGlobbing.Tests (found 97 test cases)
  Starting:    Microsoft.Extensions.FileSystemGlobbing.Tests (parallel test collections = on, max threads = 8)
    Microsoft.Extensions.FileSystemGlobbing.Tests.FunctionalTests.VerifyInMemoryDirectoryInfo_IsNotEmpty [FAIL]
      Assert.Equal() Failure
      Expected: 1
      Actual:   0
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FunctionalTests.cs(858,0): at Microsoft.Extensions.FileSystemGlobbing.Tests.FunctionalTests.VerifyInMemoryDirectoryInfo_IsNotEmpty()
  Finished:    Microsoft.Extensions.FileSystemGlobbing.Tests
=== TEST EXECUTION SUMMARY ===
   Microsoft.Extensions.FileSystemGlobbing.Tests  Total: 343, Errors: 0, Failed: 1, Skipped: 0, Time: 7.984s
----- end Fri 04/02/2021  1:01:42.74 ----- exit code 1 ----------------------------------------------------------
Author: BruceForstall
Assignees: -
Labels:

JitStress, arch-arm64, area-Extensions-FileSystem, os-windows, untriaged

Milestone: 6.0.0

@BruceForstall
Copy link
Member Author

This is due to #45139

@jozkee Can you please disable this test (or fix it) ASAP, so we don't see CI failures?

@ericstj
Copy link
Member

ericstj commented Apr 2, 2021

It's just as easy to fix it. Do a File.Exists check for that file before assuming the globbing API will find it. Ideally the test would create its own data but I can see how that might be hard here (without mocking) since it is testing the root. Spoke to soon, it looks like this type isn't supposed to even hit the disk. That's odd -- in that case it shouldn't matter if the file exists or not. One thing I notice is that the failing ARM64 run is happening on D:, perhaps that's leaking in somehow?

@ericstj
Copy link
Member

ericstj commented Apr 2, 2021

Looks like @jozkee might be out, I can submit a fix.

@ericstj
Copy link
Member

ericstj commented Apr 2, 2021

Here's the bug:

foreach (string file in files)
{
fileList.Add(Path.GetFullPath(file.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar)));
}

Here's the docs:

/// <summary>
/// Creates a new InMemoryDirectoryInfo with the root directory and files given.
/// </summary>
/// <param name="rootDir">The root directory that this FileSystem will use.</param>
/// <param name="files">Collection of file names. If relative paths <paramref name="rootDir"/> will be prepended to the paths.</param>
public InMemoryDirectoryInfo(string rootDir, IEnumerable<string> files)

But that call to GetFullPath isn't prepending rootDir, it ends up treating relative paths as relative to the app's working directory.

@ericstj
Copy link
Member

ericstj commented Apr 2, 2021

There are two other places where this is happening in the same type:

string normPath = Path.GetFullPath(path.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar));

string normPath = Path.GetFullPath(path.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar));

It feels very wrong for these to be dependent upon the working directory, but we could be misunderstanding the purpose of this type... Maybe it's just that the comment was wrong.

@BrennanConroy can you let us know what's expected here? Looks like its been this way since aspnet/FileSystem@767f07f

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 2, 2021
@carlossanlop carlossanlop self-assigned this Apr 2, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 2, 2021
@BruceForstall BruceForstall reopened this Apr 2, 2021
@BruceForstall
Copy link
Member Author

This needs to stay open to track the actual failure.

@ericstj ericstj changed the title Test failure: Microsoft.Extensions.FileSystemGlobbing.Tests.FunctionalTests.VerifyInMemoryDirectoryInfo_IsNotEmpty InMemoryDirectoryInfo is unexpectedly dependent on working directory due to its use of GetFullPath. Apr 5, 2021
@BrennanConroy
Copy link
Member

can you let us know what's expected here?

I don't really remember. I assume it should be checking if the file is absolute and if not adding rootDir.

@jozkee
Copy link
Member

jozkee commented Apr 8, 2021

Assuming that we could change InMemoryDirectoryInfo to no longer depend on Path.GetFullPath, what should FullName property return if someone instantiates InMemoryDirectoryInfo with a relative path? It currently returns Path.GetFullPath(yourRelativePath).

I guess we could prefix a dummy/hardcoded root. e.g:

var imdi = new InMemoryDirectoryInfo(@".", files: null);
Console.WriteLine(imdi.FullName); // Prints "C:\"

imdi = new InMemoryDirectoryInfo(@"myDirectory", files: null);
Console.WriteLine(imdi.FullName); // Prints "C:\myDirectory"

@ericstj
Copy link
Member

ericstj commented Apr 8, 2021

What does existing usage expect?

What if we didn’t remove usage of GetFullPath completely, but made sure to prepend rootDir to relative paths as the comment suggests?

@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Jul 9, 2021
@adamsitnik
Copy link
Member

@jozkee @carlossanlop were you able to identify the source of the problem? Is this a product bug and we should fix it in 6.0? Or a flaky test and 7.0 should be fine?

@jozkee
Copy link
Member

jozkee commented Aug 11, 2021

Will try to fix this in 6.0

@adamsitnik
Copy link
Member

We have moved it to 7.0 based on offline conversation

@krwq
Copy link
Member

krwq commented Oct 6, 2023

Here are the symptoms of usage:
#93107

basically because of


it's empty

@jozkee jozkee modified the milestones: Future, 9.0.0 Oct 25, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 8, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 28, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-arm64 area-Extensions-FileSystem JitStress CLR JIT issues involving JIT internal stress modes os-windows
Projects
None yet
9 participants