test: check TTY mode reset on exit#21027
Closed
addaleax wants to merge 1 commit intonodejs:masterfrom
Closed
Conversation
nodejs-github-bot
added
the
c++
Member
Author
|
Not sure |
Member
Author
|
@nodejs/build for the OS X failure: https://ci.nodejs.org/job/node-test-commit-osx/18941/nodes=osx1010/console |
Member
Author
apapirovski
approved these changes
May 30, 2018
bnoordhuis
reviewed
May 30, 2018
Member
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.
Member
Author
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.
Member
There was a problem hiding this comment.
Okay, I get it now. Doesn't #20592 also address this?
Member
Author
There was a problem hiding this comment.
@bnoordhuis Yes, it does. I’ll land that one and strip this PR down to tests.
Member
addaleax
changed the title
addaleax
added
test
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
Member
Author
|
Landed the other PR and made this tests-only. |
jasnell
approved these changes
May 31, 2018
Member
Author
|
Landed in 7c8eec0 |
addaleax
added a commit
that referenced
this pull request
Jun 1, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
addaleax
added a commit
that referenced
this pull request
Jun 1, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
2 tasks
addaleax
added a commit
that referenced
this pull request
Jun 29, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
Merged
rvagg
pushed a commit
that referenced
this pull request
Aug 16, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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