Most async methods in Essentials are Task.Run wrappers over synchronous work rather than genuine asynchronous I/O. There are 55 such call sites across 8 interfaces, all routed through a single helper:
internal static Task<T> RunAsync<T>(Func<T> action, CancellationToken cancellationToken)
=> cancellationToken.IsCancellationRequested
? Task.FromCanceled<T>(cancellationToken)
: Task.Run(action, cancellationToken);
Distribution of ProviderHelpers.RunAsync call sites:
| Interface |
Sites |
ICompressionProvider |
10 |
IEncodingProvider |
10 |
IEncryptionProvider |
10 |
IObfuscationProvider |
10 |
ICacheProvider |
6 |
ISerializationProvider |
5 |
IValidationProvider |
3 |
IHashProvider |
1 |
CLAUDE.md records this internally — "Note these are Task.Run wrappers over synchronous work, not genuine async I/O" — but the README tells consumers the opposite:
Comprehensive Async Support: Every operation has async variants with proper CancellationToken support
Why this matters
There are two distinct harms, and they pull in opposite directions, which is why one blanket fix will not do.
For stream-based operations, Task.Run does not deliver what the caller asked for. TryEncryptAsync(Stream, …) and TryCompressAsync(Stream, …) wrap a synchronous stream loop. The calling thread is released, but a thread pool thread is then blocked for the full duration of the I/O. On a request path moving large payloads, that is a pool thread held for the length of a full disk or network read — the thread was relocated, not freed. Under load this is how thread pool starvation starts.
This is exactly the complaint in #6, generalised. #6 fixes it for IHashProvider; the same defect remains in compression, encryption, and stream serialization.
For in-memory and CPU-bound operations, Task.Run is overhead the caller did not ask for. IEncodingProvider, IObfuscationProvider, IValidationProvider, and ICacheProvider operate on buffers already in memory. Wrapping them costs a thread pool dispatch, a Task allocation, and a context switch, to perform work that frequently takes microseconds. Base64-encoding a 200-byte span through EncodeAsync is slower and allocates more than the synchronous call it wraps.
It also takes a decision away from the consumer. The long-standing guidance for library authors is that a library should not call Task.Run on the caller's behalf — the caller is the one who knows whether offloading is worth it, and only the caller knows whether they are on a UI thread. Exposing an async method that is internally Task.Run advertises a capability the library does not have.
Where it bites
The practical consequence is that an async-looking API cannot be trusted without reading its implementation. A consumer profiling thread pool starvation has no way to tell from the surface which of these methods actually yield and which occupy a thread — and the README actively tells them the wrong thing.
Not all of it is affected
Worth stating plainly, because it narrows the work considerably. IPersistenceProvider and ICommandExecutor declare their async members abstractly and their implementations are genuinely asynchronous — FileSystemPersistenceProvider uses ReadAllTextAsync and WriteAllTextAsync, and NativeCommandExecutor uses WaitForExitAsync and ReadToEndAsync. Those two are fine and are not part of this issue.
Suggested direction
The two categories want different treatment.
Stream-based operations should become genuinely asynchronous. Compression, encryption, and stream serialization should use real ReadAsync/WriteAsync loops, the same way #6 proposes to fix stream hashing. The BCL primitives underneath all support it — Stream.CopyToAsync, CryptoStream, and the compression streams are all async-capable. This is additive work with no surface change.
In-memory operations need a decision. The relevant guidance here is Stephen Toub's Should I expose asynchronous wrappers for synchronous methods?, whose answer is no. Offloading is a policy decision that only the caller can make: the library cannot know whether the caller is on a UI thread, whether thread pool capacity is scarce, or whether the payload is 10 bytes or 10 GB. Calling Task.Run internally hard-codes an answer to a question the library cannot see.
Three options, in rough order of preference:
-
Expose only the synchronous method. A caller who wants the work backgrounded writes await Task.Run(() => provider.Encode(data)), which is one line at the call site and puts the decision where the information is. This is how consumers already treat comparable BCL APIs such as Encoding.UTF8.GetBytes. Breaking, so it requires a major version.
-
Keep an async-shaped surface but return an already-completed ValueTask<T> computed synchronously. Preserves uniformity across providers and composability with await, with no thread pool dispatch and no Task allocation. This is the idiomatic way to express "async-shaped but not actually asynchronous". Changing Task to ValueTask is a signature change, so this is also a major version.
-
Keep the current signatures and document them accurately. Cheapest and non-breaking, and strictly better than the present state, but leaves the overhead in place.
Note that a genuine backgrounding feature — fire-and-forget, queued work, progress reporting — is a separate abstraction (a channel or queue, IHostedService, or a dedicated provider category) rather than something to express through per-method async wrappers on every provider interface.
The README claim should be corrected regardless of which option is chosen, so that it states which operations are genuinely asynchronous and which are conveniences over synchronous work.
Related
Adjacent, possibly worth splitting out
ICommandExecutor.Execute blocks on an async call with .Result, behind a suppression:
#pragma warning disable VSTHRD002 // Avoid problematic synchronous waits
ExecuteAsync(command, workingDirectory, CancellationToken.None).Result;
#pragma warning restore VSTHRD002
This is sync-over-async, which can deadlock in any context with a synchronization context and turns exceptions into AggregateException. It is a different class of defect from the above and may deserve its own issue, but it belongs to the same theme of the async surface not behaving the way it appears to.
Most async methods in Essentials are
Task.Runwrappers over synchronous work rather than genuine asynchronous I/O. There are 55 such call sites across 8 interfaces, all routed through a single helper:Distribution of
ProviderHelpers.RunAsynccall sites:ICompressionProviderIEncodingProviderIEncryptionProviderIObfuscationProviderICacheProviderISerializationProviderIValidationProviderIHashProviderCLAUDE.mdrecords this internally — "Note these areTask.Runwrappers over synchronous work, not genuine async I/O" — but the README tells consumers the opposite:Why this matters
There are two distinct harms, and they pull in opposite directions, which is why one blanket fix will not do.
For stream-based operations,
Task.Rundoes not deliver what the caller asked for.TryEncryptAsync(Stream, …)andTryCompressAsync(Stream, …)wrap a synchronous stream loop. The calling thread is released, but a thread pool thread is then blocked for the full duration of the I/O. On a request path moving large payloads, that is a pool thread held for the length of a full disk or network read — the thread was relocated, not freed. Under load this is how thread pool starvation starts.This is exactly the complaint in #6, generalised. #6 fixes it for
IHashProvider; the same defect remains in compression, encryption, and stream serialization.For in-memory and CPU-bound operations,
Task.Runis overhead the caller did not ask for.IEncodingProvider,IObfuscationProvider,IValidationProvider, andICacheProvideroperate on buffers already in memory. Wrapping them costs a thread pool dispatch, aTaskallocation, and a context switch, to perform work that frequently takes microseconds. Base64-encoding a 200-byte span throughEncodeAsyncis slower and allocates more than the synchronous call it wraps.It also takes a decision away from the consumer. The long-standing guidance for library authors is that a library should not call
Task.Runon the caller's behalf — the caller is the one who knows whether offloading is worth it, and only the caller knows whether they are on a UI thread. Exposing an async method that is internallyTask.Runadvertises a capability the library does not have.Where it bites
The practical consequence is that an async-looking API cannot be trusted without reading its implementation. A consumer profiling thread pool starvation has no way to tell from the surface which of these methods actually yield and which occupy a thread — and the README actively tells them the wrong thing.
Not all of it is affected
Worth stating plainly, because it narrows the work considerably.
IPersistenceProviderandICommandExecutordeclare their async members abstractly and their implementations are genuinely asynchronous —FileSystemPersistenceProviderusesReadAllTextAsyncandWriteAllTextAsync, andNativeCommandExecutorusesWaitForExitAsyncandReadToEndAsync. Those two are fine and are not part of this issue.Suggested direction
The two categories want different treatment.
Stream-based operations should become genuinely asynchronous. Compression, encryption, and stream serialization should use real
ReadAsync/WriteAsyncloops, the same way #6 proposes to fix stream hashing. The BCL primitives underneath all support it —Stream.CopyToAsync,CryptoStream, and the compression streams are all async-capable. This is additive work with no surface change.In-memory operations need a decision. The relevant guidance here is Stephen Toub's Should I expose asynchronous wrappers for synchronous methods?, whose answer is no. Offloading is a policy decision that only the caller can make: the library cannot know whether the caller is on a UI thread, whether thread pool capacity is scarce, or whether the payload is 10 bytes or 10 GB. Calling
Task.Runinternally hard-codes an answer to a question the library cannot see.Three options, in rough order of preference:
Expose only the synchronous method. A caller who wants the work backgrounded writes
await Task.Run(() => provider.Encode(data)), which is one line at the call site and puts the decision where the information is. This is how consumers already treat comparable BCL APIs such asEncoding.UTF8.GetBytes. Breaking, so it requires a major version.Keep an async-shaped surface but return an already-completed
ValueTask<T>computed synchronously. Preserves uniformity across providers and composability withawait, with no thread pool dispatch and noTaskallocation. This is the idiomatic way to express "async-shaped but not actually asynchronous". ChangingTasktoValueTaskis a signature change, so this is also a major version.Keep the current signatures and document them accurately. Cheapest and non-breaking, and strictly better than the present state, but leaves the overhead in place.
Note that a genuine backgrounding feature — fire-and-forget, queued work, progress reporting — is a separate abstraction (a channel or queue,
IHostedService, or a dedicated provider category) rather than something to express through per-method async wrappers on every provider interface.The README claim should be corrected regardless of which option is chosen, so that it states which operations are genuinely asynchronous and which are conveniences over synchronous work.
Related
IHashProvider's stream path, and is the narrow case of the general problem described here.Adjacent, possibly worth splitting out
ICommandExecutor.Executeblocks on an async call with.Result, behind a suppression:This is sync-over-async, which can deadlock in any context with a synchronization context and turns exceptions into
AggregateException. It is a different class of defect from the above and may deserve its own issue, but it belongs to the same theme of the async surface not behaving the way it appears to.