Merge pull request #283 from murphymcquet-msft/dev/murphymcquet/avoid…

…SpinlockPipeConnect

Avoid spinlock pipe connect on Windows

src/Microsoft.ServiceHub.Framework/AsyncNamedPipeClientStream.cs

@@ -0,0 +1,200 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System.IO.Pipes;
+using System.Runtime.InteropServices;
+using System.Runtime.Versioning;
+using System.Security;
+using System.Security.Principal;
+using Microsoft.Win32.SafeHandles;
+using Windows.Win32.Foundation;
+using Windows.Win32.Storage.FileSystem;
+using static Windows.Win32.PInvoke;
+
+namespace Microsoft.ServiceHub.Framework;
+
+/// <summary>
+/// Named pipe client that avoids TimeoutExceptions and burning CPU.
+/// </summary>
+[SupportedOSPlatform("windows")]
+internal class AsyncNamedPipeClientStream : PipeStream
+{
+	private const FILE_SHARE_MODE SharingFlags = FILE_SHARE_MODE.FILE_SHARE_READ | FILE_SHARE_MODE.FILE_SHARE_WRITE | FILE_SHARE_MODE.FILE_SHARE_DELETE;
+
+	private readonly string pipePath;
+	private readonly TokenImpersonationLevel impersonationLevel;
+	private readonly PipeOptions pipeOptions;
+	private readonly PipeDirection direction;
+	private readonly uint access;
+
+	/// <summary>
+	/// Initializes a new instance of the <see cref="AsyncNamedPipeClientStream"/> class.
+	/// </summary>
+	/// <param name="serverName">Pipe server name.</param>
+	/// <param name="pipeName">Pipe name.</param>
+	/// <param name="direction">Communication direction.</param>
+	/// <param name="options">Pipe options.</param>
+	/// <param name="impersonationLevel">Impersonation level.</param>
+	internal AsyncNamedPipeClientStream(
+		string serverName,
+		string pipeName,
+		PipeDirection direction,
+		PipeOptions options,
+		TokenImpersonationLevel impersonationLevel = TokenImpersonationLevel.None)
+		: base(direction, 4096)
+	{
+		Requires.NotNullOrEmpty(serverName);
+		Requires.NotNullOrEmpty(pipeName);
+
+		this.pipePath = $@"\\{serverName}\pipe\{pipeName}";
+		this.direction = direction;
+		this.pipeOptions = options;
+		this.impersonationLevel = impersonationLevel;
+		this.access = GetAccess(direction);
+	}
+
+	/// <summary>
+	/// Wrapper for CreateFile to create a named pipe client.
+	/// Previous attempts used CreateFile directly, but for unknown reasons there were issues with the pipe handle.
+	/// </summary>
+	/// <returns>A handle to the named pipe.</returns>
+	/// <param name="lpFileName">The name of the pipe.</param>
+	/// <param name="dwDesiredAccess">The requested access to the file or device.</param>
+	/// <param name="dwShareMode">The requested sharing mode of the file or device.</param>
+	/// <param name="securityAttributes">A SECURITY_ATTRIBUTES structure.</param>
+	/// <param name="dwCreationDisposition">The action to take on files that exist, and on files that do not exist.</param>
+	/// <param name="dwFlagsAndAttributes">The file attributes and flags.</param>
+	/// <param name="hTemplateFile">A handle to a template file with the GENERIC_READ access right.</param>
+	[DllImport("kernel32.dll", EntryPoint = "CreateFile", CharSet = CharSet.Auto, SetLastError = true, BestFitMapping = false)]
+	[SecurityCritical]
+	internal static extern SafePipeHandle CreateNamedPipeClient(
+		string lpFileName,
+		int dwDesiredAccess,
+		System.IO.FileShare dwShareMode,
+		Windows.Win32.Security.SECURITY_ATTRIBUTES securityAttributes,
+		System.IO.FileMode dwCreationDisposition,
+		int dwFlagsAndAttributes,
+		IntPtr hTemplateFile);
+
+	/// <summary>
+	/// Connects pipe client to server.
+	/// </summary>
+	/// <param name="maxRetries">Maximum number of retries.</param>
+	/// <param name="retryDelayMs">Milliseconds delay between retries.</param>
+	/// <param name="cancellationToken">Cancellation token.</param>
+	/// <returns>A task representing the establishment of the client connection.</returns>
+	/// <exception cref="TimeoutException">Thrown if no connection can be established.</exception>
+	internal async Task ConnectAsync(
+		int maxRetries,
+		int retryDelayMs,
+		CancellationToken cancellationToken)
+	{
+		var errorCodeMap = new Dictionary<int, int>();
+		int retries = 0;
+		while (!cancellationToken.IsCancellationRequested)
+		{
+			if (retries > maxRetries || this.TryConnect())
+			{
+				break;
+			}
+
+			retries++;
+			var errorCode = Marshal.GetLastWin32Error();
+			errorCodeMap[errorCode] = errorCodeMap.ContainsKey(errorCode) ? errorCodeMap[errorCode] + 1 : 1;
+
+			await Task.Delay(retryDelayMs, cancellationToken).ConfigureAwait(false);
+		}
+
+		// Throw if cancellation requested, otherwise throw a TimeoutException
+		cancellationToken.ThrowIfCancellationRequested();
+
+		if (retries > maxRetries || !this.IsConnected)
+		{
+			throw new TimeoutException($"Failed with errors: {string.Join(", ", errorCodeMap.Select(x => $"(code: {x.Key}, count: {x.Value})"))}");
+		}
+	}
+
+	private static uint GetAccess(PipeDirection direction)
+	{
+		uint access = 0;
+		if ((PipeDirection.In & direction) == PipeDirection.In)
+		{
+			access |= (uint)GENERIC_ACCESS_RIGHTS.GENERIC_READ;
+		}
+
+		if ((PipeDirection.Out & direction) == PipeDirection.Out)
+		{
+			access |= (uint)GENERIC_ACCESS_RIGHTS.GENERIC_WRITE;
+		}
+
+		return access;
+	}
+
+	private bool TryConnect()
+	{
+		// Immediately return if the pipe is not available
+		if (!WaitNamedPipe(this.pipePath, 1))
+		{
+			return false;
+		}
+
+		SafePipeHandle handle = CreateNamedPipeClient(
+			this.pipePath,
+			(int)this.access,
+			FileShare.ReadWrite | FileShare.Delete,
+			this.GetSecurityAttributes(true),
+			System.IO.FileMode.Open,
+			(int)this.pipeOptions,
+			IntPtr.Zero);
+
+		if (handle.IsInvalid)
+		{
+			handle.Dispose();
+			return false;
+		}
+
+		// Success!
+		this.InitializeHandle(handle, false, true);
+		this.IsConnected = true;
+		this.ValidateRemotePipeUser();
+		return true;
+	}
+
+	private void ValidateRemotePipeUser()
+	{
+#if NETFRAMEWORK
+		var isCurrentUserOnly = (this.pipeOptions & PolyfillExtensions.PipeOptionsCurrentUserOnly) == PolyfillExtensions.PipeOptionsCurrentUserOnly;
+#elif NET5_0_OR_GREATER
+		var isCurrentUserOnly = (this.pipeOptions & PipeOptions.CurrentUserOnly) == PipeOptions.CurrentUserOnly;
+#endif
+
+#if NETFRAMEWORK || NET5_0_OR_GREATER
+		// No validation needed - pipe is not restricted to current user
+		if (!isCurrentUserOnly)
+		{
+			return;
+		}
+
+		System.IO.Pipes.PipeSecurity accessControl = this.GetAccessControl();
+		IdentityReference? remoteOwnerSid = accessControl.GetOwner(typeof(SecurityIdentifier));
+		using WindowsIdentity currentIdentity = WindowsIdentity.GetCurrent();
+		SecurityIdentifier? currentUserSid = currentIdentity.Owner;
+		if (remoteOwnerSid != currentUserSid)
+		{
+			this.IsConnected = false;
+			throw new UnauthorizedAccessException();
+		}
+#endif
+	}
+
+	private Windows.Win32.Security.SECURITY_ATTRIBUTES GetSecurityAttributes(bool inheritable)
+	{
+		var secAttr = new Windows.Win32.Security.SECURITY_ATTRIBUTES
+		{
+			bInheritHandle = inheritable,
+			nLength = (uint)Marshal.SizeOf(typeof(Windows.Win32.Security.SECURITY_ATTRIBUTES)),
+		};
+
+		return secAttr;
+	}
+}

src/Microsoft.ServiceHub.Framework/GeneratorOverrides/UnscopedRefAttribute.cs

@@ -0,0 +1,16 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+namespace System.Diagnostics.CodeAnalysis;
+
+/// <summary>
+/// Used to indicate a byref escapes and is not scoped.
+/// </summary>
+/// <devremarks>
+/// Prevents conflict between PolySharp and CsWin generation of UnscopedRefAttribute.
+/// </devremarks>
+[AttributeUsageAttribute(AttributeTargets.Method | AttributeTargets.Property | AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
+internal sealed class UnscopedRefAttribute
+	: Attribute
+{
+}

src/Microsoft.ServiceHub.Framework/NativeMethods.txt

@@ -1,3 +1,10 @@
+CreateFile
+CreateNamedPipeA
+GENERIC_ACCESS_RIGHTS
 GetNamedPipeClientProcessId
+GetVersionEx
 HRESULT_FROM_WIN32
 NMPWAIT_NOWAIT
+OSVERSIONINFOEXW
+OSVERSIONINFOW
+WaitNamedPipe

src/Microsoft.ServiceHub.Framework/PublicAPI.Unshipped.txt

@@ -5,4 +5,4 @@ Microsoft.VisualStudio.Shell.ServiceBroker.ExportBrokeredServiceAttribute.Export
 Microsoft.VisualStudio.Shell.ServiceBroker.ExportBrokeredServiceAttribute.OptionalInterfacesImplemented.get -> string![]!
 Microsoft.VisualStudio.Utilities.ServiceBroker.GlobalBrokeredServiceContainer.TraceEvents.AdditionalProxyInterfaceTypeLoadFailure = 5 -> Microsoft.VisualStudio.Utilities.ServiceBroker.GlobalBrokeredServiceContainer.TraceEvents
 Microsoft.VisualStudio.Utilities.ServiceBroker.ServiceRegistration.AdditionalServiceInterfaceTypeNames.get -> System.Collections.Immutable.ImmutableArray<string!>
-Microsoft.VisualStudio.Utilities.ServiceBroker.ServiceRegistration.AdditionalServiceInterfaceTypeNames.init -> void
+Microsoft.VisualStudio.Utilities.ServiceBroker.ServiceRegistration.AdditionalServiceInterfaceTypeNames.init -> void

src/Microsoft.ServiceHub.Framework/ServerFactory.cs

@@ -5,6 +5,7 @@
 using System.Diagnostics;
 using System.IO.Pipes;
 using System.Runtime.InteropServices;
+using System.Runtime.Versioning;
 using System.Security.Principal;
 using Windows.Win32.Foundation;
 using static Windows.Win32.PInvoke;
@@ -25,7 +26,7 @@ public static class ServerFactory
 	internal const PipeOptions StandardPipeOptions = PipeOptions.Asynchronous | PolyfillExtensions.PipeOptionsCurrentUserOnly;
 #endif
 
-	private const int ConnectRetryIntervalMs = 20;
+	private const int ConnectRetryIntervalMs = 50;
 	private const int MaxRetryAttemptsForFileNotFoundException = 3;
 	private static readonly string PipePrefix = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? @"\\.\pipe" : Path.GetTempPath();
 
@@ -96,16 +97,31 @@ public static async Task<Stream> ConnectAsync(string pipeName, ClientOptions opt
 		// that were not defined in the enumeration.
 		pipeOptions &= ~PolyfillExtensions.PipeOptionsCurrentUserOnly;
 #endif
-
-		NamedPipeClientStream pipeStream = new(".", TrimWindowsPrefixForDotNet(pipeName), PipeDirection.InOut, pipeOptions);
+		var name = TrimWindowsPrefixForDotNet(pipeName);
+		var maxRetries = options.FailFast ? 0 : int.MaxValue;
+		PipeStream? pipeStream = null;
 		try
 		{
-			await ConnectWithRetryAsync(pipeStream, fullPipeOptions, cancellationToken, maxRetries: options.FailFast ? 0 : int.MaxValue, withSpinningWait: options.CpuSpinOverFirstChanceExceptions).ConfigureAwait(false);
+			if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+			{
+				pipeStream = new AsyncNamedPipeClientStream(".", name, PipeDirection.InOut, pipeOptions);
+				await ((AsyncNamedPipeClientStream)pipeStream).ConnectAsync(maxRetries, ConnectRetryIntervalMs, cancellationToken).ConfigureAwait(false);
+			}
+			else
+			{
+				pipeStream = new NamedPipeClientStream(".", name, PipeDirection.InOut, pipeOptions);
+				await ConnectWithRetryAsync((NamedPipeClientStream)pipeStream, fullPipeOptions, cancellationToken, maxRetries, withSpinningWait: options.CpuSpinOverFirstChanceExceptions).ConfigureAwait(false);
+			}
+
 			return pipeStream;
 		}
 		catch
 		{
-			await pipeStream.DisposeAsync().ConfigureAwait(false);
+			if (pipeStream is not null)
+			{
+				await pipeStream.DisposeAsync().ConfigureAwait(false);
+			}
+
 			throw;
 		}
 	}

test/Microsoft.ServiceHub.Framework.Tests/ServerFactoryTests.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using System.Diagnostics;
+using System.Runtime.InteropServices;
 using Microsoft.ServiceHub.Framework;
 using Microsoft.VisualStudio.Threading;
 using Xunit;
@@ -13,6 +15,38 @@ public ServerFactoryTests(ITestOutputHelper logger)
 	{
 	}
 
+	[Fact]
+	public async Task TestConnection()
+	{
+		TaskCompletionSource<Stream> serverStreamSource = new();
+		Stream? clientStream = null;
+		IIpcServer server = ServerFactory.Create(
+			stream =>
+			{
+				serverStreamSource.TrySetResult(stream);
+				return Task.CompletedTask;
+			},
+			new ServerFactory.ServerOptions
+			{
+				TraceSource = this.CreateTestTraceSource(nameof(this.TestConnection)),
+			});
+
+		try
+		{
+			clientStream = await ServerFactory.ConnectAsync(server.Name, default, this.TimeoutToken);
+			Task writeTask = clientStream.WriteAsync(new byte[] { 1, 2, 3 }, 0, 3, this.TimeoutToken);
+			byte[] buffer = new byte[3];
+			Stream serverStream = await serverStreamSource.Task.WithCancellation(this.TimeoutToken);
+			Task<int> bytesReadTask = serverStream.ReadAsync(buffer, 0, 3, this.TimeoutToken);
+			await writeTask.WithCancellation(this.TimeoutToken);
+			Assert.NotEqual(0, await bytesReadTask.WithCancellation(this.TimeoutToken));
+		}
+		finally
+		{
+			await server.DisposeAsync();
+		}
+	}
+
 	[Fact]
 	public async Task FactoryAllowsMultipleClients_ConcurrentCallback()
 	{