terminal-unix: fix infinite loop when read returns EIO #11805
Conversation
nightfriendly
changed the title
osdep/terminal-unix.c
Outdated
| @@ -420,7 +420,7 @@ static void *terminal_thread(void *ptr) | |||
| break; | |||
| if (fds[1].revents) { | |||
There was a problem hiding this comment.
IMO the proper fix would be to fix this check. It should be if (fds[1].revents & POLLIN) and there should be another check if (fds[1].revents & POLLHUP) break; after this block.
This commit fixes mpv-player#11795. If parent terminal is closed while mpv is running, read(2) starts returning EIO, resulting in an infinite loop using 100% of the CPU. This commit makes sure we don't loop needlessly if read(2) returns EIO.
|
Thank you very much for your review. I updated the patch accordingly, but please note I am not very familiar with |
| @@ -427,6 +427,9 @@ static void *terminal_thread(void *ptr) | |||
| process_input(input_ctx, false); | |||
| } | |||
| } | |||
| if (fds[1].revents & POLLHUP) { | |||
There was a problem hiding this comment.
mpv doesn't use braces for single line ifs.
There was a problem hiding this comment.
Also, I'd move this check after the other if (r == 0) process_input(...) line. Just in case there was still some input left in the buffer to process, since AFAIK POLLIN and POLLHUP can be true in parallel. E.g when there's still data left in the pipe, but the fd has been closed from the other end.
|
Oh and the commit message needs to be changed too. Since this no longer checks for |
There was a problem hiding this comment.
Had some time to test this. Some observations:
- I cannot reproduce the issue on linux
- Although it looked like
pollwas being used, in reality, mpv is using this weirdpolldevshim (which does some funny stuff, probably unrelated to this issue) - The
polldevshim, never setsPOLLHUPso this fix probably doesn't work.
Okay, seems like the Does anyone have any issues with this? Out of all the int retval = read(tty_in, &buf.b[buf.len], BUF_LEN - buf.len);
- if (!retval || (retval == -1 && (errno == EBADF || errno == EINVAL)))
+ if (!retval || (retval == -1 && errno != EINTR))
break; // EOF/closedP.S: maybe add |
the reason for checking `EBADF|EINVAL` specifically is unknown. but it's clearly not working as intended since it can cause issues like mpv-player#11795. instead of checking for "bad errors" check for known "good errors" where we might not want to break out. this includes: * EINTR: which can happen if read() is interrupted by some signal. * EAGAIN: which happens if a non-blocking fd would block. `tty_in` is _not_ non-blocking, so EAGAIN should never occur here. but it's added just in case that changes in the future. Fixes: mpv-player#11795 Closes: mpv-player#11805
|
Thank you very much @N-R-K. I am sorry I was traveling and could not get back to this earlier. |
This commit fixes #11795.
If parent terminal is closed while mpv is running, read(2) starts returning EIO, resulting in an infinite loop using 100% of the CPU.
This commit makes sure we don't loop needlessly if read(2) returns EIO.
Read this before you submit this pull request:
https://github.com/mpv-player/mpv/blob/master/DOCS/contribute.md
Reading this link and following the rules will get your pull request reviewed
and merged faster. Nobody wants lazy pull requests.