top: Fix refresh hangs by correctly configuring non-blocking TTY #26428
Conversation
github-actions
bot
added
the
👀 pr-needs-review
|
Do you know what caused the regression? |
|
I hadn't dug into the history. Top code seems good. I guess a deeper change of IO/TTY/fcntl/tcsetattr caused the regression. Explicitly setting VMIN and VTIME makes top more robust. |
|
I bisected this to 96675e6. VMIN is by default 1: serenity/Kernel/API/ttydefaults.h Line 24 in 6bcd011 (This also matches behavior with other unixes, e.g. OpenBSD: https://github.com/openbsd/src/blob/cfc59e797c2eed7e722f5fc0f94f22ef692897c1/sys/sys/ttydefaults.h#L65. glibc's version is very similar and actually has a BSD license header.) Though I'm confused why this affects reading from stdin. VMIN and VTIME seem to about reading: http://unixwiz.net/techtips/termios-vmin-vtime.html. And we also only set those values for stdout. So why does this fix the bug? |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! |
In normal use, both stdin and stdout are connected to the same file: the terminal. So I think that's how it's working. |
|
Yeah, but I think it would still be better to set it for stdin. |
Setting O_NONBLOCK via fcntl is not sufficient for TTYs in non-canonical mode. A subsequent tcsetattr call would override this, as the default VMIN and VTIME settings cause the driver to block. Explicitly set VMIN and VTIME to 0 to ensure a true non-blocking read. Also, apply these TTY settings to STDIN_FILENO instead of STDOUT_FILENO. This is semantically more correct as we are configuring input behavior. Fixes SerenityOS#26166.
|
I found swapping the fcntl and tcsetattr call order works without setting VMIN=0 (because fcntl sets it back to non-blocking), but I think explicitly setting VMIN=0 and VTIME=0 is safer and less brittle. I've also moved the config to STDIN_FILENO. |
4 participants
Setting O_NONBLOCK via fcntl is not sufficient for TTYs in non-canonical mode. A subsequent tcsetattr call would override this, as the default VMIN and VTIME settings cause the driver to block.
Explicitly set VMIN and VTIME to ensure a true non-blocking read.
Fixes #26166.