Skip to content
This repository has been archived by the owner on Apr 20, 2023. It is now read-only.

Fix Razor server shutdown on Windows. #9160

Merged
merged 1 commit into from
Apr 30, 2018
Merged

Fix Razor server shutdown on Windows. #9160

merged 1 commit into from
Apr 30, 2018

Conversation

peterhuene
Copy link

On Windows, the Razor server correctly creates the pid file with
FileAccess.Write and FileOptions.DeleteOnClose. This requires a share mode
of FileShare.Write | FileShare.Delete to open. However, the
dotnet build-server shutdown command was opening the file with
FileShare.Read. As a result, an IOException was being thrown and was not
handled.

This change first opens the file with the appropriate share access and also
properly handles a failure to access or read the contents of the pid file.

Additionally, an integration test was added to test that Razor server shutdown
works as expected.

Fixes #9158.

@peterhuene peterhuene added this to the 2.1.3xx milestone Apr 27, 2018
@peterhuene peterhuene requested a review from a team April 27, 2018 07:00
@peterhuene
Copy link
Author

peterhuene commented Apr 27, 2018

@MattGertz for approval (RTM)

Issues fixed

#9158

Description of Issue

Following a manual test pass on the dotnet build-server shutdown command, it was discovered that the command fails due to an unhandled exception when attempting to shut down a running Razor server. The reason for the failure was that dotnet was not opening the pid file created by the Razor server with the correct share access. This only affect Windows because file sharing violations are not implemented on macOS and Linux.

Customer Impact

If the customer has a running Razor server when executing dotnet build-server shutdown, the command will fail to shut down any servers and instead print an unhandled IOException. Customers can partially work around the issue by running dotnet build-server shutdown --msbuild --vbcscompiler to skip the shutdown of the Razor server, but still shut down the other types of servers.

Risk

Low - the fix is to change the FileShare flags passed when opening the file and handle exceptions that occur if the pid file cannot be accessed or read from.

Testing

The bug was not caught by automated tests because an integration test was not able to be implemented in time for RC due to being a cross-team feature. Additionally, the developer (me) did not test Razor shut down on Windows, only on macOS and Linux.

This PR implements an integration test that would have caught this failure. Manual tests were also run on macOS, Linux, and Windows to validate the change.

@MattGertz
Copy link

OK to take to .NET Core Shiproom.

}
catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException)
{
// Any failure to read the file will ignore the file

This comment was marked as spam.

This comment was marked as spam.

@peterhuene
Copy link
Author

It looks like the integration test potentially interfered with another CI process that was running a Razor server. I'm going to have to think about how to isolate this or disable the test.

string.Format(
LocalizableStrings.ShutDownSucceededWithPid,
CommandLocalizableStrings.RazorServer,
@"\d+").Replace("(", @"\(").Replace(")", @"\)"));

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

On Windows, the Razor server correctly creates the pid file with
`FileAccess.Write` and `FileOptions.DeleteOnClose`.  This requires a share mode
of `FileShare.Write | FileShare.Delete` to open.  However, the
`dotnet build-server shutdown` command was opening the file with
`FileShare.Read`.  As a result, an `IOException` was being thrown and was not
handled.

This change first opens the file with the appropriate share access and also
properly handles a failure to access or read the contents of the pid file.

Additionally, an integration test was added to test that Razor server shutdown
works as expected.

Fixes #9158.
@peterhuene
Copy link
Author

Okay, pushed up a fix for the integration test. The Razor server now uses a unique pipe name and pid file directory, which should effectively isolate it from any other CI process.

@wli3 wli3 self-requested a review April 30, 2018 19:13
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.

3 participants