From c24e78d49211f6b47773685709268bbac9dc37bb Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 5 Jan 2021 16:17:47 +0100 Subject: [PATCH 1/3] bpo-41818: Close file descriptors in test_openpty When stdin is a TTY, the test added in commit c13d89955d9a2942c6355d6839d7096323244136 is expected to fail. However, when it failed, it did not close its file descriptors. This is flagged by the refleak tests (but only when stdin is a TTY, which doesn't seem to be the case on CI). --- Lib/test/test_pty.py | 89 ++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/Lib/test/test_pty.py b/Lib/test/test_pty.py index 190d8d787a2cc9..5a6562e96dc267 100644 --- a/Lib/test/test_pty.py +++ b/Lib/test/test_pty.py @@ -176,53 +176,54 @@ def test_openpty(self): # " An optional feature could not be imported " ... ? raise unittest.SkipTest("Pseudo-terminals (seemingly) not functional.") - self.assertTrue(os.isatty(slave_fd), "slave_fd is not a tty") - - if mode: - self.assertEqual(tty.tcgetattr(slave_fd), mode, - "openpty() failed to set slave termios") - if new_stdin_winsz: - self.assertEqual(_get_term_winsz(slave_fd), new_stdin_winsz, - "openpty() failed to set slave window size") - - # Solaris requires reading the fd before anything is returned. - # My guess is that since we open and close the slave fd - # in master_open(), we need to read the EOF. - # - # NOTE: the above comment is from an older version of the test; - # master_open() is not being used anymore. - - # Ensure the fd is non-blocking in case there's nothing to read. - blocking = os.get_blocking(master_fd) try: - os.set_blocking(master_fd, False) + self.assertTrue(os.isatty(slave_fd), "slave_fd is not a tty") + + if mode: + self.assertEqual(tty.tcgetattr(slave_fd), mode, + "openpty() failed to set slave termios") + if new_stdin_winsz: + self.assertEqual(_get_term_winsz(slave_fd), new_stdin_winsz, + "openpty() failed to set slave window size") + + # Solaris requires reading the fd before anything is returned. + # My guess is that since we open and close the slave fd + # in master_open(), we need to read the EOF. + # + # NOTE: the above comment is from an older version of the test; + # master_open() is not being used anymore. + + # Ensure the fd is non-blocking in case there's nothing to read. + blocking = os.get_blocking(master_fd) try: - s1 = os.read(master_fd, 1024) - self.assertEqual(b'', s1) - except OSError as e: - if e.errno != errno.EAGAIN: - raise + os.set_blocking(master_fd, False) + try: + s1 = os.read(master_fd, 1024) + self.assertEqual(b'', s1) + except OSError as e: + if e.errno != errno.EAGAIN: + raise + finally: + # Restore the original flags. + os.set_blocking(master_fd, blocking) + + debug("Writing to slave_fd") + os.write(slave_fd, TEST_STRING_1) + s1 = _readline(master_fd) + self.assertEqual(b'I wish to buy a fish license.\n', + normalize_output(s1)) + + debug("Writing chunked output") + os.write(slave_fd, TEST_STRING_2[:5]) + os.write(slave_fd, TEST_STRING_2[5:]) + s2 = _readline(master_fd) + self.assertEqual(b'For my pet fish, Eric.\n', normalize_output(s2)) finally: - # Restore the original flags. - os.set_blocking(master_fd, blocking) - - debug("Writing to slave_fd") - os.write(slave_fd, TEST_STRING_1) - s1 = _readline(master_fd) - self.assertEqual(b'I wish to buy a fish license.\n', - normalize_output(s1)) - - debug("Writing chunked output") - os.write(slave_fd, TEST_STRING_2[:5]) - os.write(slave_fd, TEST_STRING_2[5:]) - s2 = _readline(master_fd) - self.assertEqual(b'For my pet fish, Eric.\n', normalize_output(s2)) - - os.close(slave_fd) - # closing master_fd can raise a SIGHUP if the process is - # the session leader: we installed a SIGHUP signal handler - # to ignore this signal. - os.close(master_fd) + os.close(slave_fd) + # closing master_fd can raise a SIGHUP if the process is + # the session leader: we installed a SIGHUP signal handler + # to ignore this signal. + os.close(master_fd) def test_fork(self): debug("calling pty.fork()") From b7eb9e8d4360fadf04bc5fa9fd5a22c664e1c5e3 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 5 Jan 2021 16:59:38 +0100 Subject: [PATCH 2/3] Instead of the big indent, use self.addCleanup --- Lib/test/test_pty.py | 89 ++++++++++++++++++++++---------------------- 1 file changed, 44 insertions(+), 45 deletions(-) diff --git a/Lib/test/test_pty.py b/Lib/test/test_pty.py index 5a6562e96dc267..2d0720f0e27e73 100644 --- a/Lib/test/test_pty.py +++ b/Lib/test/test_pty.py @@ -176,54 +176,53 @@ def test_openpty(self): # " An optional feature could not be imported " ... ? raise unittest.SkipTest("Pseudo-terminals (seemingly) not functional.") + # closing master_fd can raise a SIGHUP if the process is + # the session leader: we installed a SIGHUP signal handler + # to ignore this signal. + self.addCleanup(os.close, master_fd) + self.addCleanup(os.close, slave_fd) + + self.assertTrue(os.isatty(slave_fd), "slave_fd is not a tty") + + if mode: + self.assertEqual(tty.tcgetattr(slave_fd), mode, + "openpty() failed to set slave termios") + if new_stdin_winsz: + self.assertEqual(_get_term_winsz(slave_fd), new_stdin_winsz, + "openpty() failed to set slave window size") + + # Solaris requires reading the fd before anything is returned. + # My guess is that since we open and close the slave fd + # in master_open(), we need to read the EOF. + # + # NOTE: the above comment is from an older version of the test; + # master_open() is not being used anymore. + + # Ensure the fd is non-blocking in case there's nothing to read. + blocking = os.get_blocking(master_fd) try: - self.assertTrue(os.isatty(slave_fd), "slave_fd is not a tty") - - if mode: - self.assertEqual(tty.tcgetattr(slave_fd), mode, - "openpty() failed to set slave termios") - if new_stdin_winsz: - self.assertEqual(_get_term_winsz(slave_fd), new_stdin_winsz, - "openpty() failed to set slave window size") - - # Solaris requires reading the fd before anything is returned. - # My guess is that since we open and close the slave fd - # in master_open(), we need to read the EOF. - # - # NOTE: the above comment is from an older version of the test; - # master_open() is not being used anymore. - - # Ensure the fd is non-blocking in case there's nothing to read. - blocking = os.get_blocking(master_fd) + os.set_blocking(master_fd, False) try: - os.set_blocking(master_fd, False) - try: - s1 = os.read(master_fd, 1024) - self.assertEqual(b'', s1) - except OSError as e: - if e.errno != errno.EAGAIN: - raise - finally: - # Restore the original flags. - os.set_blocking(master_fd, blocking) - - debug("Writing to slave_fd") - os.write(slave_fd, TEST_STRING_1) - s1 = _readline(master_fd) - self.assertEqual(b'I wish to buy a fish license.\n', - normalize_output(s1)) - - debug("Writing chunked output") - os.write(slave_fd, TEST_STRING_2[:5]) - os.write(slave_fd, TEST_STRING_2[5:]) - s2 = _readline(master_fd) - self.assertEqual(b'For my pet fish, Eric.\n', normalize_output(s2)) + s1 = os.read(master_fd, 1024) + self.assertEqual(b'', s1) + except OSError as e: + if e.errno != errno.EAGAIN: + raise finally: - os.close(slave_fd) - # closing master_fd can raise a SIGHUP if the process is - # the session leader: we installed a SIGHUP signal handler - # to ignore this signal. - os.close(master_fd) + # Restore the original flags. + os.set_blocking(master_fd, blocking) + + debug("Writing to slave_fd") + os.write(slave_fd, TEST_STRING_1) + s1 = _readline(master_fd) + self.assertEqual(b'I wish to buy a fish license.\n', + normalize_output(s1)) + + debug("Writing chunked output") + os.write(slave_fd, TEST_STRING_2[:5]) + os.write(slave_fd, TEST_STRING_2[5:]) + s2 = _readline(master_fd) + self.assertEqual(b'For my pet fish, Eric.\n', normalize_output(s2)) def test_fork(self): debug("calling pty.fork()") From 494fd7b9f4e52ef8f2dd54acbb8822ea04dc8c6d Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 12 Jan 2021 15:51:56 +0100 Subject: [PATCH 3/3] Clean up also in test_master_read and test_fork --- Lib/test/test_pty.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_pty.py b/Lib/test/test_pty.py index 2d0720f0e27e73..7585c42bf01338 100644 --- a/Lib/test/test_pty.py +++ b/Lib/test/test_pty.py @@ -91,7 +91,6 @@ def _set_term_winsz(fd, winsz): # Marginal testing of pty suite. Cannot do extensive 'do or fail' testing # because pty code is not too portable. -# XXX(nnorwitz): these tests leak fds when there is an error. class PtyTest(unittest.TestCase): def setUp(self): old_alarm = signal.signal(signal.SIGALRM, self.handle_sig) @@ -227,6 +226,7 @@ def test_openpty(self): def test_fork(self): debug("calling pty.fork()") pid, master_fd = pty.fork() + self.addCleanup(os.close, master_fd) if pid == pty.CHILD: # stdout should be connected to a tty. if not os.isatty(1): @@ -305,13 +305,14 @@ def test_fork(self): ##else: ## raise TestFailed("Read from master_fd did not raise exception") - os.close(master_fd) - def test_master_read(self): + # XXX(nnorwitz): this test leaks fds when there is an error. debug("Calling pty.openpty()") master_fd, slave_fd = pty.openpty() debug(f"Got master_fd '{master_fd}', slave_fd '{slave_fd}'") + self.addCleanup(os.close, master_fd) + debug("Closing slave_fd") os.close(slave_fd) @@ -321,7 +322,6 @@ def test_master_read(self): except OSError: # Linux data = b"" - os.close(master_fd) self.assertEqual(data, b"") class SmallPtyTests(unittest.TestCase):