Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Version 0.13.3 breaks LINQPad (and any non-Console application) #2237

Closed
albahari opened this issue Dec 29, 2022 · 4 comments · Fixed by #2238
Closed

Version 0.13.3 breaks LINQPad (and any non-Console application) #2237

albahari opened this issue Dec 29, 2022 · 4 comments · Fixed by #2238
Labels
Milestone

Comments

@albahari
Copy link
Contributor

albahari commented Dec 29, 2022

BenchmarkDotNet 0.13.3 throws an IOException whenever called from LINQPad. The cause is line 171 of BenchmarkRunnerClean:

var consoleTitle = RuntimeInformation.IsWindows() ? Console.Title : string.Empty;

The IOException is thrown when accessing Console.Title from a non-Console application in Windows. BenchmarkDotNet is effectively assuming that a Console window will always be available, which is not the case with a Windows Forms, WPF, Windows Service or daemon application. The line of code was introduced by #2140.

An easy way to fix this would be to read and restore the Console.Title property only when console output is not redirected. (This will not change the existing behavior because the same check is also performed before updating Console.Title in line 659 and line 674.) To implement this fix requires two changes. First, line 171 is modified as follows:

var consoleTitle = RuntimeInformation.IsWindows() && !Console.IsOutputRedirected ? Console.Title : string.Empty;

We also add the Console.IsOutputRedirected check at line 248:

if (RuntimeInformation.IsWindows() && !Console.IsOutputRedirected)
{
   Console.Title = consoleTitle;
}

This will get LINQPad working again (because LINQPad redirects console output). It would also provide a workaround for other non-Console apps that wish to call BenchmarkDotNet (by redirecting Console output).

P.S. If you prefer a cleaner solution, with no repetition of logic, how about introducing a class similar to the following:

using System;
using System.IO;
using BenchmarkDotNet.Portability;

namespace BenchmarkDotNet.Running
{
    internal class ConsoleTitler : IDisposable
    {
        public bool IsEnabled { get; private set; }

        private string oldConsoleTitle;

        public ConsoleTitler()
        {
            // Return without enabling if Console output is redirected, or if we're not on a platform that supports Console retitling.
            if (Console.IsOutputRedirected || !IsSupportedPlatform())
            {
                return;
            }

            try
            {
                oldConsoleTitle = RuntimeInformation.IsWindows() ? Console.Title : "";
                IsEnabled = true;
            }
            catch (IOException)
            {
                // No Console window is available - return without enabling.
                // This might be due to the hosting application being Windows Forms, WPF, Windows Service or a daemon.
            }
        }

        private bool IsSupportedPlatform() => RuntimeInformation.IsWindows() || RuntimeInformation.IsLinux() || RuntimeInformation.IsMacOS();

        public void UpdateTitle (string title)
        {
            if (IsEnabled)
            {
                Console.Title = title;
            }
        }

        public void Dispose()
        {
            if (IsEnabled && !string.IsNullOrEmpty (oldConsoleTitle))
            {
                Console.Title = oldConsoleTitle;
                IsEnabled = false;
            }
        }
    }
}

Instantiate it in line 171 of BenchmarkRunnerClean as follows:

using var consoleTitler = new ConsoleTitler();

Pass the instance as a parameter where required; no explicit disposal required.

@AndreyAkinshin
Copy link
Member

@albahari thank you for the bug report and for the proposed solution! Would you like to send a pull request?

cc @timcassell

@AndreyAkinshin AndreyAkinshin added this to the v0.13.4 milestone Dec 29, 2022
@timcassell
Copy link
Collaborator

timcassell commented Dec 29, 2022

I like the class approach, similar to TaskbarProgress.

@timcassell
Copy link
Collaborator

timcassell commented Dec 29, 2022

Also, keep in mind that the Console.Title is write-only on some OSes, so a single flag like your proposed class will not work there. #2140 (comment) I suggest adding the initial title as an argument to the constructor and try to write it, regardless if the read fails.

albahari added a commit to albahari/BenchmarkDotNet that referenced this issue Dec 29, 2022
albahari added a commit to albahari/BenchmarkDotNet that referenced this issue Dec 29, 2022
albahari added a commit to albahari/BenchmarkDotNet that referenced this issue Dec 29, 2022
@albahari
Copy link
Contributor Author

Done. I've kept with a single flag, but updated the logic so that it can restore Console.Title on platforms where the property is write-only.

albahari added a commit to albahari/BenchmarkDotNet that referenced this issue Jan 3, 2023
AndreyAkinshin pushed a commit that referenced this issue Jan 4, 2023
* Fix IOException when Console window unavailable (#2237)

* Refactored ConsoleTitler (#2237)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants