Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 29d5555

Browse files
authored
Fix Console.CancelKeyPress deadlock (#28286)
If the CancelKeyPress handler tries to unregister itself, it can deadlock on Windows. This is because there's a lock that's held by Windows while invoking the callback, and the .NET logic for handling that callback is queueing the actual user callback to run on a ThreadPool thread and is then blocking waiting for that queued work to complete. So the calling thread that holds a lock is blocked waiting for another thread to run the user callback. If that user callback then tries to unregister the callback, it ends up trying to take the same lock that's already held by the other thread, and we deadlock. The simple fix is to stop queueing the work item and to just run it on the same thread. The code that does the queueing goes back over a decade, and was added during a phase where we were trying to ensure there was a ton of stack space available, but a) even then there were hundreds of kb of stack space available, which is plenty, and b) that effort was abandoned. (There is a small risk here for a corner case. The code currently has a timeout, where if the queued work item hasn't completed within 30 seconds, it unblocks the calling thread (but leaves the queued work item running indefinitely). With this change, we no longer have the work item and thus no longer have the timeout, but it is theoretically conceivable that someone somewhere was relying on the 30 second timeout.)
1 parent 2977973 commit 29d5555

File tree

1 file changed

+5
-56
lines changed

1 file changed

+5
-56
lines changed

src/System.Console/src/System/Console.cs

Lines changed: 5 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -688,68 +688,17 @@ public static void Write(String value)
688688
Out.Write(value);
689689
}
690690

691-
private sealed class ControlCDelegateData
692-
{
693-
private readonly ConsoleSpecialKey _controlKey;
694-
private readonly ConsoleCancelEventHandler _cancelCallbacks;
695-
696-
internal bool Cancel;
697-
internal bool DelegateStarted;
698-
699-
internal ControlCDelegateData(ConsoleSpecialKey controlKey, ConsoleCancelEventHandler cancelCallbacks)
700-
{
701-
_controlKey = controlKey;
702-
_cancelCallbacks = cancelCallbacks;
703-
}
704-
705-
// This is the worker delegate that is called on the Threadpool thread to fire the actual events. It sets the DelegateStarted flag so
706-
// the thread that queued the work to the threadpool knows it has started (since it does not want to block indefinitely on the task
707-
// to start).
708-
internal void HandleBreakEvent()
709-
{
710-
DelegateStarted = true;
711-
var args = new ConsoleCancelEventArgs(_controlKey);
712-
_cancelCallbacks(null, args);
713-
Cancel = args.Cancel;
714-
}
715-
}
716-
717691
internal static bool HandleBreakEvent(ConsoleSpecialKey controlKey)
718692
{
719-
// The thread that this gets called back on has a very small stack on some systems. There is
720-
// not enough space to handle a managed exception being caught and thrown. So, run a task
721-
// on the threadpool for the actual event callback.
722-
723-
// To avoid the race condition between remove handler and raising the event
724-
ConsoleCancelEventHandler cancelCallbacks = Console._cancelCallbacks;
725-
if (cancelCallbacks == null)
726-
{
727-
return false;
728-
}
729-
730-
var delegateData = new ControlCDelegateData(controlKey, cancelCallbacks);
731-
Task callBackTask = Task.Factory.StartNew(
732-
d => ((ControlCDelegateData)d).HandleBreakEvent(),
733-
delegateData,
734-
CancellationToken.None,
735-
TaskCreationOptions.DenyChildAttach,
736-
TaskScheduler.Default);
737-
738-
// Block until the delegate is done. We need to be robust in the face of the task not executing
739-
// but we also want to get control back immediately after it is done and we don't want to give the
740-
// handler a fixed time limit in case it needs to display UI. Wait on the task twice, once with a
741-
// timeout and a second time without if we are sure that the handler actually started.
742-
TimeSpan controlCWaitTime = new TimeSpan(0, 0, 30); // 30 seconds
743-
callBackTask.Wait(controlCWaitTime);
744-
745-
if (!delegateData.DelegateStarted)
693+
ConsoleCancelEventHandler handler = _cancelCallbacks;
694+
if (handler == null)
746695
{
747-
Debug.Assert(false, "The task to execute the handler did not start within 30 seconds.");
748696
return false;
749697
}
750698

751-
callBackTask.Wait();
752-
return delegateData.Cancel;
699+
var args = new ConsoleCancelEventArgs(controlKey);
700+
handler(null, args);
701+
return args.Cancel;
753702
}
754703
}
755704
}

0 commit comments

Comments
 (0)