Fix deadlock when cancelling prompts #1439

caesay · 2024-01-30T14:58:58Z

Changes

Because ReadKeyAsync returns null if the cancellationToken is cancelled, this creates infinite loops in the AnsiConsoleExtensions.ReadLine method and the ListPrompt.Show method.

caesay · 2024-01-30T14:59:24Z

@microsoft-github-policy-service agree

caesay · 2024-01-31T07:37:42Z

Just as a note here, the check failed because the macOS runner failed to start, not because of the change.

patriksvensson · 2024-01-31T11:02:12Z

Merged! Thank you for your contribution. Much appreciated! 👍

The current prompt APIs suffer from several problems. First, the prompt APIs are fundamentally synchronous APIs. If we get down to the underlying implementation of all the prompt APIs, i.e. the `DefaultInput` class we can see some issues. Here's are the problematic implementation (before this commit fixes it): ```csharp public ConsoleKeyInfo? ReadKey(bool intercept) { return System.Console.ReadKey(intercept); } public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken) { while (true) { if (cancellationToken.IsCancellationRequested) { return null; } if (System.Console.KeyAvailable) { break; } await Task.Delay(5, cancellationToken).ConfigureAwait(false); } return ReadKey(intercept); } ``` * The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`. * The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged. * The asyncrhonous `ReadKeyAsync` method is not actually an synchronous method, it's waiting for a key to be pressed in a loop, waiting 5 milliseconds before checking again if it can break out of the loop. The proposed fix obsoletes the `ReadKeyAsync` method and add cancellation support to the synchronous `ReadKey` method through an optional `CancellationToken`. It also returns a non-nullable `ConsoleKeyInfo` making it clear that the only way to get out of this method is through cancellation. Then this change bubbles up to all the prompt APIs, also obsoleting the `IPrompt.ShowAsync` methods. This is a better alternative to [Async overloads for AnsiConsole Prompt/Ask/Confirm (spectreconsole#1194)](spectreconsole#1194) where the actual need is having a `CancellationToken` to perform some cleanup and not having async prompt APIs. Note that this is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface but that should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`. I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos).

The current prompt APIs suffer from several problems. First, the prompt APIs are fundamentally synchronous APIs. If we get down to the underlying implementation of all the prompt APIs, i.e. the `DefaultInput` class we can see some issues. Here's are the problematic implementation (before this commit fixes it): ```csharp public ConsoleKeyInfo? ReadKey(bool intercept) { return System.Console.ReadKey(intercept); } public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken) { while (true) { if (cancellationToken.IsCancellationRequested) { return null; } if (System.Console.KeyAvailable) { break; } await Task.Delay(5, cancellationToken).ConfigureAwait(false); } return ReadKey(intercept); } ``` * The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`. * The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged. * The asyncrhonous `ReadKeyAsync` method is not actually an synchronous method, it's waiting for a key to be pressed in a loop, waiting 5 milliseconds before checking again if it can break out of the loop. The proposed fix obsoletes the `ReadKeyAsync` method and add cancellation support to the synchronous `ReadKey` method through an optional `CancellationToken`. It also returns a non-nullable `ConsoleKeyInfo` making it clear that the only way to get out of this method is through cancellation. Then this change bubbles up to all the prompt APIs, also obsoleting the `IPrompt.ShowAsync` methods. This is a better alternative to [Async overloads for AnsiConsole Prompt/Ask/Confirm (spectreconsole#1194)](spectreconsole#1194) where the actual need is having a `CancellationToken` to perform some cleanup and not having async prompt APIs. Note that this is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface but that should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`. I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos). The addition of the `CancellationToken` to `IPrompt.Show(IAnsiConsole console, CancellationToken cancellationToken = default)` is also a breaking change but it should be mitigated since it has bee introduced with a default value.

The current prompt APIs suffer from a few problems. * The syncrhonous `ReadKey` method does not support cancellation. * The syncrhonous `ReadKey` method returns a nullable `ConsoleKeyInfo` struct but `System.Console.ReadKey` can never return a nullable `ConsoleKeyInfo`. * The asyncrhonous `ReadKeyAsync` method can return `null` only if cancellation has been requested. But this can never actually happen since [Fix deadlock when cancelling prompts (spectreconsole#1439)](spectreconsole#1439) was merged. Here's are the problematic implementation (before this commit fixes it): ```csharp public ConsoleKeyInfo? ReadKey(bool intercept) { return System.Console.ReadKey(intercept); } public async Task<ConsoleKeyInfo?> ReadKeyAsync(bool intercept, CancellationToken cancellationToken) { while (true) { if (cancellationToken.IsCancellationRequested) { return null; } if (System.Console.KeyAvailable) { break; } await Task.Delay(5, cancellationToken).ConfigureAwait(false); } return ReadKey(intercept); } ``` Note that adding a `CancellationToken` parameter and returning `ConsoleKeyInfo` instead of `ConsoleKeyInfo?` is a breaking change since it modifies the signatures of the public `IAnsiConsoleInput` interface. But this should not be an issue since it's impossible to use another implentation than `DefaultInput` when used through `AnsiConsole.Create(AnsiConsoleSettings settings)`. I have also searched for [implementers of IAnsiConsoleInput](https://grep.app/search?q=IAnsiConsoleInput) and I think this change won't break anything since nobody actually implemented `IAnsiConsoleInput`. Only exising implementations which have been udated are being used (at least across a half million public git repos). The addition of the `CancellationToken` to `IPrompt.Show(IAnsiConsole console, CancellationToken cancellationToken = default)` is also a breaking change but it should be mitigated since it has bee introduced with a default value.

Fix potential deadlock when cancelling prompts

254abb5

caesay mentioned this pull request Jan 30, 2024

Prompt Cancellation is broken #1076

Closed

patriksvensson approved these changes Jan 31, 2024

View reviewed changes

patriksvensson merged commit 71631b2 into spectreconsole:main Jan 31, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deadlock when cancelling prompts #1439

Fix deadlock when cancelling prompts #1439

caesay commented Jan 30, 2024

caesay commented Jan 30, 2024

caesay commented Jan 31, 2024

patriksvensson commented Jan 31, 2024

Fix deadlock when cancelling prompts #1439

Fix deadlock when cancelling prompts #1439

Conversation

caesay commented Jan 30, 2024

Changes

caesay commented Jan 30, 2024

caesay commented Jan 31, 2024

patriksvensson commented Jan 31, 2024