Fix IOException when Console window unavailable (#2237) #2238
Conversation
|
@albahari thanks for the PR! @timcassell do you have any comments on this? |
|
@timcassell does everything look fine from your side? |
|
I left comments that should be addressed before you merge. |
Which comments are you referring to? I added a constructor argument to the class as suggested, and updated the logic to handle platforms with write-only Console.Title. |
|
[Edit] Sorry, I didn't submit the change request so the comments weren't posted. |
| private static void UpdateTitle(int totalBenchmarkCount, int benchmarksToRunCount, ConsoleTitler consoleTitler) | ||
| { | ||
| if (!Console.IsOutputRedirected && (RuntimeInformation.IsWindows() || RuntimeInformation.IsLinux() || RuntimeInformation.IsMacOS())) | ||
| { | ||
| Console.Title = $"{benchmarksToRunCount}/{totalBenchmarkCount} Remaining"; | ||
| } | ||
| consoleTitler.UpdateTitle(() => $"{benchmarksToRunCount}/{totalBenchmarkCount} Remaining"); | ||
| } |
There was a problem hiding this comment.
UpdateTitle function is no longer needed, just call consoleTitler.UpdateTitle directly.
| var reports = new List<BenchmarkReport>(); | ||
| string title = GetTitle(new[] { benchmarkRunInfo }); | ||
| var consoleTitle = RuntimeInformation.IsWindows() ? Console.Title : string.Empty; | ||
| using var consoleTitler = new ConsoleTitler(""); |
There was a problem hiding this comment.
- using var consoleTitler = new ConsoleTitler("");
+ using var consoleTitler = new ConsoleTitler($"{benchmarksToRunCount}/{totalBenchmarkCount} Remaining");| logger.WriteLine(); | ||
|
|
||
| UpdateTitle(totalBenchmarkCount, benchmarksToRunCount); | ||
| UpdateTitle(totalBenchmarkCount, benchmarksToRunCount, consoleTitler); |
There was a problem hiding this comment.
Remove this update title line since it's passed in the constructor.
| /// Constructs a ConsoleTitler | ||
| /// </summary> | ||
| /// <param name="fallbackTitle">What to restore Console.Title to upon disposal (for platforms with write-only Console.Title)</param> | ||
| public ConsoleTitler(string fallbackTitle) |
There was a problem hiding this comment.
I was thinking the passed in string would be the initial title to write, not the fallback title on dispose. I don't think there is any value in having a custom fallback title on dispose.
| // We're unable to read Console.Title on a platform that supports it. This can happen when no console | ||
| // window is available due to the application being Windows Forms, WPF, Windows Service or a daemon. | ||
| // Because we won't be able to write Console.Title either, return without enabling the titler. | ||
| } |
There was a problem hiding this comment.
Write the passed in title here. This is where you need the flag to check in UpdateTitle if it is writable. The flag you have now only checks if it's readable.
There was a problem hiding this comment.
I think you need a separate try/catch for the write instead of a platform check. We'd want to catch IOException and PlatformNotSupportedException. Let other exceptions propagate.
try
{
Console.Title = title;
IsEnabled = true;
}
catch (IOException) { }
catch (PlatformNotSupportedException) { }| /// <summary> | ||
| /// Updates Console.Title if enabled, using a Func to avoid potential string-building cost. | ||
| /// </summary> | ||
| public void UpdateTitle(Func<string> title) | ||
| { | ||
| if (IsEnabled) | ||
| { | ||
| Console.Title = title(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think this is necessary. The cost of string building in this case is minimal, and this is just adding the cost of a delegate and closures.
| try | ||
| { | ||
| oldConsoleTitle = PlatformSupportsTitleRead() ? Console.Title : fallbackTitle; | ||
| IsEnabled = true; | ||
| } | ||
| catch (IOException) |
There was a problem hiding this comment.
I think this can be simplified to
try
{
oldConsoleTitle = Console.Title
}
catch
{
oldConsoleTitle = string.Empty;
}That should make it more future-proof.
There was a problem hiding this comment.
This will require a #pragma to suppress CA1416 - you OK with that?
Console.Title's get accessor is defined with [SupportedOSPlatform("windows")]
There was a problem hiding this comment.
Well, I suppose it makes sense to have the platform check for the get since that's currently the only supported platform. It can be updated in the future if that ever changes. But no platform check for the set. I still think the simpler try/catch makes sense, though, even with the platform check.
timcassell
left a comment
timcassell
left a comment
There was a problem hiding this comment.
LGTM, thanks for implementing the feedback.
| // Return without enabling if Console output is redirected. | ||
| if (Console.IsOutputRedirected) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'm not 100% sure if we want to disable this if output is redirected. I can imagine a scenario where the output is piped to a file, but it would still be useful to see the progress in the title. But not a big deal to me either way.
|
@albahari @timcassell thanks for your help! |
|
@albahari could you please verify that the latest nightly version ( You can get the nupkg from the appveyor build artifacts or via the nightly feed: <packageSources>
<add key="bdn-nightly" value="https://ci.appveyor.com/nuget/benchmarkdotnet" />
</packageSources> |
|
Works a treat:
|
|
@albahari v0.13.4 has been released. Thanks again for your assistance! |
3 participants
Fixes #2237