dotnet / dotnet/diagnostics

LineRewriter: `_isSetCursorPositionSupported` cache depends on instance-specific `LineToClear` value

Open
#5,780 0 comments 0 reactions 0 assignees View on GitHub
Priority:3
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.