fix(std.Progress): some follow-ups

[ghost]

Just some follow-ups to #12079

Some things mentioned here could still be fixed in the future in some way.

Fixes #13150

[andrewrk]

I've noticed, for the first time ever, my terminal getting into a weird locked-up state sometimes. I haven't managed to figure out how to reproduce it, but it happens after a compilation completes successfully. No keyboard inputs are reflected in the terminal, however I can type reset and then return, and the terminal gets fixed. I suspect it has to do with the recent std.Progress updates. I'm going to revert all the recent std.Progress changes in the meantime until this is diagnosed.

[andrewrk]

1952dd6

[ghost]

@andrewrk sure, seems like the best thing to do for now. Sorry for the trouble.
Well I think the only reason this could possibly happen is because of getTerminalCursorColumn:

zig/lib/std/Progress.zig

Line 236 in ab4e696

fn getTerminalCursorColumn(self: Progress, file: std.fs.File) !u16 {

This function temporarily puts your terminal into a state that would be regarded as "broken" in order to read the escape sequence's output.
It restores your terminal state with that defer os.tcsetattr.
Maybe you are hitting this?

zig/lib/std/Progress.zig

Line 247 in ab4e696

// Sorry for ruining your terminal

One thing you could try is to panic in there in order to see if that's what you're hitting.

I can really not imagine any other place where what you are describing could be happening.

Or could it be a similar issue to #13198? That issue is caused by the same function (and more specifically those tc attr functions that manipulate the terminal).

Or actually... maybe this is a race condition?

As a last resort we could remove getTerminalCursorColumn altogether and always use 0.

[ghost]

I do think this could be a race condition because if you look at those two tc attr calls you can see we do .NOW which means we immediately do this change, so there could be some issues if you run multiple std.Progress instances in a single terminal in parallel... I doubt that's it because it only happens once at startup, actually.

TLDR

So if you ever do find the time, could you try to put a panic in that defer and or just not call getTerminalCursorColumn and default to 0?

Also what exactly are you running on? On MIPS we have some incorrect constants (causing #13198). Maybe there's a similar issue for the thing you are running on.

[andrewrk]

I noticed that it happens 100% consistently when I run zig-bootstrap (master branch) and the build completes successfully. This is on x86_64-linux.

One other possible factor - I was doing some advanced troubleshooting and had inadvertently left a process (might have been zig, might have been cmake) suspended in the background (i.e. with Ctrl+Z) so perhaps that had something to do with it?

[ghost]

Sorry for the late reply. I've been trying to build with zig-bootstrap, and I'm still building...

Maybe you can compile faster: as the zig sources aren't updated yet, can you try to just modify the code manually and put a panic here
https://github.com/ziglang/zig-bootstrap/blob/326b8a409688d91f6392d06a8d26c2f9dfe01421/zig/lib/std/Progress.zig#L247
and see what happens and then after that replace
https://github.com/ziglang/zig-bootstrap/blob/326b8a409688d91f6392d06a8d26c2f9dfe01421/zig/lib/std/Progress.zig#L203
with

            const chars_already_printed = 0;

and see what that does? I expect the latter to "fix" the issue.

[andrewrk]

Sorry, I won't have time to troubleshoot this for a while. I'm typing up the release notes for 0.10.0 and also behind on a bunch of bug fixes that I really want to try to get in before the release is cut.

[marler8997]

It restores your terminal state with that defer os.tcsetattr.
Maybe you are hitting this?

Does Zig need to change the terminal mode?

If we do change the terminal mode, we should also be catching all terminating signals and vectored exceptions and restoring the mode there. Also note that there's no full-proof way to always restore the mode when we exit, for example. if someone kills the Zig compiler process with certain signals then Zig won't even get a chance to restore the terminal. This is just one of the drawbacks you have to accept when you decide you need to change the terminal mode.

If not changing the terminal mode means there are some more cases where we can't support progress update then maybe that's fine? I don't know what we're changing about the mode though, maybe it is necessary?

P.S. Now that I think about it, terminal attributes should have probably been implemented as "process-specific". Meaning, once the process who set them has exited, they get restored. Can't really change it now but maybe future generations will do better :)

[ghost]

@marler8997

Does Zig need to change the terminal mode?

This is why we were doing it in getTerminalCursorColumn:

zig/lib/std/Progress.zig

Line 241 in ab4e696

// (so that no enter press required for us to read the output of the escape sequence below)

It's so that if we have one std.Progress that's doing something like this:

doing something... 

then a second std.Progress instance can come in from a different process and allow this:

doing something... building llvm... 

etc. So here we have one std.Progress ("doing something") waiting for another ("building llvm") to finish, so they can stack up like that. I.e. we can have multiple std.Progress printing to the terminal.
getTerminalCursorColumn serves the purpose of knowing where we are. So if it's 0 it means no other std.Progress instance is before us (we'd be "doing something" in this example); if it's > 0 we know not too print past the terminal width (it makes the "hello world" -> "hello wo..." truncation still work properly because we truncate earlier because we checked where our cursor is).

So it's just so that we don't print past the terminal width even if there has already been output on the same terminal line before us.

As for the issue

I got the bootstrap finished eventually and was able to reproduce the issue of the terminal being broken after a successful compilation as well. Then I tried adding a @panic in a relevant place and tried to build again. Then I couldn't reproduce the issue anymore. I also tried deleting zig/zig-cache. Same problem. I tried it with a new terminal as well but on all other attempts the terminal did not break.
So it looks like I would have to throw away the entire cache and build with zig-bootstrap from scratch again in order to do a run with that @panic in place. It's a very strange issue.

• Note to self: if std.Progress: fix inaccurate line truncation and use optimal max terminal width #12079 is ever reopened, use better error codes (not error.Unexpected) (see std.debug: handle some possible errors and resolve low-hanging TODOs #12298 (comment)).