Refactor the windows driver to avoid the races

.editorconfig

@@ -1,7 +1,7 @@
 [*.cs]
 indent_style = tab
 tab_width = 8 
-csharp_new_line_before_open_brace = true
+csharp_new_line_before_open_brace = methods,local_functions
 csharp_new_line_before_else = false
 csharp_new_line_before_catch = false
 csharp_new_line_before_finally = false

Terminal.Gui/Core.cs

@@ -1591,20 +1591,21 @@ namespace Terminal.Gui {
 				return;
 
 			var p = Environment.OSVersion.Platform;
+			Mono.Terminal.IMainLoopDriver mainLoopDriver;
 
-			if (UseSystemConsole)
+			if (UseSystemConsole) {
+				mainLoopDriver = new Mono.Terminal.NetMainLoop ();
 				Driver = new NetDriver ();
-			//
-			// The driver currently has a race and does not integrate into mainloop, so input
-			// only works at random.   I need to change the code to do proper polling on mainloop
-			// and then delegate the reading of events to WindowsConsole
-			//
-			//else if (p == PlatformID.Win32NT || p == PlatformID.Win32S || p == PlatformID.Win32Windows)
-			//	Driver = new WindowsDriver();
-			else
+			} else if (p == PlatformID.Win32NT || p == PlatformID.Win32S || p == PlatformID.Win32Windows){
+				var windowsDriver = new WindowsDriver ();
+				mainLoopDriver = windowsDriver;
+				Driver = windowsDriver;
+			} else {
+				mainLoopDriver = new Mono.Terminal.UnixMainLoop ();
 				Driver = new CursesDriver ();
+			}
 			Driver.Init (TerminalResized);
-			MainLoop = new Mono.Terminal.MainLoop (Driver is CursesDriver, UseSystemConsole);
+			MainLoop = new Mono.Terminal.MainLoop (mainLoopDriver);
 			SynchronizationContext.SetSynchronizationContext (new MainLoopSyncContext (MainLoop));
 			Top = Toplevel.Create ();
 			Current = Top;

Terminal.Gui/Drivers/CursesDriver.cs

@@ -196,7 +196,7 @@ namespace Terminal.Gui {
 		{
 			Curses.timeout (-1);
 
-			mainLoop.AddWatch (0, Mono.Terminal.MainLoop.Condition.PollIn, x => {
+			(mainLoop.Driver as Mono.Terminal.UnixMainLoop).AddWatch (0, Mono.Terminal.UnixMainLoop.Condition.PollIn, x => {
 				ProcessInput (keyHandler, mouseHandler);
 				return true;
 			});

Terminal.Gui/Drivers/NetDriver.cs

@@ -316,7 +316,7 @@ namespace Terminal.Gui {
 
 		public override void PrepareToRun (MainLoop mainLoop, Action<KeyEvent> keyHandler, Action<MouseEvent> mouseHandler)
 		{
-			mainLoop.WindowsKeyPressed = delegate (ConsoleKeyInfo consoleKey) {
+			(mainLoop.Driver as NetMainLoop).WindowsKeyPressed = delegate (ConsoleKeyInfo consoleKey) {
 				var map = MapKey (consoleKey);
 				if (map == (Key)0xffffffff)
 					return;

Terminal.Gui/Drivers/WindowsDriver.cs

@@ -27,6 +27,7 @@
 //
 using System;
 using System.Runtime.InteropServices;
+using System.Threading;
 using System.Threading.Tasks;
 using Mono.Terminal;
 using NStack;
@@ -38,14 +39,19 @@ namespace Terminal.Gui {
 		public const int STD_INPUT_HANDLE = -10;
 		public const int STD_ERROR_HANDLE = -12;
 
-		IntPtr InputHandle, OutputHandle;
-
+		internal IntPtr InputHandle, OutputHandle;
 		IntPtr ScreenBuffer;
+		uint originalConsoleMode;
 
 		public WindowsConsole ()
 		{
 			InputHandle = GetStdHandle (STD_INPUT_HANDLE);
 			OutputHandle = GetStdHandle (STD_OUTPUT_HANDLE);
+			originalConsoleMode = ConsoleMode;
+			var newConsoleMode = originalConsoleMode;
+			newConsoleMode |= (uint)(ConsoleModes.EnableMouseInput | ConsoleModes.EnableExtendedFlags);
+			newConsoleMode &= ~(uint)ConsoleModes.EnableQuickEditMode;
+			ConsoleMode = newConsoleMode;
 		}
 
 		public CharInfo[] OriginalStdOutChars;
@@ -86,37 +92,9 @@ namespace Terminal.Gui {
 			return SetConsoleCursorPosition (ScreenBuffer, position);
 		}
 
-		public void PollEvents (Action<InputRecord> inputEventHandler)
-		{
-			if (OriginalConsoleMode != 0)
-				return;
-
-			OriginalConsoleMode = ConsoleMode;
-
-			ConsoleMode |= (uint)ConsoleModes.EnableMouseInput;
-			ConsoleMode &= ~(uint)ConsoleModes.EnableQuickEditMode;
-			ConsoleMode |= (uint)ConsoleModes.EnableExtendedFlags;
-
-			Task.Run (() =>
-			{
-				uint numberEventsRead = 0;
-				uint length = 1;
-				InputRecord[] records = new InputRecord[length];
-
-				while (ContinueListeningForConsoleEvents &&
-					ReadConsoleInput(InputHandle, records, length, out numberEventsRead) &&
-				       numberEventsRead > 0){
-					inputEventHandler (records[0]);
-				}
-			});
-		}
-
 		public void Cleanup ()
 		{
 			ContinueListeningForConsoleEvents = false;
-			ConsoleMode = OriginalConsoleMode;
-			OriginalConsoleMode = 0;
-
 			if (!SetConsoleActiveScreenBuffer (OutputHandle)){
 				var err = Marshal.GetLastWin32Error ();
 				Console.WriteLine("Error: {0}", err);
@@ -125,8 +103,6 @@ namespace Terminal.Gui {
 
 		private bool ContinueListeningForConsoleEvents = true;
 
-		private uint OriginalConsoleMode = 0;
-
 		public uint ConsoleMode {
 			get {
 				uint v;
@@ -401,31 +377,132 @@ namespace Terminal.Gui {
 		[DllImport("kernel32.dll", SetLastError = true)]
 		static extern bool SetConsoleActiveScreenBuffer(IntPtr Handle);
 
+		[DllImport ("kernel32.dll", SetLastError = true)]
+		static extern bool GetNumberOfConsoleInputEvents (IntPtr handle, out uint lpcNumberOfEvents);
+		public uint InputEventCount {
+			get {
+				uint v;
+				GetNumberOfConsoleInputEvents (InputHandle, out v);
+				return v;
+			}
+		}
 	}
 
-	internal class WindowsDriver : ConsoleDriver {
-
+	internal class WindowsDriver : ConsoleDriver, Mono.Terminal.IMainLoopDriver {
+		static bool sync;
+		AutoResetEvent eventReady = new AutoResetEvent (false);
+		AutoResetEvent waitForProbe = new AutoResetEvent (false);
+		MainLoop mainLoop;
 		Action TerminalResized;
-
-		WindowsConsole WinConsole;
-
 		WindowsConsole.CharInfo[] OutputBuffer;
-
 		int cols, rows;
+		WindowsConsole winConsole;
 
 		public override int Cols => cols;
-
 		public override int Rows => rows;
 
-		static bool sync;
-
 		public WindowsDriver ()
 		{
-			WinConsole = new WindowsConsole();
+			winConsole = new WindowsConsole();
+
 			cols = Console.WindowWidth;
 			rows = Console.WindowHeight - 1;
+
 			ResizeScreen ();
 			UpdateOffScreen ();
+
+			Task.Run ((Action)WindowsInputHandler);
+		}
+
+		// The records that we keep fetching
+		WindowsConsole.InputRecord [] result, records = new WindowsConsole.InputRecord [1];
+
+		void WindowsInputHandler () 
+		{
+			while (true) {
+				waitForProbe.WaitOne ();
+
+				uint numberEventsRead = 0;
+
+				WindowsConsole.ReadConsoleInput (winConsole.InputHandle, records, 1, out numberEventsRead);
+				if (numberEventsRead == 0)
+					result = null;
+				else
+					result = records;
+				
+				eventReady.Set ();
+			}
+		}
+
+		void IMainLoopDriver.Setup (MainLoop mainLoop)
+		{
+			this.mainLoop = mainLoop;
+		}
+
+		void IMainLoopDriver.Wakeup ()
+		{
+		}
+
+		bool IMainLoopDriver.EventsPending (bool wait)
+		{
+			long now = DateTime.UtcNow.Ticks;
+
+			int waitTimeout;
+			if (mainLoop.timeouts.Count > 0) {
+				waitTimeout = (int)((mainLoop.timeouts.Keys [0] - now) / TimeSpan.TicksPerMillisecond);
+				if (waitTimeout < 0)
+					return true;
+			} else
+				waitTimeout = -1;
+
+			if (!wait)
+				waitTimeout = 0;
+
+			result = null;
+			waitForProbe.Set ();
+			eventReady.WaitOne (waitTimeout);
+			return result != null;
+		}
+
+		Action<KeyEvent> keyHandler;
+		Action<MouseEvent> mouseHandler;
+
+		public override void PrepareToRun (MainLoop mainLoop, Action<KeyEvent> keyHandler, Action<MouseEvent> mouseHandler)
+		{
+			this.keyHandler = keyHandler;
+			this.mouseHandler = mouseHandler;
+		}
+		
+
+		void IMainLoopDriver.MainIteration ()
+		{
+			if (result == null)
+				return;
+
+			var inputEvent = result [0];
+			switch (inputEvent.EventType) {
+			case WindowsConsole.EventType.Key:
+				if (inputEvent.KeyEvent.bKeyDown == false)
+					return;
+				var map = MapKey (ToConsoleKeyInfo (inputEvent.KeyEvent));
+				if (map == (Key)0xffffffff)
+					return;
+				keyHandler (new KeyEvent (map));
+				break;
+
+			case WindowsConsole.EventType.Mouse:
+				mouseHandler (ToDriverMouse (inputEvent.MouseEvent));
+				break;
+
+			case WindowsConsole.EventType.WindowBufferSize:
+				cols = inputEvent.WindowBufferSizeEvent.size.X;
+				rows = inputEvent.WindowBufferSizeEvent.size.Y - 1;
+				ResizeScreen ();
+				UpdateOffScreen ();
+				TerminalResized ();
+				break;
+			}
+			result = null;
 		}
 
 		private WindowsConsole.ButtonState? LastMouseButtonPressed = null;
@@ -572,35 +649,6 @@ namespace Terminal.Gui {
 			return (Key)(0xffffffff);
 		}
 
-		public override void PrepareToRun (MainLoop mainLoop, Action<KeyEvent> keyHandler, Action<MouseEvent> mouseHandler)
-		{
-			WinConsole.PollEvents (inputEvent =>
-			{
-				switch(inputEvent.EventType){
-				case WindowsConsole.EventType.Key:
-					if (inputEvent.KeyEvent.bKeyDown == false)
-						return;
-					var map = MapKey (ToConsoleKeyInfo (inputEvent.KeyEvent));
-					if (map == (Key) 0xffffffff)
-						return;
-					keyHandler (new KeyEvent (map));
-					break;
-
-				case WindowsConsole.EventType.Mouse:
-					mouseHandler (ToDriverMouse (inputEvent.MouseEvent));
-					break;
-
-				case WindowsConsole.EventType.WindowBufferSize:
-					cols = inputEvent.WindowBufferSizeEvent.size.X;
-					rows = inputEvent.WindowBufferSizeEvent.size.Y - 1;
-					ResizeScreen ();
-					UpdateOffScreen ();
-					TerminalResized ();
-					break;
-				}
-			});
-		}
-		
 		public override void Init (Action terminalResized)
 		{
 			TerminalResized = terminalResized;
@@ -722,7 +770,7 @@ namespace Terminal.Gui {
 			};
 
 			UpdateCursor();
-			WinConsole.WriteToConsole (OutputBuffer, bufferCoords, window);
+			winConsole.WriteToConsole (OutputBuffer, bufferCoords, window);
 		}
 
 		public override void UpdateScreen ()
@@ -740,7 +788,7 @@ namespace Terminal.Gui {
 			};
 
 			UpdateCursor();
-			WinConsole.WriteToConsole (OutputBuffer, bufferCoords, window);
+			winConsole.WriteToConsole (OutputBuffer, bufferCoords, window);
 		}
 
 		public override void UpdateCursor()
@@ -749,11 +797,11 @@ namespace Terminal.Gui {
 				X = (short)ccol,
 				Y = (short)crow
 			};
-			WinConsole.SetCursorPosition(position);
+			winConsole.SetCursorPosition(position);
 		}
 		public override void End ()
 		{
-			WinConsole.Cleanup();
+			winConsole.Cleanup();
 		}
 
 		#region Unused
@@ -785,5 +833,8 @@ namespace Terminal.Gui {
 		{
 		}
 		#endregion
+
 	}
+
+	
 }

Terminal.Gui/MonoCurses/mainloop.cs

@@ -32,20 +32,75 @@ using System.Threading;
 
 namespace Mono.Terminal {
 
+	/// <summary>
+	/// Public interface to create your own platform specific main loop driver.
+	/// </summary>
 	public interface IMainLoopDriver {
+		/// <summary>
+		/// Initializes the main loop driver, gets the calling main loop for the initialization.
+		/// </summary>
+		/// <param name="mainLoop">Main loop.</param>
 		void Setup (MainLoop mainLoop);
+
+		/// <summary>
+		/// Wakes up the mainloop that might be waiting on input, must be thread safe.
+		/// </summary>
 		void Wakeup ();
+
+		/// <summary>
+		/// Must report whether there are any events pending, or even block waiting for events.
+		/// </summary>
+		/// <returns><c>true</c>, if there were pending events, <c>false</c> otherwise.</returns>
+		/// <param name="wait">If set to <c>true</c> wait until an event is available, otherwise return immediately.</param>
 		bool EventsPending (bool wait);
 		void MainIteration ();
 	}
 
-	internal class UnixMainLoop : IMainLoopDriver {
+	/// <summary>
+	/// Unix main loop, suitable for using on Posix systems
+	/// </summary>
+	/// <remarks>
+	/// In addition to the general functions of the mainloop, the Unix version
+	/// can watch file descriptors using the AddWatch methods.
+	/// </remarks>
+	public class UnixMainLoop : IMainLoopDriver {
 		[StructLayout (LayoutKind.Sequential)]
 		struct Pollfd {
 			public int fd;
 			public short events, revents;
 		}
 
+		/// <summary>
+		///   Condition on which to wake up from file descriptor activity.  These match the Linux/BSD poll definitions.
+		/// </summary>
+		[Flags]
+		public enum Condition : short {
+			/// <summary>
+			/// There is data to read
+			/// </summary>
+			PollIn = 1,
+			/// <summary>
+			/// Writing to the specified descriptor will not block
+			/// </summary>
+			PollOut = 4,
+			/// <summary>
+			/// There is urgent data to read
+			/// </summary>
+			PollPri = 2,
+			/// <summary>
+			///  Error condition on output
+			/// </summary>
+			PollErr = 8,
+			/// <summary>
+			/// Hang-up on output
+			/// </summary>
+			PollHup = 16,
+			/// <summary>
+			/// File descriptor is not open.
+			/// </summary>
+			PollNval = 32
+		}
+
 		class Watch {
 			public int File;
 			public Condition Condition;
@@ -70,6 +125,7 @@ namespace Mono.Terminal {
 		bool poll_dirty = true;
 		int [] wakeupPipes = new int [2];
 		static IntPtr ignore = Marshal.AllocHGlobal (1);
+		MainLoop mainLoop;
 
 		void IMainLoopDriver.Wakeup ()
 		{
@@ -77,8 +133,9 @@ namespace Mono.Terminal {
 		}
 
 		void IMainLoopDriver.Setup (MainLoop mainLoop) {
+			this.mainLoop = mainLoop;
 			pipe (wakeupPipes);
-			mainLoop.AddWatch (wakeupPipes [0], MainLoop.Condition.PollIn, ml => {
+			AddWatch (wakeupPipes [0], Condition.PollIn, ml => {
 				read (wakeupPipes [0], ignore, (IntPtr)1);
 				return true;
 			});			
@@ -109,7 +166,7 @@ namespace Mono.Terminal {
 		///  The return value is a token that represents this watch, you can
 		///  use this token to remove the watch by calling RemoveWatch.
 		/// </remarks>
-		public object AddWatch (int fileDescriptor, MainLoop.Condition condition, Func<MainLoop, bool> callback)
+		public object AddWatch (int fileDescriptor, Condition condition, Func<MainLoop, bool> callback)
 		{
 			if (callback == null)
 				throw new ArgumentNullException (nameof(callback));
@@ -140,8 +197,8 @@ namespace Mono.Terminal {
 			long now = DateTime.UtcNow.Ticks;
 
 			int pollTimeout, n;
-			if (timeouts.Count > 0) {
-				pollTimeout = (int)((timeouts.Keys [0] - now) / TimeSpan.TicksPerMillisecond);
+			if (mainLoop.timeouts.Count > 0) {
+				pollTimeout = (int)((mainLoop.timeouts.Keys [0] - now) / TimeSpan.TicksPerMillisecond);
 				if (pollTimeout < 0)
 					return true;
 
@@ -155,9 +212,9 @@ namespace Mono.Terminal {
 
 			n = poll (pollmap, (uint)pollmap.Length, pollTimeout);
 			int ic;
-			lock (idleHandlers)
-				ic = idleHandlers.Count;
-			return n > 0 || timeouts.Count > 0 && ((timeouts.Keys [0] - DateTime.UtcNow.Ticks) < 0) || ic > 0;			
+			lock (mainLoop.idleHandlers)
+				ic = mainLoop.idleHandlers.Count;
+			return n > 0 || mainLoop.timeouts.Count > 0 && ((mainLoop.timeouts.Keys [0] - DateTime.UtcNow.Ticks) < 0) || ic > 0;			
 		}
 
 		void IMainLoopDriver.MainIteration () 
@@ -171,22 +228,27 @@ namespace Mono.Terminal {
 
 					if (!descriptorWatchers.TryGetValue (p.fd, out watch))
 						continue;
-					if (!watch.Callback (this))
+					if (!watch.Callback (this.mainLoop))
 						descriptorWatchers.Remove (p.fd);
 				}
 			}			
 		}
 	}
 
-	internal class NetMainLoop : IMainLoopDriver {
+	/// <summary>
+	/// Mainloop intended to be used with the .NET System.Console API, and can
+	/// be used on Windows and Unix, it is cross platform but lacks things like
+	/// file descriptor monitoring.
+	/// </summary>
+	class NetMainLoop : IMainLoopDriver {
 		AutoResetEvent keyReady = new AutoResetEvent (false);
 		AutoResetEvent waitForProbe = new AutoResetEvent (false);
 		ConsoleKeyInfo? windowsKeyResult = null;
-		Action<ConsoleKeyInfo> keyCallback;
+		public Action<ConsoleKeyInfo> WindowsKeyPressed;
+		MainLoop mainLoop;
 
-		public NetMainLoop (Action<ConsoleKeyInfo> keyCallback) 
+		public NetMainLoop () 
 		{
-			this.keyCallback = keyCallback;	
 		}
 
 		void WindowsKeyReader ()
@@ -200,6 +262,7 @@ namespace Mono.Terminal {
 
 		void IMainLoopDriver.Setup (MainLoop mainLoop)
 		{
+			this.mainLoop = mainLoop;
 			Thread readThread = new Thread (WindowsKeyReader);
 			readThread.Start ();			
 		}
@@ -213,8 +276,8 @@ namespace Mono.Terminal {
 			long now = DateTime.UtcNow.Ticks;
 
 			int waitTimeout;
-			if (timeouts.Count > 0) {
-				waitTimeout = (int)((timeouts.Keys [0] - now) / TimeSpan.TicksPerMillisecond);
+			if (mainLoop.timeouts.Count > 0) {
+				waitTimeout = (int)((mainLoop.timeouts.Keys [0] - now) / TimeSpan.TicksPerMillisecond);
 				if (waitTimeout < 0)
 					return true;
 			} else
@@ -232,32 +295,13 @@ namespace Mono.Terminal {
 		void IMainLoopDriver.MainIteration ()
 		{
 			if (windowsKeyResult.HasValue) {
-				if (keyCallback != null)
-					keyCallback (windowsKeyResult.Value);
+				if (WindowsKeyPressed!= null)
+					WindowsKeyPressed (windowsKeyResult.Value);
 				windowsKeyResult = null;
 			}			
 		}
 	}
 
-	internal class WinConsoleLoop : IMainLoopDriver {
-		void IMainLoopDriver.Setup (MainLoop mainLoop)
-		{
-		}
-
-		void IMainLoopDriver.Wakeup () 
-		{
-		}
-
-		bool IMainLoopDriver.EventsPending (bool wait)
-		{
-			return false;
-		}
-
-		void IMainLoopDriver.MainIteration ()
-		{
-		}
-	}
-
 	/// <summary>
 	///   Simple main loop implementation that can be used to monitor
 	///   file descriptor, run timers and idle handlers.
@@ -266,54 +310,21 @@ namespace Mono.Terminal {
 	///   Monitoring of file descriptors is only available on Unix, there
 	///   does not seem to be a way of supporting this on Windows.
 	/// </remarks>
-	public class MainLoop {
-		bool useUnix = true;
-
-		/// <summary>
-		///   Condition on which to wake up from file descriptor activity.  These match the Linux/BSD poll definitions.
-		/// </summary>
-		[Flags]
-		public enum Condition : short {
-			/// <summary>
-			/// There is data to read
-			/// </summary>
-			PollIn = 1,
-			/// <summary>
-			/// Writing to the specified descriptor will not block
-			/// </summary>
-			PollOut = 4,
-			/// <summary>
-			/// There is urgent data to read
-			/// </summary>
-			PollPri = 2,
-			/// <summary>
-			///  Error condition on output
-			/// </summary>
-			PollErr = 8,
-			/// <summary>
-			/// Hang-up on output
-			/// </summary>
-			PollHup = 16,
-			/// <summary>
-			/// File descriptor is not open.
-			/// </summary>
-			PollNval = 32
-		}
-
-
-		class Timeout {
+	public class 	MainLoop {
+		internal class Timeout {
 			public TimeSpan Span;
 			public Func<MainLoop,bool> Callback;
 		}
 
-		SortedList <long, Timeout> timeouts = new SortedList<long,Timeout> ();
-		List<Func<bool>> idleHandlers = new List<Func<bool>> ();
+		internal SortedList <long, Timeout> timeouts = new SortedList<long,Timeout> ();
+		internal List<Func<bool>> idleHandlers = new List<Func<bool>> ();
 
 		IMainLoopDriver driver;
-		IMainLoopDriver Driver => driver;
+		public IMainLoopDriver Driver => driver;
 
 		/// <summary>
-		///  Default constructor
+		///  Creates a new Mainloop, to run it you must provide a driver, and choose
+		///  one of the implementations UnixMainLoop, NetMainLoop or WindowsMainLoop.
 		/// </summary>
 		public MainLoop (IMainLoopDriver driver)
 		{