From d536af305ee4ee700aa5b2d0d1725460ac04c8e3 Mon Sep 17 00:00:00 2001 From: Dariusz Działak Date: Oct 17 2017 23:37:09 +0000 Subject: Improve performance of close_all_open_files --- diff --git a/ChangeLog b/ChangeLog index 0aa5fdf..26fa560 100644 --- a/ChangeLog +++ b/ChangeLog @@ -26,6 +26,10 @@ Changes: * The test suite now relies on the test discovery feature in ‘unittest’. This feature is in Python version 2.7 and later. +* Improve performance of `daemon.close_all_open_files`. + + Closes: Pagure #10. + Version 2.1.2 ============= diff --git a/daemon/daemon.py b/daemon/daemon.py index 809f808..7d3f6b2 100644 --- a/daemon/daemon.py +++ b/daemon/daemon.py @@ -802,29 +802,6 @@ def is_detach_process_context_required(): return result - -def close_file_descriptor_if_open(fd): - """ Close a file descriptor if already open. - - :param fd: The file descriptor to close. - :return: ``None``. - - Close the file descriptor `fd`, suppressing an error in the - case the file was not open. - - """ - try: - os.close(fd) - except EnvironmentError as exc: - if exc.errno == errno.EBADF: - # File descriptor was not open. - pass - else: - error = DaemonOSEnvironmentError( - "Failed to close file descriptor {fd:d} ({exc})".format( - fd=fd, exc=exc)) - raise error - MAXFD = 2048 @@ -860,12 +837,24 @@ def close_all_open_files(exclude=None): close. """ - if exclude is None: - exclude = set() - maxfd = get_maximum_file_descriptors() - for fd in reversed(range(maxfd)): - if fd not in exclude: - close_file_descriptor_if_open(fd) + min_fd = 0 + max_fd = get_maximum_file_descriptors() + + if not exclude: + os.closerange(min_fd, max_fd) + return + + for ex_fd in sorted(exclude): + if ex_fd < min_fd: + continue + if ex_fd == min_fd: + min_fd = ex_fd + 1 + continue + os.closerange(min_fd, ex_fd) + min_fd = ex_fd + 1 + + if min_fd and min_fd < max_fd: + os.closerange(min_fd, max_fd) def redirect_stream(system_stream, target_stream): diff --git a/doc/CREDITS b/doc/CREDITS index 434457a..52922e1 100644 --- a/doc/CREDITS +++ b/doc/CREDITS @@ -33,6 +33,7 @@ Additional contributors People who have also contributed substantial improvements: * Malcolm Purvis +* Darek Działak .. diff --git a/test/test_daemon.py b/test/test_daemon.py index 6e6b888..8a3adb2 100644 --- a/test/test_daemon.py +++ b/test/test_daemon.py @@ -1230,59 +1230,6 @@ class prevent_core_dump_TestCase(scaffold.TestCase): self.assertEqual(test_error, exc.__cause__) -@mock.patch.object(os, "close") -class close_file_descriptor_if_open_TestCase(scaffold.TestCase): - """ Test cases for close_file_descriptor_if_open function. """ - - def setUp(self): - """ Set up test fixtures. """ - super(close_file_descriptor_if_open_TestCase, self).setUp() - - self.fake_fd = 274 - - def test_requests_file_descriptor_close(self, mock_func_os_close): - """ Should request close of file descriptor. """ - fd = self.fake_fd - daemon.daemon.close_file_descriptor_if_open(fd) - mock_func_os_close.assert_called_with(fd) - - def test_ignores_badfd_error_on_close(self, mock_func_os_close): - """ Should ignore OSError EBADF when closing. """ - fd = self.fake_fd - test_error = OSError(errno.EBADF, "Bad file descriptor") - def fake_os_close(fd): - raise test_error - mock_func_os_close.side_effect = fake_os_close - daemon.daemon.close_file_descriptor_if_open(fd) - mock_func_os_close.assert_called_with(fd) - - def test_raises_error_if_oserror_on_close(self, mock_func_os_close): - """ Should raise DaemonError if an OSError occurs when closing. """ - fd = self.fake_fd - test_error = OSError(object(), "Unexpected error") - def fake_os_close(fd): - raise test_error - mock_func_os_close.side_effect = fake_os_close - expected_error = daemon.daemon.DaemonOSEnvironmentError - exc = self.assertRaises( - expected_error, - daemon.daemon.close_file_descriptor_if_open, fd) - self.assertEqual(test_error, exc.__cause__) - - def test_raises_error_if_ioerror_on_close(self, mock_func_os_close): - """ Should raise DaemonError if an IOError occurs when closing. """ - fd = self.fake_fd - test_error = IOError(object(), "Unexpected error") - def fake_os_close(fd): - raise test_error - mock_func_os_close.side_effect = fake_os_close - expected_error = daemon.daemon.DaemonOSEnvironmentError - exc = self.assertRaises( - expected_error, - daemon.daemon.close_file_descriptor_if_open, fd) - self.assertEqual(test_error, exc.__cause__) - - class maxfd_TestCase(scaffold.TestCase): """ Test cases for module MAXFD constant. """ @@ -1374,35 +1321,30 @@ def fake_get_maximum_file_descriptors(): @mock.patch.object( daemon.daemon, "get_maximum_file_descriptors", new=fake_get_maximum_file_descriptors) -@mock.patch.object(daemon.daemon, "close_file_descriptor_if_open") +@mock.patch.object(os, "closerange") class close_all_open_files_TestCase(scaffold.TestCase): """ Test cases for close_all_open_files function. """ - def test_requests_all_open_files_to_close( - self, mock_func_close_file_descriptor_if_open): + def test_requests_all_open_files_to_close(self, mock_os_closerange): """ Should request close of all open files. """ - expected_file_descriptors = range(fake_default_maxfd) - expected_calls = [ - mock.call(fd) for fd in expected_file_descriptors] + expected_calls = [mock.call(0, fake_default_maxfd)] daemon.daemon.close_all_open_files() - mock_func_close_file_descriptor_if_open.assert_has_calls( - expected_calls, any_order=True) + mock_os_closerange.assert_has_calls(expected_calls, any_order=True) def test_requests_all_but_excluded_files_to_close( - self, mock_func_close_file_descriptor_if_open): + self, mock_os_closerange): """ Should request close of all open files but those excluded. """ test_exclude = set([3, 7]) args = dict( exclude=test_exclude, ) - expected_file_descriptors = set( - fd for fd in range(fake_default_maxfd) - if fd not in test_exclude) expected_calls = [ - mock.call(fd) for fd in expected_file_descriptors] + mock.call(0, 3), mock.call(4, 7), + ] + if fake_default_maxfd > 8: + expected_calls.append(mock.call(8, fake_default_maxfd)) daemon.daemon.close_all_open_files(**args) - mock_func_close_file_descriptor_if_open.assert_has_calls( - expected_calls, any_order=True) + mock_os_closerange.assert_has_calls(expected_calls, any_order=True) class detach_process_context_TestCase(scaffold.TestCase):