test: check TTY mode reset on exit#21027
Conversation
|
Not sure |
|
@nodejs/build for the OS X failure: https://ci.nodejs.org/job/node-test-commit-osx/18941/nodes=osx1010/console |
src/node.cc
Outdated
There was a problem hiding this comment.
uv_tty_reset_mode() runs as an atexit handler and on signal exit so I don't understand why calling it here fixes the issue. Can you explain?
I read #21020 (comment) but libuv stores the struct termios on the first call to uv_tty_set_mode(), not uv_tty_init(), so I don't think that's it.
There was a problem hiding this comment.
@bnoordhuis Sorry, I probably didn’t describe it quite clearly enough.
strace node -e 'process.stdin.setRawMode(true)' on v10.0.0:
[...]
readlink("/proc/self/fd/0", "/dev/pts/2", 255) = 10
stat("/dev/pts/2", {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 2), ...}) = 0
openat(AT_FDCWD, "/dev/pts/2", O_RDWR|O_CLOEXEC) = 12
dup3(12, 0, O_CLOEXEC) = 0
ioctl(12, FIONBIO, [1]) = 0
ioctl(12, TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(12, TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(12, SNDCTL_TMR_STOP or TCSETSW, {B38400 opost -isig -icanon -echo ...}) = 0
ioctl(12, TCGETS, {B38400 opost -isig -icanon -echo ...}) = 0
[...]
ioctl(12, TCGETS, {B38400 opost -isig -icanon -echo ...}) = 0
ioctl(12, SNDCTL_TMR_START or TCSETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(12, TCGETS, {B38400 opost isig icanon echo ...}) = 0
[...]
strace node -e 'process.stdin.setRawMode(true)' on v10.2.0:
[...]
readlink("/proc/self/fd/0", "/dev/pts/2", 255) = 10
stat("/dev/pts/2", {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 2), ...}) = 0
openat(AT_FDCWD, "/dev/pts/2", O_RDWR|O_CLOEXEC) = 12
dup3(12, 0, O_CLOEXEC) = 0
ioctl(12, FIONBIO, [1]) = 0
ioctl(12, TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(12, TCGETS, {B38400 opost isig icanon echo ...}) = 0
ioctl(12, SNDCTL_TMR_STOP or TCSETSW, {B38400 opost -isig -icanon -echo ...}) = 0
ioctl(12, TCGETS, {B38400 opost -isig -icanon -echo ...}) = 0
[...]
/* this is the new part: Node.js cleans up resources like FDs now through uv_close() */
epoll_ctl(3, EPOLL_CTL_DEL, 12, 0x7ffeba4a1f90) = -1 ENOENT (No such file or directory)
close(12) = 0
[...]
ioctl(12, TCGETS, 0x7ffeba4a2a10) = -1 EBADF (Bad file descriptor)
ioctl(12, SNDCTL_TMR_START or TCSETS, {B38400 opost isig icanon echo ...}) = -1 EBADF (Bad file descriptor)
[...]
In the second output it’s visible that uv_close() for the stdin handle closes fd 12 (= libuv’s dup’ed copy of the original fd 0) – like it should – but it’s still stored with that value in libuv as orig_termios_fd and used for uv_tty_resst_mode().
Ideally, there would be a more well-defined API in libuv around this, without reliance on global state, but this is a fix for this issue without having to wait for that.
There was a problem hiding this comment.
Okay, I get it now. Doesn't #20592 also address this?
There was a problem hiding this comment.
@bnoordhuis Yes, it does. I’ll land that one and strip this PR down to tests.
|
Re-running Windows CI: https://ci.nodejs.org/job/node-test-commit-windows-fanned/18402/ |
Before PR 20592, closing all handles associated with the main event loop would also mean that `uv_tty_reset_mode()` can’t function properly because the corresponding FDs have already been closed. Add regression tests for this condition. Refs: nodejs#21020 Refs: nodejs#20592
|
Landed the other PR and made this tests-only. |
|
Landed in 7c8eec0 |
Before PR 20592, closing all handles associated with the main event loop would also mean that `uv_tty_reset_mode()` can’t function properly because the corresponding FDs have already been closed. Add regression tests for this condition. Refs: #21020 Refs: #20592 PR-URL: #21027 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Before PR 20592, closing all handles associated with the main event loop would also mean that `uv_tty_reset_mode()` can’t function properly because the corresponding FDs have already been closed. Add regression tests for this condition. Refs: #21020 Refs: #20592 PR-URL: #21027 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Before PR 20592, closing all handles associated with the main event loop would also mean that `uv_tty_reset_mode()` can’t function properly because the corresponding FDs have already been closed. Add regression tests for this condition. Refs: #21020 Refs: #20592 PR-URL: #21027 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Before PR 20592, closing all handles associated with the main event loop would also mean that `uv_tty_reset_mode()` can’t function properly because the corresponding FDs have already been closed. Add regression tests for this condition. Refs: #21020 Refs: #20592 PR-URL: #21027 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Before PR 20592, closing all handles associated with the main
event loop would also mean that
uv_tty_reset_mode()can’t function properly because the corresponding FDs have
already been closed.
Add regression tests for this condition.
Refs: #21020
Refs: #20592
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes