termio: report color scheme synchronously

[tobiaskohlbau]

The reporting of color scheme was handled asynchronously by queuing a handler in the surface. This could lead to race conditions where the DSR is reported after subsequent VT sequences.

Fixes #5922

[tobiaskohlbau]

This is my first try at contributing back to Ghostty, please let me know if I missed something prior to implementation. Thanks to the awesome issue description it was relatively straightforward to resolve it. I've tested the changes on my machine and running the commands from the issue multiple times resulted in always the same order of the responses.

I've tried to attach an execution log but GitHub mangles with the escape codes in the code block.

[mitchellh]

It seems this doesn't fully faithfully convert this, since we're ignoring the mode flag in some cases where we previously didn't.

I don't have a problem with copying the conditional state.

[tobiaskohlbau]

Thanks for the review, just to clarify because I'm not that familiar. By mode flag do you refer to the force argument previously being present? I've removed it in places where it was always used with false and forcefully send the mode flag unconditionally in cases where it should by possible async changes (in the Surface.zig).

[mitchellh]

I'll re-review later, but it looks like both true and false values were converted to an unconditional report scheme.

[tobiaskohlbau]

That's right I've made the assumption that in case it's requested in termio it always needs to be responded to, cause if that code is reached the request was explicitly driven by request. But I could be mistaken here, I will think about it once again.

[tobiaskohlbau]

You were right, I've found two instances where I mistakenly changed the behavior. I've reintroduced the force flag and updated the callers.

If I transplant the mutex handling from Surface.zig into reportColorScheme living in stream_handler.zig I experienced deadlocks. I've looked at other places in stream_handler.zig and found it unlocks and relocks afterwards (the other way around). I've tested this and it works on my setup, but I'm not really confident this is the right thing to do.

Update:
I've searched up the call stack of the reportColorScheme function and came to the conclusion every caller already holds the mutex and therefore an additional lock is root cause of the deadlock. I've added and comment in the documentation about the callers responsibility.

[mitchellh]

Thanks for the changes, and sorry for the continued slow review. I've suggested one more thing. I'm working my way through PR backlogs now, got down 30 PRs the last week. 😄

[mitchellh]

Instead of duplicating this logic, we should add a new termio message so it's all sent from the same thread. Writes like this are already not synchronous ANYWAYS so that should be fine.

[tobiaskohlbau]

Now I was out for vacation, sorry for the delay on my side. I hope I understood your suggestion and implemented in the way you envisioned. PTAL and let me know if things needs to be updated.

[jcollie]

Renaming from verb-noun to noun-verb seems unnecessary, and a bit counter to existing practice.

[tobiaskohlbau]

I've named it according to

ghostty/src/termio/Thread.zig

Line 321 in 1c2db85

.size_report => |v| try io.sizeReport(data, v),

I'm happy to rename it to whatever is preferred.