SEBWIN-220: Fixed race condition caused by the client stopping its communication host before the runtime had a chance to disconnect from it.

SafeExamBrowser.Client.UnitTests/Communication/ClientHostTests.cs

@@ -7,6 +7,7 @@
  */
 
 using System;
+using System.Threading;
 using Microsoft.VisualStudio.TestTools.UnitTesting;
 using Moq;
 using SafeExamBrowser.Client.Communication;
@@ -53,6 +54,7 @@ namespace SafeExamBrowser.Client.UnitTests.Communication
 
 			Assert.IsNotNull(response);
 			Assert.IsTrue(response.ConnectionEstablished);
+			Assert.IsTrue(sut.IsConnected);
 		}
 
 		[TestMethod]
@@ -80,8 +82,10 @@ namespace SafeExamBrowser.Client.UnitTests.Communication
 		[TestMethod]
 		public void MustCorrectlyDisconnect()
 		{
+			var eventFired = false;
 			var token = Guid.NewGuid();
 
+			sut.RuntimeDisconnected += () => eventFired = true;
 			sut.StartupToken = token;
 
 			var connectionResponse = sut.Connect(token);
@@ -89,6 +93,8 @@ namespace SafeExamBrowser.Client.UnitTests.Communication
 
 			Assert.IsNotNull(response);
 			Assert.IsTrue(response.ConnectionTerminated);
+			Assert.IsTrue(eventFired);
+			Assert.IsFalse(sut.IsConnected);
 		}
 
 		[TestMethod]
@@ -119,6 +125,59 @@ namespace SafeExamBrowser.Client.UnitTests.Communication
 			Assert.AreEqual(PROCESS_ID, (response as AuthenticationResponse)?.ProcessId);
 		}
 
+		[TestMethod]
+		public void MustHandlePasswordRequestCorrectly()
+		{
+			var passwordRequested = false;
+			var purpose = PasswordRequestPurpose.Administrator;
+			var requestId = Guid.NewGuid();
+			var resetEvent = new AutoResetEvent(false);
+
+			sut.PasswordRequested += (args) =>
+			{
+				passwordRequested = args.Purpose == purpose && args.RequestId == requestId;
+				resetEvent.Set();
+			};
+			sut.StartupToken = Guid.Empty;
+
+			var token = sut.Connect(Guid.Empty).CommunicationToken.Value;
+			var message = new PasswordRequestMessage(purpose, requestId) { CommunicationToken = token };
+			var response = sut.Send(message);
+
+			resetEvent.WaitOne();
+
+			Assert.IsTrue(passwordRequested);
+			Assert.IsNotNull(response);
+			Assert.IsInstanceOfType(response, typeof(SimpleResponse));
+			Assert.AreEqual(SimpleResponsePurport.Acknowledged, (response as SimpleResponse)?.Purport);
+		}
+
+		[TestMethod]
+		public void MustHandleReconfigurationDenialCorrectly()
+		{
+			var filePath = @"C:\Some\Random\Path\To\A\File.seb";
+			var reconfigurationDenied = false;
+			var resetEvent = new AutoResetEvent(false);
+
+			sut.ReconfigurationDenied += (args) =>
+			{
+				reconfigurationDenied = new Uri(args.ConfigurationPath).Equals(new Uri(filePath));
+				resetEvent.Set();
+			};
+			sut.StartupToken = Guid.Empty;
+
+			var token = sut.Connect(Guid.Empty).CommunicationToken.Value;
+			var message = new ReconfigurationDeniedMessage(filePath) { CommunicationToken = token };
+			var response = sut.Send(message);
+
+			resetEvent.WaitOne();
+
+			Assert.IsTrue(reconfigurationDenied);
+			Assert.IsNotNull(response);
+			Assert.IsInstanceOfType(response, typeof(SimpleResponse));
+			Assert.AreEqual(SimpleResponsePurport.Acknowledged, (response as SimpleResponse)?.Purport);
+		}
+
 		[TestMethod]
 		public void MustHandleShutdownRequestCorrectly()
 		{

SafeExamBrowser.Client.UnitTests/Operations/ClientHostDisconnectionOperationTests.cs

@@ -0,0 +1,109 @@
+/*
+ * Copyright (c) 2018 ETH Zürich, Educational Development and Technology (LET)
+ * 
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+using System.Diagnostics;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Moq;
+using SafeExamBrowser.Client.Operations;
+using SafeExamBrowser.Contracts.Communication.Hosts;
+using SafeExamBrowser.Contracts.Core.OperationModel;
+using SafeExamBrowser.Contracts.Logging;
+
+namespace SafeExamBrowser.Client.UnitTests.Operations
+{
+	[TestClass]
+	public class ClientHostDisconnectionOperationTests
+	{
+		private Mock<IClientHost> clientHost;
+		private Mock<ILogger> logger;
+
+		private ClientHostDisconnectionOperation sut;
+
+		[TestInitialize]
+		public void Initialize()
+		{
+			clientHost = new Mock<IClientHost>();
+			logger = new Mock<ILogger>();
+
+			sut = new ClientHostDisconnectionOperation(clientHost.Object, logger.Object, 0);
+		}
+
+		[TestMethod]
+		public void MustWaitForDisconnectionIfConnectionIsActive()
+		{
+			var stopWatch = new Stopwatch();
+			var timeout_ms = 1000;
+
+			sut = new ClientHostDisconnectionOperation(clientHost.Object, logger.Object, timeout_ms);
+
+			clientHost.SetupGet(h => h.IsConnected).Returns(true).Callback(() => clientHost.Raise(h => h.RuntimeDisconnected += null));
+
+			stopWatch.Start();
+			sut.Revert();
+			stopWatch.Stop();
+
+			clientHost.VerifyGet(h => h.IsConnected);
+			clientHost.VerifyNoOtherCalls();
+
+			Assert.IsFalse(stopWatch.IsRunning);
+			Assert.IsTrue(stopWatch.ElapsedMilliseconds < timeout_ms);
+		}
+
+		[TestMethod]
+		public void MustRespectTimeoutIfWaitingForDisconnection()
+		{
+			var stopWatch = new Stopwatch();
+			var timeout_ms = 50;
+
+			sut = new ClientHostDisconnectionOperation(clientHost.Object, logger.Object, timeout_ms);
+
+			clientHost.SetupGet(h => h.IsConnected).Returns(true);
+
+			stopWatch.Start();
+			sut.Revert();
+			stopWatch.Stop();
+
+			clientHost.VerifyGet(h => h.IsConnected);
+			clientHost.VerifyNoOtherCalls();
+
+			Assert.IsFalse(stopWatch.IsRunning);
+			Assert.IsTrue(stopWatch.ElapsedMilliseconds > timeout_ms);
+		}
+
+		[TestMethod]
+		public void MustDoNothingIfNoConnectionIsActive()
+		{
+			clientHost.SetupGet(h => h.IsConnected).Returns(false);
+
+			sut.Revert();
+
+			clientHost.VerifyGet(h => h.IsConnected);
+			clientHost.VerifyNoOtherCalls();
+		}
+
+		[TestMethod]
+		public void MustDoNothingOnPerform()
+		{
+			var result = sut.Perform();
+
+			clientHost.VerifyNoOtherCalls();
+
+			Assert.AreEqual(OperationResult.Success, result);
+		}
+
+		[TestMethod]
+		public void MustDoNothingOnRepeat()
+		{
+			var result = sut.Repeat();
+
+			clientHost.VerifyNoOtherCalls();
+
+			Assert.AreEqual(OperationResult.Success, result);
+		}
+	}
+}

SafeExamBrowser.Client.UnitTests/SafeExamBrowser.Client.UnitTests.csproj

@@ -78,6 +78,7 @@
   </ItemGroup>
   <ItemGroup>
     <Compile Include="Operations\BrowserOperationTests.cs" />
+    <Compile Include="Operations\ClientHostDisconnectionOperationTests.cs" />
     <Compile Include="Operations\ClipboardOperationTests.cs" />
     <Compile Include="Operations\DisplayMonitorOperationTests.cs" />
     <Compile Include="Operations\KeyboardInterceptorOperationTests.cs" />

SafeExamBrowser.Client/Communication/ClientHost.cs

@@ -7,11 +7,11 @@
  */
 
 using System;
+using SafeExamBrowser.Communication.Hosts;
 using SafeExamBrowser.Contracts.Communication.Data;
 using SafeExamBrowser.Contracts.Communication.Events;
 using SafeExamBrowser.Contracts.Communication.Hosts;
 using SafeExamBrowser.Contracts.Logging;
-using SafeExamBrowser.Communication.Hosts;
 
 namespace SafeExamBrowser.Client.Communication
 {
@@ -21,9 +21,11 @@ namespace SafeExamBrowser.Client.Communication
 		private int processId;
 
 		public Guid StartupToken { private get; set; }
+		public bool IsConnected { get; private set; }
 
 		public event CommunicationEventHandler<PasswordRequestEventArgs> PasswordRequested;
 		public event CommunicationEventHandler<ReconfigurationEventArgs> ReconfigurationDenied;
+		public event CommunicationEventHandler RuntimeDisconnected;
 		public event CommunicationEventHandler Shutdown;
 
 		public ClientHost(string address, IHostObjectFactory factory, ILogger logger, int processId) : base(address, factory, logger)
@@ -39,6 +41,7 @@ namespace SafeExamBrowser.Client.Communication
 			if (accepted)
 			{
 				allowConnection = false;
+				IsConnected = true;
 			}
 
 			return accepted;
@@ -46,7 +49,8 @@ namespace SafeExamBrowser.Client.Communication
 
 		protected override void OnDisconnect()
 		{
-			// Nothing to do here...
+			RuntimeDisconnected?.Invoke();
+			IsConnected = false;
 		}
 
 		protected override Response OnReceive(Message message)

SafeExamBrowser.Client/CompositionRoot.cs

@@ -97,7 +97,8 @@ namespace SafeExamBrowser.Client
 			operations.Enqueue(new RuntimeConnectionOperation(logger, runtimeProxy, startupToken));
 			operations.Enqueue(new ConfigurationOperation(configuration, logger, runtimeProxy));
 			operations.Enqueue(new DelegateOperation(UpdateAppConfig));
-			operations.Enqueue(new LazyInitializationOperation(BuildCommunicationHostOperation));
+			operations.Enqueue(new LazyInitializationOperation(BuildClientHostOperation));
+			operations.Enqueue(new LazyInitializationOperation(BuildClientHostDisconnectionOperation));
 			operations.Enqueue(new LazyInitializationOperation(BuildKeyboardInterceptorOperation));
 			operations.Enqueue(new LazyInitializationOperation(BuildWindowMonitorOperation));
 			operations.Enqueue(new LazyInitializationOperation(BuildProcessMonitorOperation));
@@ -177,12 +178,12 @@ namespace SafeExamBrowser.Client
 			return operation;
 		}
 
-		private IOperation BuildCommunicationHostOperation()
+		private IOperation BuildClientHostOperation()
 		{
 			var processId = Process.GetCurrentProcess().Id;
 			var factory = new HostObjectFactory();
 			var host = new ClientHost(configuration.AppConfig.ClientAddress, factory, new ModuleLogger(logger, nameof(ClientHost)), processId);
-			var operation = new CommunicationOperation(host, logger);
+			var operation = new CommunicationHostOperation(host, logger);
 
 			clientHost = host;
 			clientHost.StartupToken = startupToken;
@@ -190,6 +191,14 @@ namespace SafeExamBrowser.Client
 			return operation;
 		}
 
+		private IOperation BuildClientHostDisconnectionOperation()
+		{
+			var timeout_ms = 5000;
+			var operation = new ClientHostDisconnectionOperation(clientHost, logger, timeout_ms);
+
+			return operation;
+		}
+
 		private IOperation BuildKeyboardInterceptorOperation()
 		{
 			var keyboardInterceptor = new KeyboardInterceptor(configuration.Settings.Keyboard, new ModuleLogger(logger, nameof(KeyboardInterceptor)));

SafeExamBrowser.Client/Operations/ClientHostDisconnectionOperation.cs

@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2018 ETH Zürich, Educational Development and Technology (LET)
+ * 
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+using System.Threading;
+using SafeExamBrowser.Contracts.Communication.Events;
+using SafeExamBrowser.Contracts.Communication.Hosts;
+using SafeExamBrowser.Contracts.Core.OperationModel;
+using SafeExamBrowser.Contracts.Logging;
+using SafeExamBrowser.Contracts.UserInterface;
+
+namespace SafeExamBrowser.Client.Operations
+{
+	/// <summary>
+	/// During application shutdown, it could happen that the client stops its communication host before the runtime had the chance to
+	/// disconnect from it. This operation prevents the described race condition by waiting on the runtime to disconnect from the client.
+	/// </summary>
+	internal class ClientHostDisconnectionOperation : IOperation
+	{
+		private IClientHost clientHost;
+		private ILogger logger;
+		private int timeout_ms;
+
+		public IProgressIndicator ProgressIndicator { private get; set; }
+
+		public ClientHostDisconnectionOperation(IClientHost clientHost, ILogger logger, int timeout_ms)
+		{
+			this.clientHost = clientHost;
+			this.logger = logger;
+			this.timeout_ms = timeout_ms;
+		}
+
+		public OperationResult Perform()
+		{
+			return OperationResult.Success;
+		}
+
+		public OperationResult Repeat()
+		{
+			return OperationResult.Success;
+		}
+
+		public void Revert()
+		{
+			var disconnected = false;
+			var disconnectedEvent = new AutoResetEvent(false);
+			var disconnectedEventHandler = new CommunicationEventHandler(() => disconnectedEvent.Set());
+
+			clientHost.RuntimeDisconnected += disconnectedEventHandler;
+
+			if (clientHost.IsConnected)
+			{
+				logger.Info("Waiting for runtime to disconnect from client communication host...");
+				disconnected = disconnectedEvent.WaitOne(timeout_ms);
+
+				if (!disconnected)
+				{
+					logger.Error($"Runtime failed to disconnect within {timeout_ms / 1000} seconds!");
+				}
+			}
+			else
+			{
+				logger.Info("The runtime has already disconnected from the client communication host.");
+			}
+
+			clientHost.RuntimeDisconnected -= disconnectedEventHandler;
+		}
+	}
+}

SafeExamBrowser.Client/SafeExamBrowser.Client.csproj

@@ -72,6 +72,7 @@
   <ItemGroup>
     <Compile Include="App.cs" />
     <Compile Include="ClientController.cs" />
+    <Compile Include="Operations\ClientHostDisconnectionOperation.cs" />
     <Compile Include="Operations\ConfigurationOperation.cs" />
     <Compile Include="Operations\RuntimeConnectionOperation.cs" />
     <Compile Include="Communication\ClientHost.cs" />

SafeExamBrowser.Communication/Proxies/BaseProxy.cs

@@ -121,10 +121,11 @@ namespace SafeExamBrowser.Communication.Proxies
 			FailIfNotConnected();
 
 			message.CommunicationToken = communicationToken.Value;
+			Logger.Debug($"Sending message '{ToString(message)}'...");
 
 			var response = proxy.Send(message);
 
-			Logger.Debug($"Sent message '{ToString(message)}', got response '{ToString(response)}'.");
+			Logger.Debug($"Received response '{ToString(response)}' for message '{ToString(message)}'.");
 
 			return response;
 		}

SafeExamBrowser.Contracts/Communication/Hosts/IClientHost.cs

@@ -16,6 +16,11 @@ namespace SafeExamBrowser.Contracts.Communication.Hosts
 	/// </summary>
 	public interface IClientHost : ICommunicationHost
 	{
+		/// <summary>
+		/// Indicates whether the runtime has established a connection to this host.
+		/// </summary>
+		bool IsConnected { get; }
+
 		/// <summary>
 		/// The startup token used for initial authentication.
 		/// </summary>
@@ -31,6 +36,11 @@ namespace SafeExamBrowser.Contracts.Communication.Hosts
 		/// </summary>
 		event CommunicationEventHandler<ReconfigurationEventArgs> ReconfigurationDenied;
 
+		/// <summary>
+		/// Event fired when the runtime disconnected from the client.
+		/// </summary>
+		event CommunicationEventHandler RuntimeDisconnected;
+
 		/// <summary>
 		/// Event fired when the runtime commands the client to shutdown.
 		/// </summary>

SafeExamBrowser.Core.UnitTests/Operations/CommunicationHostOperationTests.cs

@@ -16,11 +16,11 @@ using SafeExamBrowser.Core.Operations;
 namespace SafeExamBrowser.Core.UnitTests.Operations
 {
 	[TestClass]
-	public class CommunicationOperationTests
+	public class CommunicationHostOperationTests
 	{
 		private Mock<ICommunicationHost> hostMock;
 		private Mock<ILogger> loggerMock;
-		private CommunicationOperation sut;
+		private CommunicationHostOperation sut;
 
 		[TestInitialize]
 		public void Initialize()
@@ -28,7 +28,7 @@ namespace SafeExamBrowser.Core.UnitTests.Operations
 			hostMock = new Mock<ICommunicationHost>();
 			loggerMock = new Mock<ILogger>();
 
-			sut = new CommunicationOperation(hostMock.Object, loggerMock.Object);
+			sut = new CommunicationHostOperation(hostMock.Object, loggerMock.Object);
 		}
 
 		[TestMethod]

SafeExamBrowser.Core.UnitTests/SafeExamBrowser.Core.UnitTests.csproj

@@ -78,7 +78,7 @@
     <Reference Include="System.Xml" />
   </ItemGroup>
   <ItemGroup>
-    <Compile Include="Operations\CommunicationOperationTests.cs" />
+    <Compile Include="Operations\CommunicationHostOperationTests.cs" />
     <Compile Include="Operations\LazyInitializationOperationTests.cs" />
     <Compile Include="Operations\I18nOperationTests.cs" />
     <Compile Include="Operations\DelegateOperationTests.cs" />

SafeExamBrowser.Core/Operations/CommunicationHostOperation.cs

@@ -18,14 +18,14 @@ namespace SafeExamBrowser.Core.Operations
 	/// An operation to handle the lifetime of an <see cref="ICommunicationHost"/>. The host is started during <see cref="Perform"/>,
 	/// stopped and restarted during <see cref="Repeat"/> (if not running) and stopped during <see cref="Revert"/>.
 	/// </summary>
-	public class CommunicationOperation : IOperation
+	public class CommunicationHostOperation : IOperation
 	{
 		private ICommunicationHost host;
 		private ILogger logger;
 
 		public IProgressIndicator ProgressIndicator { private get; set; }
 
-		public CommunicationOperation(ICommunicationHost host, ILogger logger)
+		public CommunicationHostOperation(ICommunicationHost host, ILogger logger)
 		{
 			this.host = host;
 			this.logger = logger;

SafeExamBrowser.Core/SafeExamBrowser.Core.csproj

@@ -54,7 +54,7 @@
     <Reference Include="System.Xml.Linq" />
   </ItemGroup>
   <ItemGroup>
-    <Compile Include="Operations\CommunicationOperation.cs" />
+    <Compile Include="Operations\CommunicationHostOperation.cs" />
     <Compile Include="Operations\LazyInitializationOperation.cs" />
     <Compile Include="Operations\I18nOperation.cs" />
     <Compile Include="Operations\DelegateOperation.cs" />

SafeExamBrowser.Runtime/CompositionRoot.cs

@@ -68,7 +68,7 @@ namespace SafeExamBrowser.Runtime
 			var sessionOperations = new Queue<IOperation>();
 
 			bootstrapOperations.Enqueue(new I18nOperation(logger, text, textResource));
-			bootstrapOperations.Enqueue(new CommunicationOperation(runtimeHost, logger));
+			bootstrapOperations.Enqueue(new CommunicationHostOperation(runtimeHost, logger));
 
 			sessionOperations.Enqueue(new ConfigurationOperation(appConfig, configuration, logger, messageBox, resourceLoader, runtimeHost, text, uiFactory, args));
 			sessionOperations.Enqueue(new SessionInitializationOperation(configuration, logger, runtimeHost));