Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: check TTY mode reset on exit #21027

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 30, 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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 30, 2018
@addaleax
Copy link
Member Author

Not sure stty is available/has standard output on all platforms, so:

CI: https://ci.nodejs.org/job/node-test-commit/18866/

@addaleax addaleax added the tty Issues and PRs related to the tty subsystem. label May 30, 2018
@addaleax
Copy link
Member Author

@nodejs/build for the OS X failure: https://ci.nodejs.org/job/node-test-commit-osx/18941/nodes=osx1010/console

@addaleax
Copy link
Member Author

src/node.cc Outdated
@@ -4286,6 +4286,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
WaitForInspectorDisconnect(&env);

env.set_can_call_into_js(false);
uv_tty_reset_mode();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I get it now. Doesn't #20592 also address this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Yes, it does. I’ll land that one and strip this PR down to tests.

@Trott
Copy link
Member

Trott commented May 31, 2018

@addaleax addaleax changed the title src: reset TTY mode before cleaning up resources test: check TTY mode reset on exit May 31, 2018
@addaleax addaleax added test Issues and PRs related to the tests. and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels May 31, 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: nodejs#21020
Refs: nodejs#20592
@addaleax
Copy link
Member Author

Landed the other PR and made this tests-only.

CI: https://ci.nodejs.org/job/node-test-commit/18894/

@addaleax
Copy link
Member Author

addaleax commented Jun 1, 2018

Landed in 7c8eec0

@addaleax addaleax closed this Jun 1, 2018
@addaleax addaleax deleted the fix-tty-reset branch June 1, 2018 08:53
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 <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2018
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 <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
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 <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants