unix: restore tty attributes on handle close #4399
Conversation
Libuv stores the `struct termios` for use inside uv_tty_reset_mode(). Node.js uses said function to restore the tty to its original mode on SIGINT or SIGTERM, when there is no opportunity to shut down the process normally. Track uv_tty_t handle closing, otherwise we might be trying to use a stale termios. The current solution is not ideal because there can be multiple handles that refer to the same tty/pty and, for various reasons, we can't really determine when we close the last handle. The last handle may not even be inside the current process. Still, all things considered, it's probably (hopefully!) an improvement over the status quo. Refs: libuv#4398
|
Ha, this is exactly what I had in mind, but wanted to discuss first :-P Thanks for jumping at it! |
IMHO it is, the only potential problem I see is if someone expected the tty to remain in raw mode after closing the handle since they didn't explicitly reverse it. That's why I suggested the extra flag for the auto-reset, but then again, that sounds like a fringe case. |
If there's potential breakage for downstream users I'd prefer to be conservative so the extra flag sounds good to me. Won't block this though if you think the chance to do so is minimal. |
|
I think it's minimal indeed. |
| atomic_store(&termios_spinlock, 0); | ||
|
|
||
| done: | ||
| uv__stream_close((uv_stream_t*)handle); |
There was a problem hiding this comment.
style nit
| uv__stream_close((uv_stream_t*)handle); | |
| uv__stream_close((uv_stream_t*) handle); |
| * kind of process-global data structure, and that still won't work in a | ||
| * multi-process setup. | ||
| */ | ||
| uv__tcsetattr(fd, TCSANOW, &orig_termios); |
There was a problem hiding this comment.
This is sort of pre-existing, but why do we use TCSANOW for this? The documentation just says "use TCSADRAIN" if it may be writable. It seems this has some backwards-in-time behavior that may be best to avoid where it changes the way the tty interprets previously written data, but maybe also avoids a stall while the data gets pushed out the serial port.
| uv__tcsetattr(fd, TCSANOW, &orig_termios); | |
| uv__tcsetattr(fd, TCSADRAIN, &orig_termios); |
There was a problem hiding this comment.
So, the reason uv_tty_reset_mode() uses TCSANOW is that it can be called from signal handlers. TCSADRAIN and TCSAFLUSH have the potential to block indefinitely, which is undesirable, doubly so inside a signal handler.
(They're also interruptible, meaning you have to have an EINTR retry loop. Also undesirable inside a signal handler.)
That rationale doesn't really apply here so I could change it TCSADRAIN. Alternatively, I could insert a tcflush(TCIOFLUSH) before the tcsetattr call. I guess that's a good idea anyway inside uv_tty_reset_mode().
| * kind of process-global data structure, and that still won't work in a | ||
| * multi-process setup. | ||
| */ | ||
| uv__tcsetattr(fd, TCSANOW, &orig_termios); |
There was a problem hiding this comment.
Add a conditional here to fix the XXX comment above?
| uv__tcsetattr(fd, TCSANOW, &orig_termios); | |
| if (tty->mode != UV_TTY_MODE_NORMAL) | |
| uv__tcsetattr(fd, TCSANOW, &orig_termios); |
There was a problem hiding this comment.
That won't fix the issue. In an ideal world we only reset the tty when we're the final "owner" (so to speak) but we can't really know when that is.
There was a problem hiding this comment.
True, not a fix in the absolute sense (we would have to store the ptyN info that libuv computed earlier for that and ref count those to make this fully correct), but it would hopefully minimize surprises?
|
Where do we stand with this? |
Libuv stores the
struct termiosfor use inside uv_tty_reset_mode().Node.js uses said function to restore the tty to its original mode on SIGINT or SIGTERM, when there is no opportunity to shut down the process normally.
Track uv_tty_t handle closing, otherwise we might be trying to use a stale termios.
The current solution is not ideal because there can be multiple handles that refer to the same tty/pty and, for various reasons, we can't really determine when we close the last handle. The last handle may not even be inside the current process.
Still, all things considered, it's probably (hopefully!) an improvement over the status quo.
Refs: #4398