ICommandExecutor provides three synchronous Execute overloads as default interface implementations, and all three block on .Result behind a suppression:
https://github.com/ktsu-dev/Essentials/blob/main/Essentials/ICommandExecutor.cs#L71-L73
public CommandResult Execute(string command, string? workingDirectory = null) =>
#pragma warning disable VSTHRD002 // Avoid problematic synchronous waits
ExecuteAsync(command, workingDirectory, CancellationToken.None).Result;
#pragma warning restore VSTHRD002The same shape repeats at ICommandExecutor.cs:84 and ICommandExecutor.cs:115.
Why this is worth fixing
It changes the exception a caller sees. Task<T>.Result wraps whatever the operation threw in an AggregateException. A caller writing the obvious try/catch around a synchronous Execute call catches nothing, because the exception they are looking for is now an inner exception. GetAwaiter().GetResult() rethrows the original and preserves the stack trace. This one is observable from outside the library and is the part I would treat as a defect rather than a smell.
It blocks a pool thread for the whole lifetime of a child process. Command execution is not a short wait. Under any concurrent load, callers on the thread pool can starve it.
It hard-codes CancellationToken.None. A synchronous caller has no way to bound the wait, so a hung child process hangs the caller permanently.
Relationship to #8
Separate defect class, and deliberately not folded into that issue. #8 is about async methods that aren't genuinely async, so they waste a thread doing work that could yield. This is the mirror image: a synchronous method implemented on top of an asynchronous one. Fixing #8 does not touch it, and it would still be here afterwards.
Sketch of a fix
The minimum, and a strict improvement on its own, is .GetAwaiter().GetResult() at all three sites, which fixes the exception wrapping. VSTHRD002 still applies, so the suppression stays, but it would then be suppressing thread-blocking alone rather than also hiding an exception bug.
The fuller fix is for ICommandExecutor to declare a genuinely synchronous primitive that Execute calls directly, the same way the compression providers were converted in #14: a provider that declares the sync primitive replaces the default, and no path bridges between the two worlds. NativeCommandExecutor already drives System.Diagnostics.Process, which has a synchronous WaitForExit (with the caveat recorded in ktsu-dev/Sdk#35 about WaitForExit() and redirected streams).
Worth deciding deliberately, since it is a question about which half of the API is the primitive.
ICommandExecutorprovides three synchronousExecuteoverloads as default interface implementations, and all three block on.Resultbehind a suppression:https://github.com/ktsu-dev/Essentials/blob/main/Essentials/ICommandExecutor.cs#L71-L73
The same shape repeats at
ICommandExecutor.cs:84andICommandExecutor.cs:115.Why this is worth fixing
It changes the exception a caller sees.
Task<T>.Resultwraps whatever the operation threw in anAggregateException. A caller writing the obvioustry/catcharound a synchronousExecutecall catches nothing, because the exception they are looking for is now an inner exception.GetAwaiter().GetResult()rethrows the original and preserves the stack trace. This one is observable from outside the library and is the part I would treat as a defect rather than a smell.It blocks a pool thread for the whole lifetime of a child process. Command execution is not a short wait. Under any concurrent load, callers on the thread pool can starve it.
It hard-codes
CancellationToken.None. A synchronous caller has no way to bound the wait, so a hung child process hangs the caller permanently.Relationship to #8
Separate defect class, and deliberately not folded into that issue. #8 is about async methods that aren't genuinely async, so they waste a thread doing work that could yield. This is the mirror image: a synchronous method implemented on top of an asynchronous one. Fixing #8 does not touch it, and it would still be here afterwards.
Sketch of a fix
The minimum, and a strict improvement on its own, is
.GetAwaiter().GetResult()at all three sites, which fixes the exception wrapping.VSTHRD002still applies, so the suppression stays, but it would then be suppressing thread-blocking alone rather than also hiding an exception bug.The fuller fix is for
ICommandExecutorto declare a genuinely synchronous primitive thatExecutecalls directly, the same way the compression providers were converted in #14: a provider that declares the sync primitive replaces the default, and no path bridges between the two worlds.NativeCommandExecutoralready drivesSystem.Diagnostics.Process, which has a synchronousWaitForExit(with the caveat recorded in ktsu-dev/Sdk#35 aboutWaitForExit()and redirected streams).Worth deciding deliberately, since it is a question about which half of the API is the primitive.