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

Fix SendPacketsAsync failures on macos #62726

Merged

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Dec 13, 2021

While looking at the Windows failures reported in #62384, #60267, #60204, #60017 I noticed a bunch of new MacOS failures starting from 2021-12-08, which all report UnauthorizedAccessException

By default, LocalAppData is undefined on Unix, the following line will actually reference to a file named \NCLTest.Socket.SendPacketsAsync.testpayload in the working directory of the test execution process:

private string TestFileName = Environment.GetEnvironmentVariable("LocalAppData") + @"\NCLTest.Socket.SendPacketsAsync.testpayload";

On CI it's a known folder so the path points to stuff like /private/tmp/helix/working/A48D0921/w/C66409E5/e/System.Net.Sockets.Tests.app/Contents/Resources/\NCLTest.Socket.SendPacketsAsync.testpayload on Helix Macs. I'm not sure what goes wrong with the access to the above path, but I believe it's better to use the preferred TempFile utility to create temporary files, and this change will likely fix the UnauthorizedAccessException, since other tests using TempFile seem to be healthy right now.

Also changing the test to use the standard xunit test output instead of the legacy TestLogging.GetInstance().

@ghost
Copy link

ghost commented Dec 13, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

While looking at the Windows failures reported in #62384, #60267, #60204, #60017 I noticed a bunch of new MacOS failures starting from 2021-12-08, which all report UnauthorizedAccessException

By default, LocalAppData is undefined on Unix, the following line will actually reference to a file named \NCLTest.Socket.SendPacketsAsync.testpayload in the working directory of the test execution process:

private string TestFileName = Environment.GetEnvironmentVariable("LocalAppData") + @"\NCLTest.Socket.SendPacketsAsync.testpayload";

On CI it's a known folder so the path points to stuff like /private/tmp/helix/working/A48D0921/w/C66409E5/e/System.Net.Sockets.Tests.app/Contents/Resources/\NCLTest.Socket.SendPacketsAsync.testpayload on Helix Macs. I'm not sure what goes wrong with the access to the above folder, but I believe it's better to use the preferred TempFile utility to create temporary files, and this change will likely fix the UnauthorizedAccessException, since other tests using TempFile seem to be healthy right now.

Also changing the test to use the standard xunit test output instead of the legacy TestLogging.GetInstance().

Author: antonfirsov
Assignees: -
Labels:

area-System.Net.Sockets

Milestone: -

// Test payload file already exists.
_log.WriteLine("Payload file exists: {0}", TestFileName);
}
_tempFile = TempFile.Create(buffer);
Copy link
Member

Choose a reason for hiding this comment

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

One good aspect of the current approach is that Helix can cleanup on test or other failures. Since the run directory is unique this is very easy. I don't know if Helix actually does: cc @MattGal
Using TempFile.Create has potential of filling up temp space.
File name starting with '' seems weird but I don't know if that is relevant.

Copy link
Member Author

@antonfirsov antonfirsov Dec 13, 2021

Choose a reason for hiding this comment

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

Unless there is a hang or a crash, TempFile.Dispose() should delete the file after every test run:

public void Dispose()
{
_tempFile.Dispose();
}

If we want to be more robust, it should be a feature request towards Helix, so they cleanup Path.GetTempPath() after the test process finished. Plenty of BCL tests use the TempFile util especially System.IO.* stuff, I see no reason for keeping a custom approach in SendPacketsAsync.

@MattGal
Copy link
Member

MattGal commented Dec 13, 2021

If we want to be more robust, it should be a feature request towards Helix, so they cleanup Path.GetTempPath() after the test process finished

This is already true. The Temp path is redirected per work item to a fresh path to prevent collisions from different work coming in to the machines, and this path is cleaned up after the work item finishes running. If you see this not working it would be a bug; send along some details (correlation id of job / work item details should be plenty) to the dnceng team and it will get addressed.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@antonfirsov antonfirsov merged commit 522086a into dotnet:main Dec 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 13, 2022
@karelz karelz added this to the 7.0.0 milestone Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants