LineRewriter: `_isSetCursorPositionSupported` cache depends on instance-specific `LineToClear` value
- Dominant language
- C++
- Stars
- 1.3k
- Forks
- 404
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 35
Description
Just came across some slightly confusing LineRewriter behavior when looking into PR feedback https://github.com/dotnet/diagnostics/pull/5771#discussion_r2961322933
I don't think it's high priority, but would be nice to clarify at some point.
## Summary
`LineRewriter.IsRewriteConsoleLineSupported` probes console capability by calling `SetCursorPosition(0, LineToClear)`, but caches the result in a `static` field. Since `LineToClear` is a mutable, instance-specific property, the cached result reflects whether a *specific row* is valid — not whether the console supports cursor repositioning in general.
## Current behavior
```csharp
// LineRewriter.cs
public int LineToClear { get; set; } // mutable, instance-specific
private static bool? _isSetCursorPositionSupported; // cached globally
public bool IsRewriteConsoleLineSupported
{
get => _isSetCursorPositionSupported ?? EnsureInitialized();
bool EnsureInitialized()
{
try
{
Console.SetCursorPosition(0, LineToClear); // probes with instance value
_isSetCursorPositionSupported = true; // caches globally
}
catch
{
_isSetCursorPositionSupported = false; // caches globally
}
}
}
```
If `LineToClear` is -1 (which happens when `CursorTop` is 0 in no-TTY environments), `SetCursorPosition` throws, and the static cache permanently records `false` — even if the console genuinely supports cursor repositioning and a subsequent `LineRewriter` instance has a valid `LineToClear`.
## Why this doesn't cause issues today
`dotnet-trace` runs one command per process invocation. `collect` and `collect-linux` can't run in the same process. Each invocation creates a single `LineRewriter` instance with one `LineToClear` value. The static cache is effectively instance-scoped by accident.
Additionally, PR #5771 added a `lineToClear < 0` guard in `collect-linux`'s `ProgressWriter` that prevents probing with an invalid value.
## Design questions
1. **Should `_isSetCursorPositionSupported` be instance-scoped?** The cached result depends on `LineToClear`, which is instance state. A static cache is only correct if all instances share the same `LineToClear` — which happens to be true today but isn't guaranteed by the API.
2. **Should the probe test console capability independently of `LineToClear`?** For example, probing with the current cursor position (`SetCursorPosition(left, top)`) would test "can this console reposition at all?" without conflating it with "is this specific row valid?" But this changes the semantics — `collect` relies on the probe to validate its specific `LineToClear` value.
3. **Should `LineToClear` be immutable?** It's set once after construction in both callers (`collect` and `collect-linux`), with one exception: `collect` does `LineToClear--` as a one-time adjustment when the cursor is at the buffer bottom. Making it a constructor parameter would make the API harder to misuse.
4. **Should `RewriteConsoleLine()` validate `LineToClear` internally?** Currently callers must check `IsRewriteConsoleLineSupported` before calling `RewriteConsoleLine()`. If they don't, and `LineToClear` is invalid, it crashes. A self-guarding `RewriteConsoleLine` would be safer.
## References
- PR #3678 — Original introduction of `IsRewriteConsoleLineSupported` (static cache, probes with `LineToClear`)
- PR #5771 — Fix for `collect-linux` crash when `CursorTop=0`, added `lineToClear < 0` guard in `ProgressWriter`
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.