Remove PathTooLongException and associated tests. #14124

stephenmichaelf · 2016-11-30T13:48:14Z

Remove preemptive check for path too long. Refer to #8655 .

karelz

Overall we should IMO keep the tests which tests long path. Mainly we want to ensure that the OS APIs are translated into the right exception (should be backward compatible).
The tests won't have fixed file name, but can grow the file name until they hit the OS limit.

karelz · 2016-11-30T16:44:13Z

src/System.IO.FileSystem/tests/Directory/CreateDirectory.cs

-                Assert.Throws<PathTooLongException>(() => Create(path));
-            });
-        }
-


I think we should keep a test which checks the long path (even though it is "variable" and not known ahead of time).

I'm not sure what the OS thinks in this case (with a long segment). It might not consider them too long and just "invalid". 255 is a limit that does vary (sometimes is smaller, but no known bigger cases). We should have a test just to validate they fail, but I'm not too concerned about which exception it is, just that it is consistent.

The exception should be compatible with Desktop for compat.

The exception should be compatible with Desktop for compat.

It can't be by design.

Windows doesn't always return "TooLong". The whole point of this is to not second-guess the OS. We already get this wrong for some file system formats and we're currently blocking any new ones that might support > 255.

"Compat" with this change is we should get an IOException of some sort.

A bit more detail. "TooLong" from the OS typically has to do with not being able to allocate a string that is large enough. This doesn't necessarily have to happen with a path you pass in, at least not before it fails for other reasons. So you may have \\?\C:\Foo\DoesNotExist\{somesuperlongsegment} and the error you get may be "DirectoryNotFound" as it navigates the path.

255 as a max isn't always true- we already get it wrong (it is shorter in a number of cases I'm aware of) and there is no technical reason that there can't be existing or new device drivers that support segments that are longer than that.

To clarify for this test, are you saying we should add the test back and just expect a different type of IOException?

Yes- these exceptions may change, but as long as they're some type of IOException we should be ok.

That means we should have tests growing the path length until an IOException happens, right?

JeremyKuhne

I'd keep the tests running, but updated. I don't see PathHelper.Windows.cs, which has PathTooLong in a number of places- you should also be able to stop keeping track of the segment length in there as a result.

JeremyKuhne · 2016-11-30T18:57:10Z

src/System.IO.FileSystem/tests/Directory/CreateDirectory.cs

-                Assert.Throws<PathTooLongException>(() => Create(path));
-            });
-        }
-


I'm not sure what the OS thinks in this case (with a long segment). It might not consider them too long and just "invalid". 255 is a limit that does vary (sometimes is smaller, but no known bigger cases). We should have a test just to validate they fail, but I'm not too concerned about which exception it is, just that it is consistent.

JeremyKuhne · 2016-11-30T18:58:52Z

src/System.IO.FileSystem/tests/Directory/CreateDirectory.cs

-        [ActiveIssue(11687)]
-        [PlatformSpecific(TestPlatforms.Windows)]
-        public void DirectoryLongerThanMaxLongPath_ThrowsPathTooLongException()
-        {


These ones should still be throwing the same exception.

JeremyKuhne · 2016-11-30T19:00:02Z

src/System.IO.FileSystem/tests/Directory/GetFileSystemEntries_str_str.cs

-        [PlatformSpecific(TestPlatforms.Windows)]
-        public void WindowsSearchPatternLongSegment()
-        {
-            // Create a path segment longer than the normal max of 255


Again, on this one I don't care what it throws, just as long as it fails with some sort of IO exception.

JeremyKuhne · 2016-11-30T19:03:36Z

src/System.IO.FileSystem/tests/Directory/Move.cs

-        {
-            string testDir = GetTestFilePath();
-            Directory.CreateDirectory(testDir);
-            Assert.All((IOInputs.GetPathsLongerThanMaxLongPath(GetTestFilePath())), (path) =>


This should continue to throw in some way. I would expect either "too long" or "not found" or possibly "invalid" for any of these long paths. It depends on where they get kicked back in the system.

JeremyKuhne · 2016-11-30T19:05:17Z

src/System.IO.FileSystem/tests/File/Create.cs

-            DirectoryInfo testDir = Directory.CreateDirectory(GetTestFilePath());
-            Assert.Throws<PathTooLongException>(() => Create(Path.Combine(testDir.FullName, new string('a', 300))));
-
-            //TODO #645: File creation does not yet have long path support on Unix or Windows


This is a weird commented out blob. It should just be deleted- it should fail with segments that are this long.

JeremyKuhne · 2016-11-30T19:05:37Z

src/System.IO.FileSystem/tests/File/Create.cs

-        public void LongPath()
-        {
-            DirectoryInfo testDir = Directory.CreateDirectory(GetTestFilePath());
-            Assert.Throws<PathTooLongException>(() => Create(Path.Combine(testDir.FullName, new string('a', 300))));


Again, should throw something.

JeremyKuhne · 2016-11-30T19:06:01Z

src/System.IO.FileSystem/tests/File/Move.cs

-
-            Assert.All(IOInputs.GetPathsLongerThanMaxLongPath(GetTestFilePath()), (path) =>
-            {
-                Assert.Throws<PathTooLongException>(() => Move(testFileSource, path));


Again, some exception

For PathHelpers.Windows.cs I only saw it in one place and that change is in here:

I made the other changes and I am testing them now.

https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/src/System/IO/PathHelper.Windows.cs#L72 etc.

…pes.

JeremyKuhne · 2016-12-01T03:34:25Z

src/System.IO.FileSystem/tests/Directory/CreateDirectory.cs

            Assert.All(paths, (path) =>
            {
                Assert.Throws<PathTooLongException>(() => Create(path));
            });
        }

-        #endregion
+#endregion


You've lost some indenting here apparently

JeremyKuhne · 2016-12-01T03:48:54Z

What errors did you actually get back? Specifically in GetExceptionForWin32Error?

We should validate the HResult is what we expect and/or if it is something "new" that fits one of the existing cases we might want to add it. Specifically I'm wondering if ERROR_BUFFER_OVERFLOW is what we're seeing (206)- which we should probably add as another case for PathTooLong.

JeremyKuhne · 2016-12-01T04:01:21Z

Looking through things a bit, one other one I would expect is ERROR_BAD_PATHNAME (161). That one should probably stay as an IOException.

stephenmichaelf · 2016-12-01T13:57:42Z

I am trying to debug in VS but having issues. I keep getting inconsistent results, I can do the following:

msbuild System.IO.FileSystem.Tests.builds

I used to be able to run the main build:

build-managed

But now it's failing randomly with:

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Platforms\x64\PlatformToolsets\v140\Toolset.targets(36,5): error MSB8036: The Windows SDK version 8.1 was not found. Install the required version of Windows SDK or change the SDK version in the project property pages or by right-clicking the solution and selecting "Retarget solution". D:\OpenSource\corefx\src\System.ServiceProcess.ServiceController\tests\System.ServiceProcess.ServiceController.TestNativeService\System.ServiceProcess.ServiceController.TestNativeService.vcxproj]
Command execution failed with exit code 1.

Then in Visual Studio I can build the solution fine:

========== Build: 2 succeeded, 0 failed, 2 up-to-date, 0 skipped ==========

But I am seeing "errors" in the files and I see nothing in Test Explorer:

I am using Debug build:

I looked and it seems like the xUnit test runner VSIX is deprecated and that it should be finding my tests. Any ideas what I am doing wrong?

JeremyKuhne · 2016-12-02T19:33:53Z

Sorry about the delayed response, everyone is OOF with an office move.

Unfortunately our debugging experience / instructions are less than optimal at the moment. @weshaggard, am I missing any documentation on this that you are aware of? I'm going to open some issues on documenting and improving the experience (particularly from the IDE).

Use the Windows_Debug config, you'll see the intellisense/syntax highlighting light up correctly.

The Test Explorer is not currently usable with CoreFX projects. Right click on the test project you want to debug and set it as the startup project. The filtering isn't quite right so you'll need to tweak things a bit before you can run.

If you build the relevant test .builds from the command line you'll get a dump something like the following:

  Executing in P:\repos\corefx\bin\tests\Windows_NT.AnyCPU.Debug\System.Runtime.Extensions.Tests\default.netcoreapp1.1\
  Hard linking dependent files...
  Finished linking needed files, moving to running tests.
  Running tests... Start time: 10:58:09.16
  Command(s):
  call CoreRun.exe xunit.console.netcore.exe System.Runtime.Extensions.Tests.dll  -xml testResults.xml -notrait Benchmark=
  true -notrait category=nonnetcoreapp1.1tests  -notrait category=OuterLoop -notrait category=failing -notrait category=no
  nwindowstests
  xUnit.net console test runner (64-bit .NET Core)
  Copyright (C) 2014 Outercurve Foundation.

In the debug tab for the properties for the test project you'll see these values. You'll want to tweak the existing command line arguments from

xunit.console.netcore.exe System.Runtime.Extensions.Tests.dll -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapp1.1tests -wait -parallel none

to whatever you get from the command prompt plus -wait -parallel none:

xunit.console.netcore.exe System.Runtime.Extensions.Tests.dll  -xml testResults.xml -notrait Benchmark=true -notrait category=nonnetcoreapp1.1tests  -notrait category=OuterLoop -notrait category=failing -notrait category=nonwindowstests -wait -parallel none

Then you'll be able to set breakpoints and F5 to run/debug. Note that I'm not sure why it won't run the Windows specific tests, I'll look more next week. In the meantime, just pull the attribute while you're testing.

The developer guide has instructions that you can go through.

danmoseley · 2016-12-03T07:04:22Z

This is how I debug tests. Make sure VS is on the path (eg use a developer command prompt) then:

msbuild thetestproject.csproj /t:buildandtest /p:testdebugger=devenv.exe

When Xunit starts it will pop the debugger. First time around it will not guess the correct debug engine, you must open solution properties and change it to the Core debug engine (I am using VS2015) Now set your breakpoints and hit F5.

You can Save the solution and open that solution next time and F5 it will just work without need to reset debugger engine or breakpoints.

stephenmichaelf · 2016-12-04T16:57:44Z

@JeremyKuhne thanks for taking the time to write that up, I appreciate it.

Setting the build to Windows_Debug did fix the intellisense issue.

I also changed the project properties debug tab setting and that is letting me debug in VS.

One strange thing I am seeing is that when exceptions are thrown, that I believe are supposed to be thrown, the debugger is displaying and highlighting them. If I continue it's fine and the tests continue to run. Not sure why that is happening. I checked my exception settings and this is what I have:

I also see this option:

But it seems problematic for times when I want to see that this type of exception was thrown when it shouldn't be. This is making it super slow to debug :) Is there a setting I need to change?

Here are some of the Win32Errors I am seeing(sorry I missed the path on the first two):

= Directory/GetFileSystemEntries_str_str.cs
WindowsSearchPatternLongSegment()
errorCode = 183 = 0xB7 = ERROR_ALREADY_EXISTS -> So it's throwing an IOException since it goes to default case.
errorCode = 87 = 0x57= ERROR_INVALID_PARAMETER -> Throwing IOException under case statement.

= File/Create.cs
LongPath()
This one appears to be throwing a PathTooLongException from within the FileStream constructor in File.Create(). I had a breakpoint set on the switch in GetExceptionForWin32Error and it didn't trigger but the code still threw.

Thanks again for taking the time to walk me through this.

stephenmichaelf · 2016-12-04T19:10:46Z

One more question.

What is the process for debugging on other OSes? I see there is a failure in Jenkins:

/mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_release_prtest/Tools/tests.targets(257,5): error : One or more tests failed while running tests from 'System.IO.FileSystem.Tests' please check /mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_release_prtest/bin/tests/Unix.AnyCPU.Release/System.IO.FileSystem.Tests/default.netcoreapp1.1/testResults.xml for details! [/mnt/j/workspace/dotnet_corefx/master/ubuntu14.04_release_prtest/src/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj]

But I would guess we can't just remote in and check that XML file :)

danmoseley · 2016-12-08T02:36:45Z

@alexperovich can you point to your instructions for debugging a unit test on unix?

karelz · 2016-12-20T01:12:33Z

@alexperovich ping? @JeremyKuhne can you please help? The PR is stuck for 2 weeks :(

karelz · 2017-01-04T18:21:32Z

@alexperovich @JeremyKuhne ping - the PR is stuck for 1 month :(

alexperovich · 2017-01-04T18:37:45Z

@stephenmichaelf @danmosemsft Sorry for the delay. I was on vacation. Instructions for debugging on unix are here: https://github.com/dotnet/corefx/blob/master/Documentation/debugging/unix-instructions.md

JeremyKuhne · 2017-01-04T19:28:30Z

@dotnet-bot test this please

JeremyKuhne · 2017-01-04T19:36:57Z

Kicking the CI again as the results were deleted.

@stephenmichaelf Sorry about the holiday delays. For first chance exceptions you want to have them off generally- this case in particular is a pain as the ones you care about are hit all the time. What I would do is set a breakpoint in the code that converts the windows error and at the beginning of the tests you care about. Disable/Enable the the error conversion breakpoint when you enter/leave the test you care about.

You can also specify arbitrary tests to run in the command line. This link has more details.

stephenmichaelf · 2017-01-06T00:34:43Z

@karelz @alexperovich @JeremyKuhne Thanks everyone for the help! I will take a look at this again.

stephenmichaelf · 2017-01-06T13:59:50Z

Thank you for the advice it was very helpful, I am now able to run the tests one by one from VS.

@JeremyKuhne I was debugging the other OSes and I see that the failures in Unix are being throw by:

FileSystem.Current.MoveFile()

Which throws and then checks /Common/Interop/Unix/Interop.IOErrors.cs:

internal static Exception GetExceptionForIoErrno(ErrorInfo errorInfo, string path = null, bool isDirectory = false)
{
	...
	case Error.ENAMETOOLONG:
		return new PathTooLongException(SR.IO_PathTooLong);
	...
}

Should I get rid of that case and let it fall through to the default which throws an IOException? Is that the behavior we are looking for?

I was doing more debugging and I found more places that the PathTooLongException is being thrown in PathHelper.Windows.cs and removed them.

After I removed them I started to get unhandled exceptions there:

An exception of type 'System.IO.PathTooLongException' occurred in system.runtime.extensions.dll but was not handled in user code.

It doesn't seem like these are being caught and converted now but I was under the impression we should remove all PathTooLong throws that aren't in conversion code.

I will now check the codes they are throwing in GetExceptionForWin32Error.

Am I making this change harder than it has to be or is it just tricky? :)

karelz · 2017-01-10T02:05:30Z

@JeremyKuhne was OOF sick today. CC @ianhays if he can help ...

JeremyKuhne · 2017-01-11T01:35:59Z

Should I get rid of that case and let it fall through to the default which throws an IOException? Is that the behavior we are looking for?

That is the Unix specific helper for errors and shouldn't be changed. We want to surface PathTooLong if the OS tells us that the path is too long. Win32Marshal.GetExceptionForWin32Error() is the Windows equivalent.

The Path class code moved to CoreCLR you'll need to update Path there. The only reason I didn't delete it here is that CoreRT (the WinRT CoreCLR) doesn't have the equivalent move (yet). I've put up a PR to rename these files to reflect this better (they have .uap now).

danmoseley · 2017-01-17T19:00:16Z

@stephenmichaelf do you plan to continue in the manner @JeremyKuhne proposes?

stephenmichaelf · 2017-01-18T19:30:54Z

@danmosemsft @JeremyKuhne Yes I can do that.

Just to clarify I should:

Get all of the codes for GetExceptionForWin32Error and list them here
Remove PathTooLongException from the Path object in CoreCLr
Anything else?
How do we want to handle the breaking tests in non-Windows? They are still throwing PathTooLongException but we updated the Windows tests(which are passing) to expect an IOException now.

Thanks!

JeremyKuhne · 2017-01-18T19:47:42Z

Get all of the codes for GetExceptionForWin32Error and list them here

I'd add ERROR_BUFFER_OVERFLOW (206) in that code as another case for PathTooLong. To get the PathTooLong exceptions you might need to create a > short.MaxValue character path to get that to happen for GetFullPath. You should be able to contrive a test case that causes PathTooLong for the other APIs- they might need to have an actual path created that is < short.MaxValue with a final file/directory name that pushes the path over.

Anything else?

If you add ERROR_BUFFER_OVERFLOW you'll need to do the same in the helper in CoreCLR- __Error is same code. (Btw, feel free to make the name of the helper match CoreFX- we should ultimately share the code here.)

karelz · 2017-01-25T20:48:22Z

@stephenmichaelf was the previous answer sufficient?

stephenmichaelf · 2017-01-27T12:18:37Z

@karelz Yes I think it is, thanks! I will have a push for this tomorrow.

karelz · 2017-02-06T18:34:50Z

@stephenmichaelf ping?

karelz · 2017-02-13T16:47:19Z

@stephenmichaelf ping again?

danmoseley · 2017-02-18T01:28:17Z

@stephenmichaelf would love to see this go in, we get a lot of feedback about long path limits...

danmoseley · 2017-02-24T22:59:54Z

I'm going to close this one as it's been a month. We can certainly reopen if anyone would like to finish it off.

karelz · 2017-02-24T23:14:42Z

BTW: I unassigned the bug. If anyone wants to finish it off later (incl. @stephenmichaelf), please let us know on the main issue #8655.

Remove PathTooLongException and associated tests.

86c6c0f

dnfclas added the cla-already-signed label Nov 30, 2016

karelz assigned stephenmichaelf and JeremyKuhne Nov 30, 2016

karelz added the area-System.IO label Nov 30, 2016

karelz reviewed Nov 30, 2016

View reviewed changes

JeremyKuhne suggested changes Nov 30, 2016

View reviewed changes

Adding back tests from code review feedback and changing exception ty…

53c304f

…pes.

JeremyKuhne reviewed Dec 1, 2016

View reviewed changes

Fix spacing.

488a5ea

karelz modified the milestone: 1.2.0 Dec 6, 2016

Remove additional throws of PathTooLong.

d4c64c6

danmoseley closed this Feb 24, 2017

am11 mentioned this pull request Mar 5, 2017

MSBuild should handle paths longer than MAX_PATH dotnet/msbuild#53

Closed

Remove PathTooLongException and associated tests. #14124

Remove PathTooLongException and associated tests. #14124

Conversation

stephenmichaelf commented Nov 30, 2016

karelz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyKuhne left a 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

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

JeremyKuhne commented Dec 1, 2016

JeremyKuhne commented Dec 1, 2016

stephenmichaelf commented Dec 1, 2016

JeremyKuhne commented Dec 2, 2016

danmoseley commented Dec 3, 2016

stephenmichaelf commented Dec 4, 2016 • edited Loading

stephenmichaelf commented Dec 4, 2016

danmoseley commented Dec 8, 2016

karelz commented Dec 20, 2016

karelz commented Jan 4, 2017

alexperovich commented Jan 4, 2017

JeremyKuhne commented Jan 4, 2017

JeremyKuhne commented Jan 4, 2017

stephenmichaelf commented Jan 6, 2017

stephenmichaelf commented Jan 6, 2017

karelz commented Jan 10, 2017

JeremyKuhne commented Jan 11, 2017

danmoseley commented Jan 17, 2017

stephenmichaelf commented Jan 18, 2017

JeremyKuhne commented Jan 18, 2017

karelz commented Jan 25, 2017

stephenmichaelf commented Jan 27, 2017

karelz commented Feb 6, 2017

karelz commented Feb 13, 2017

danmoseley commented Feb 18, 2017

danmoseley commented Feb 24, 2017

karelz commented Feb 24, 2017

stephenmichaelf commented Dec 4, 2016 •

edited

Loading