From c41dee914cfe8b9c5b1fab39046d75831fa96408 Mon Sep 17 00:00:00 2001 From: Eric Arellano Date: Fri, 28 Jun 2019 05:45:34 -0700 Subject: [PATCH] Remove Python 2 select.select() code (#7964) In #7882, we started using the much more efficient selectors module with Python 3 instead of select.select(). We may now remove the Python 2 code that was still using the original select.select(). --- src/python/pants/java/nailgun_executor.py | 55 ++++++------------- src/python/pants/util/BUILD | 3 - src/python/pants/util/socket.py | 42 ++------------ .../pants_test/java/test_nailgun_executor.py | 2 +- tests/python/pants_test/util/BUILD | 1 - tests/python/pants_test/util/test_socket.py | 41 +++++--------- 6 files changed, 40 insertions(+), 104 deletions(-) diff --git a/src/python/pants/java/nailgun_executor.py b/src/python/pants/java/nailgun_executor.py index a050b80de92..c921beff512 100644 --- a/src/python/pants/java/nailgun_executor.py +++ b/src/python/pants/java/nailgun_executor.py @@ -5,6 +5,7 @@ import logging import os import re +import selectors import threading import time from contextlib import closing @@ -20,12 +21,6 @@ from pants.util.memo import memoized_classproperty -if PY3: - import selectors -else: - import select - - logger = logging.getLogger(__name__) @@ -220,37 +215,23 @@ def possibly_raise_timeout(remaining_time): stderr=stderr, ) - if PY3: - # NB: We use PollSelector, rather than the more efficient DefaultSelector, because - # DefaultSelector results in using the epoll() syscall on Linux, which does not work with - # regular text files like ng_stdout. See https://stackoverflow.com/a/8645770. - with selectors.PollSelector() as selector, \ - safe_open(self._ng_stdout, 'r') as ng_stdout: - selector.register(ng_stdout, selectors.EVENT_READ) - while 1: - remaining_time = calculate_remaining_time() - possibly_raise_timeout(remaining_time) - events = selector.select(timeout=-1 * remaining_time) - if events: - line = ng_stdout.readline() # TODO: address deadlock risk here. - try: - return self._NG_PORT_REGEX.match(line).group(1) - except AttributeError: - pass - accumulated_stdout += line - else: - with safe_open(self._ng_stdout, 'r') as ng_stdout: - while 1: - remaining_time = calculate_remaining_time() - possibly_raise_timeout(remaining_time) - readable, _, _ = select.select([ng_stdout], [], [], (-1 * remaining_time)) - if readable: - line = ng_stdout.readline() # TODO: address deadlock risk here. - try: - return self._NG_PORT_REGEX.match(line).group(1) - except AttributeError: - pass - accumulated_stdout += line + # NB: We use PollSelector, rather than the more efficient DefaultSelector, because + # DefaultSelector results in using the epoll() syscall on Linux, which does not work with + # regular text files like ng_stdout. See https://stackoverflow.com/a/8645770. + with selectors.PollSelector() as selector, \ + safe_open(self._ng_stdout, 'r') as ng_stdout: + selector.register(ng_stdout, selectors.EVENT_READ) + while 1: + remaining_time = calculate_remaining_time() + possibly_raise_timeout(remaining_time) + events = selector.select(timeout=-1 * remaining_time) + if events: + line = ng_stdout.readline() # TODO: address deadlock risk here. + try: + return self._NG_PORT_REGEX.match(line).group(1) + except AttributeError: + pass + accumulated_stdout += line def _create_ngclient(self, port, stdout, stderr, stdin): return NailgunClient(port=port, ins=stdin, out=stdout, err=stderr) diff --git a/src/python/pants/util/BUILD b/src/python/pants/util/BUILD index 793ed197b84..533bda14b76 100644 --- a/src/python/pants/util/BUILD +++ b/src/python/pants/util/BUILD @@ -157,9 +157,6 @@ python_binary( python_library( name = 'socket', sources = ['socket.py'], - dependencies = [ - '3rdparty/python:future', - ] ) python_library( diff --git a/src/python/pants/util/socket.py b/src/python/pants/util/socket.py index be46a27bf90..8c6fbdc30b6 100644 --- a/src/python/pants/util/socket.py +++ b/src/python/pants/util/socket.py @@ -1,18 +1,10 @@ # Copyright 2015 Pants project contributors (see CONTRIBUTORS.md). # Licensed under the Apache License, Version 2.0 (see LICENSE). -import errno import io +import selectors import socket -from future.utils import PY3 - - -if PY3: - import selectors -else: - import select - def teardown_socket(s): """Shuts down and closes a socket.""" @@ -24,38 +16,16 @@ def teardown_socket(s): s.close() -# TODO(6071): Remove this once we drop Python 2, because 1) we no longer want to use select.select() -# in favor of https://docs.python.org/3/library/selectors.html, which uses more efficient and robust -# algorithms at a better level of abstraction, and because 2) PEP 474 fixed the issue with SIGINT -# https://www.python.org/dev/peps/pep-0475/. -def safe_select(*args, **kwargs): - # N.B. This while loop is purely to facilitate SA_RESTART-like behavior for select(), which is - # (apparently) not covered by signal.siginterrupt(signal.SIGINT, False) when a timeout is passed. - # This helps avoid an unhandled select.error(4, 'Interrupted system call') on SIGINT. - # See https://bugs.python.org/issue12224 for more info. - while 1: - try: - return select.select(*args, **kwargs) - except (OSError, select.error) as e: - if e[0] != errno.EINTR: - raise - - -# TODO(6071): require kwarg-only args after `fileobj`. -def is_readable(fileobj, timeout=None): +def is_readable(fileobj, *, timeout=None): """Check that the file-like resource is readable within the given timeout via polling. :param Union[int, SupportsFileNo] fileobj: :param Optional[int] timeout: (in seconds) :return bool """ - if PY3: - with selectors.DefaultSelector() as selector: - selector.register(fileobj, selectors.EVENT_READ) - events = selector.select(timeout=timeout) - return bool(events) - else: - readable, _, _ = safe_select([fileobj], [], [], timeout) - return bool(readable) + with selectors.DefaultSelector() as selector: + selector.register(fileobj, selectors.EVENT_READ) + events = selector.select(timeout=timeout) + return bool(events) class RecvBufferedSocket: diff --git a/tests/python/pants_test/java/test_nailgun_executor.py b/tests/python/pants_test/java/test_nailgun_executor.py index 213a315a1c8..0515a5b3065 100644 --- a/tests/python/pants_test/java/test_nailgun_executor.py +++ b/tests/python/pants_test/java/test_nailgun_executor.py @@ -81,7 +81,7 @@ def test_connect_timeout(self): unittest.mock.patch('pants.java.nailgun_executor.read_file') as mock_read_file: mock_open.return_value = stdout_read mock_read_file.return_value = 'err' - # The stdout write pipe has no input and hasn't been closed, so the select.select() should + # The stdout write pipe has no input and hasn't been closed, so the selector.select() should # time out regardless of the timemout argument, and raise. with self.assertRaisesWithMessage(NailgunExecutor.InitialNailgunConnectTimedOut, """\ Failed to read nailgun output after 0.0001 seconds! diff --git a/tests/python/pants_test/util/BUILD b/tests/python/pants_test/util/BUILD index c3538253580..4a9238d2609 100644 --- a/tests/python/pants_test/util/BUILD +++ b/tests/python/pants_test/util/BUILD @@ -144,7 +144,6 @@ python_tests( sources = ['test_socket.py'], coverage = ['pants.util.socket'], dependencies = [ - '3rdparty/python:future', 'src/python/pants/util:socket', ] ) diff --git a/tests/python/pants_test/util/test_socket.py b/tests/python/pants_test/util/test_socket.py index 2fc91c4cd6e..c4e939a05b7 100644 --- a/tests/python/pants_test/util/test_socket.py +++ b/tests/python/pants_test/util/test_socket.py @@ -6,8 +6,6 @@ import unittest import unittest.mock -from future.utils import PY3 - from pants.util.socket import RecvBufferedSocket, is_readable @@ -16,23 +14,17 @@ class TestSocketUtils(unittest.TestCase): - @unittest.mock.patch('selectors.DefaultSelector' if PY3 else 'select.select', **PATCH_OPTS) + @unittest.mock.patch('selectors.DefaultSelector', **PATCH_OPTS) def test_is_readable(self, mock_selector): mock_fileobj = unittest.mock.Mock() - if PY3: - mock_selector = mock_selector.return_value.__enter__.return_value - mock_selector.register = unittest.mock.Mock() - # NB: the return value should actually be List[Tuple[SelectorKey, Events]], but our code only - # cares that _some_ event happened so we choose a simpler mock here. See - # https://docs.python.org/3/library/selectors.html#selectors.BaseSelector.select. - mock_selector.select = unittest.mock.Mock(return_value=[(1, "")]) - else: - mock_selector.return_value = ([1], [], []) + mock_selector = mock_selector.return_value.__enter__.return_value + mock_selector.register = unittest.mock.Mock() + # NB: the return value should actually be List[Tuple[SelectorKey, Events]], but our code only + # cares that _some_ event happened so we choose a simpler mock here. See + # https://docs.python.org/3/library/selectors.html#selectors.BaseSelector.select. + mock_selector.select = unittest.mock.Mock(return_value=[(1, "")]) self.assertTrue(is_readable(mock_fileobj, timeout=0.1)) - if PY3: - mock_selector.select = unittest.mock.Mock(return_value=[]) - else: - mock_selector.return_value = ([], [], []) + mock_selector.select = unittest.mock.Mock(return_value=[]) self.assertFalse(is_readable(mock_fileobj, timeout=0.1)) @@ -64,17 +56,14 @@ def test_recv_max_larger_than_buf(self): self.server_sock.sendall(b'A' * double_chunk) self.assertEqual(self.buf_sock.recv(double_chunk), b'A' * double_chunk) - @unittest.mock.patch('selectors.DefaultSelector' if PY3 else 'select.select', **PATCH_OPTS) + @unittest.mock.patch('selectors.DefaultSelector', **PATCH_OPTS) def test_recv_check_calls(self, mock_selector): - if PY3: - mock_selector = mock_selector.return_value.__enter__.return_value - mock_selector.register = unittest.mock.Mock() - # NB: the return value should actually be List[Tuple[SelectorKey, Events]], but our code only - # cares that _some_ event happened so we choose a simpler mock here. See - # https://docs.python.org/3/library/selectors.html#selectors.BaseSelector.select. - mock_selector.select = unittest.mock.Mock(return_value=[(1, "")]) - else: - mock_selector.return_value = ([1], [], []) + mock_selector = mock_selector.return_value.__enter__.return_value + mock_selector.register = unittest.mock.Mock() + # NB: the return value should actually be List[Tuple[SelectorKey, Events]], but our code only + # cares that _some_ event happened so we choose a simpler mock here. See + # https://docs.python.org/3/library/selectors.html#selectors.BaseSelector.select. + mock_selector.select = unittest.mock.Mock(return_value=[(1, "")]) self.mock_socket.recv.side_effect = [b'A' * self.chunk_size, b'B' * self.chunk_size]