From f4c8db64f8fd12b85058c48273ddfe7f75e77550 Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Sun, 26 Dec 2021 21:04:04 +0530 Subject: [PATCH 1/9] Convert `--local-executor` in an integer flag, defaults to 1 i.e. enabled, use 0 to disable --- benchmark/_proxy.py | 2 +- proxy/common/flag.py | 2 +- proxy/core/acceptor/acceptor.py | 8 ++++---- proxy/core/acceptor/pool.py | 12 +++++++++++- proxy/proxy.py | 13 +++++++++---- tests/core/test_acceptor.py | 2 +- tests/test_main.py | 16 ++++++++-------- 7 files changed, 35 insertions(+), 20 deletions(-) diff --git a/benchmark/_proxy.py b/benchmark/_proxy.py index 363684f29f..838918ee5b 100644 --- a/benchmark/_proxy.py +++ b/benchmark/_proxy.py @@ -23,7 +23,7 @@ enable_web_server=True, disable_proxy_server=False, num_acceptors=10, - local_executor=True, + local_executor=1, log_file='/dev/null', ) as _: while True: diff --git a/proxy/common/flag.py b/proxy/common/flag.py index 6161dbd0b6..f0c7f7c7fb 100644 --- a/proxy/common/flag.py +++ b/proxy/common/flag.py @@ -340,7 +340,7 @@ def initialize( ) args.timeout = cast(int, opts.get('timeout', args.timeout)) args.local_executor = cast( - bool, + int, opts.get( 'local_executor', args.local_executor, diff --git a/proxy/core/acceptor/acceptor.py b/proxy/core/acceptor/acceptor.py index c600f00766..171497219f 100644 --- a/proxy/core/acceptor/acceptor.py +++ b/proxy/core/acceptor/acceptor.py @@ -40,10 +40,10 @@ flags.add_argument( '--local-executor', - action='store_true', - default=DEFAULT_LOCAL_EXECUTOR, - help='Default: ' + ('True' if DEFAULT_LOCAL_EXECUTOR else 'False') + '. ' + - 'Disabled by default. When enabled acceptors will make use of ' + + type=int, + default=int(DEFAULT_LOCAL_EXECUTOR), + help='Default: ' + ('1' if DEFAULT_LOCAL_EXECUTOR else '0') + '. ' + + 'Enabled by default. When enabled acceptors will make use of ' + 'local (same process) executor instead of distributing load across ' + 'remote (other process) executors. Enable this option to achieve CPU affinity between ' + 'acceptors and executors, instead of using underlying OS kernel scheduling algorithm.', diff --git a/proxy/core/acceptor/pool.py b/proxy/core/acceptor/pool.py index 90e7c77560..10c9fc0887 100644 --- a/proxy/core/acceptor/pool.py +++ b/proxy/core/acceptor/pool.py @@ -98,7 +98,17 @@ def __exit__(self, *args: Any) -> None: def setup(self) -> None: """Setup acceptors.""" self._start() - logger.info('Started %d acceptors' % self.flags.num_acceptors) + execution_mode = ( + "threadless (local)" + if self.flags.local_executor + else "threadless (remote)" + ) if self.flags.threadless else "threaded" + logger.info( + 'Started %d acceptors in %s mode' % ( + self.flags.num_acceptors, + execution_mode, + ), + ) # Send file descriptor to all acceptor processes. fd = self.listener.fileno() for index in range(self.flags.num_acceptors): diff --git a/proxy/proxy.py b/proxy/proxy.py index c35e9e0b4a..264c857ded 100644 --- a/proxy/proxy.py +++ b/proxy/proxy.py @@ -13,13 +13,14 @@ import time import logging +from pathlib import Path from typing import List, Optional, Any from .core.acceptor import AcceptorPool, ThreadlessPool, Listener from .core.event import EventManager from .common.utils import bytes_ from .common.flag import FlagParser, flags -from .common.constants import DEFAULT_LOG_FILE, DEFAULT_LOG_FORMAT, DEFAULT_LOG_LEVEL +from .common.constants import DEFAULT_LOCAL_EXECUTOR, DEFAULT_LOG_FILE, DEFAULT_LOG_FORMAT, DEFAULT_LOG_LEVEL from .common.constants import DEFAULT_OPEN_FILE_LIMIT, DEFAULT_PLUGINS, DEFAULT_VERSION from .common.constants import DEFAULT_ENABLE_DASHBOARD, DEFAULT_WORK_KLASS, DEFAULT_PID_FILE @@ -206,8 +207,12 @@ def shutdown(self) -> None: def _write_pid_file(self) -> None: if self.flags.pid_file is not None: - # NOTE: Multiple instances of proxy.py running on - # same host machine will currently result in overwriting the PID file + if Path(self.flags.pid_file).exists(): + raise ValueError( + 'pid file {0} already exists'.format( + self.flags.pid_file, + ), + ) with open(self.flags.pid_file, 'wb') as pid_file: pid_file.write(bytes_(os.getpid())) @@ -217,7 +222,7 @@ def _delete_pid_file(self) -> None: @property def remote_executors_enabled(self) -> bool: - return self.flags.threadless and not self.flags.local_executor + return self.flags.threadless and self.flags.local_executor != int(DEFAULT_LOCAL_EXECUTOR) def main(**opts: Any) -> None: diff --git a/tests/core/test_acceptor.py b/tests/core/test_acceptor.py index 69b6ecfba0..2a4d089898 100644 --- a/tests/core/test_acceptor.py +++ b/tests/core/test_acceptor.py @@ -26,7 +26,7 @@ def setUp(self) -> None: self.flags = FlagParser.initialize( threaded=True, work_klass=mock.MagicMock(), - local_executor=False, + local_executor=0, ) self.acceptor = Acceptor( idd=self.acceptor_id, diff --git a/tests/test_main.py b/tests/test_main.py index 22b9feb0dd..380ffd65b1 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -68,7 +68,7 @@ def mock_default_args(mock_args: mock.Mock) -> None: mock_args.enable_events = DEFAULT_ENABLE_EVENTS mock_args.enable_dashboard = DEFAULT_ENABLE_DASHBOARD mock_args.work_klass = DEFAULT_WORK_KLASS - mock_args.local_executor = DEFAULT_LOCAL_EXECUTOR + mock_args.local_executor = int(DEFAULT_LOCAL_EXECUTOR) @mock.patch('os.remove') @mock.patch('os.path.exists') @@ -93,7 +93,7 @@ def test_entry_point( ) -> None: pid_file = os.path.join(tempfile.gettempdir(), 'pid') mock_sleep.side_effect = KeyboardInterrupt() - mock_initialize.return_value.local_executor = False + mock_initialize.return_value.local_executor = 0 mock_initialize.return_value.enable_events = False mock_initialize.return_value.pid_file = pid_file entry_point() @@ -141,7 +141,7 @@ def test_main_with_no_flags( mock_sleep: mock.Mock, ) -> None: mock_sleep.side_effect = KeyboardInterrupt() - mock_initialize.return_value.local_executor = False + mock_initialize.return_value.local_executor = 0 mock_initialize.return_value.enable_events = False main() mock_event_manager.assert_not_called() @@ -181,7 +181,7 @@ def test_enable_events( mock_sleep: mock.Mock, ) -> None: mock_sleep.side_effect = KeyboardInterrupt() - mock_initialize.return_value.local_executor = False + mock_initialize.return_value.local_executor = 0 mock_initialize.return_value.enable_events = True main() mock_event_manager.assert_called_once() @@ -228,8 +228,8 @@ def test_enable_dashboard( mock_args = mock_parse_args.return_value self.mock_default_args(mock_args) mock_args.enable_dashboard = True - mock_args.local_executor = False - main(enable_dashboard=True, local_executor=False) + mock_args.local_executor = 0 + main(enable_dashboard=True, local_executor=0) mock_load_plugins.assert_called() self.assertEqual( mock_load_plugins.call_args_list[0][0][0], [ @@ -274,8 +274,8 @@ def test_enable_devtools( mock_args = mock_parse_args.return_value self.mock_default_args(mock_args) mock_args.enable_devtools = True - mock_args.local_executor = False - main(enable_devtools=True, local_executor=False) + mock_args.local_executor = 0 + main(enable_devtools=True, local_executor=0) mock_load_plugins.assert_called() self.assertEqual( mock_load_plugins.call_args_list[0][0][0], [ From 82bde52a62d994745e61261586032cce94472433 Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Sun, 26 Dec 2021 21:09:35 +0530 Subject: [PATCH 2/9] Consider any value other than 1 as remote mode --- proxy/core/acceptor/acceptor.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/proxy/core/acceptor/acceptor.py b/proxy/core/acceptor/acceptor.py index 171497219f..edc530f378 100644 --- a/proxy/core/acceptor/acceptor.py +++ b/proxy/core/acceptor/acceptor.py @@ -149,7 +149,7 @@ def run_once(self) -> None: if locked: self.lock.release() for work in works: - if self.flags.local_executor: + if self.flags.local_executor == int(DEFAULT_LOCAL_EXECUTOR): assert self._local_work_queue self._local_work_queue.put(work) else: @@ -172,7 +172,7 @@ def run(self) -> None: type=socket.SOCK_STREAM, ) try: - if self.flags.local_executor: + if self.flags.local_executor == int(DEFAULT_LOCAL_EXECUTOR): self._start_local() self.selector.register(self.sock, selectors.EVENT_READ) while not self.running.is_set(): @@ -181,7 +181,7 @@ def run(self) -> None: pass finally: self.selector.unregister(self.sock) - if self.flags.local_executor: + if self.flags.local_executor == int(DEFAULT_LOCAL_EXECUTOR): self._stop_local() self.sock.close() logger.debug('Acceptor#%d shutdown', self.idd) From ea5a45a8b4a77f6ba82bc0f0f0caf726a2b3a88b Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Sun, 26 Dec 2021 21:10:52 +0530 Subject: [PATCH 3/9] Use integer to disable local executor mode in integration tests --- tests/integration/test_integration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index bf7df0b889..8962350f27 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -59,7 +59,7 @@ def proxy_py_subprocess(request: Any) -> Generator[int, None, None]: 'proxy_py_subprocess', ( ('--threadless'), - ('--threadless --local-executor'), + ('--threadless --local-executor 0'), ('--threaded'), ), indirect=True, From b03dab09e728be72d7c11122d93b819739a096f0 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sun, 26 Dec 2021 15:43:08 +0000 Subject: [PATCH 4/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- proxy/core/acceptor/pool.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/proxy/core/acceptor/pool.py b/proxy/core/acceptor/pool.py index 10c9fc0887..a5ae95329a 100644 --- a/proxy/core/acceptor/pool.py +++ b/proxy/core/acceptor/pool.py @@ -99,10 +99,10 @@ def setup(self) -> None: """Setup acceptors.""" self._start() execution_mode = ( - "threadless (local)" + 'threadless (local)' if self.flags.local_executor - else "threadless (remote)" - ) if self.flags.threadless else "threaded" + else 'threadless (remote)' + ) if self.flags.threadless else 'threaded' logger.info( 'Started %d acceptors in %s mode' % ( self.flags.num_acceptors, From 3646ee61c86ed8fc9ac0baad28b418022163b972 Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Sun, 26 Dec 2021 21:17:30 +0530 Subject: [PATCH 5/9] Fix mypy errors --- proxy/proxy.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/proxy/proxy.py b/proxy/proxy.py index 264c857ded..d3c1a6b996 100644 --- a/proxy/proxy.py +++ b/proxy/proxy.py @@ -222,7 +222,8 @@ def _delete_pid_file(self) -> None: @property def remote_executors_enabled(self) -> bool: - return self.flags.threadless and self.flags.local_executor != int(DEFAULT_LOCAL_EXECUTOR) + return self.flags.threadless \ + and not (self.flags.local_executor == int(DEFAULT_LOCAL_EXECUTOR)) def main(**opts: Any) -> None: From fe2057d38a140cd5409339a955a0bc10aa1f76fd Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Sun, 26 Dec 2021 21:23:57 +0530 Subject: [PATCH 6/9] Update flags in readme --- README.md | 20 +++++++++++--------- proxy/core/acceptor/acceptor.py | 4 ++-- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index daade52a4b..6050d85a2f 100644 --- a/README.md +++ b/README.md @@ -2204,8 +2204,9 @@ To run standalone benchmark for `proxy.py`, use the following command from repo ```console ❯ proxy -h usage: -m [-h] [--enable-events] [--enable-conn-pool] [--threadless] - [--threaded] [--num-workers NUM_WORKERS] [--local-executor] - [--backlog BACKLOG] [--hostname HOSTNAME] [--port PORT] + [--threaded] [--num-workers NUM_WORKERS] + [--local-executor LOCAL_EXECUTOR] [--backlog BACKLOG] + [--hostname HOSTNAME] [--port PORT] [--unix-socket-path UNIX_SOCKET_PATH] [--num-acceptors NUM_ACCEPTORS] [--version] [--log-level LOG_LEVEL] [--log-file LOG_FILE] [--log-format LOG_FORMAT] @@ -2248,13 +2249,14 @@ options: handle each client connection. --num-workers NUM_WORKERS Defaults to number of CPU cores. - --local-executor Default: True. Disabled by default. When enabled - acceptors will make use of local (same process) - executor instead of distributing load across remote - (other process) executors. Enable this option to - achieve CPU affinity between acceptors and executors, - instead of using underlying OS kernel scheduling - algorithm. + --local-executor LOCAL_EXECUTOR + Default: 1. Enabled by default. Use 0 to disable. When + enabled acceptors will make use of local (same + process) executor instead of distributing load across + remote (other process) executors. Enable this option + to achieve CPU affinity between acceptors and + executors, instead of using underlying OS kernel + scheduling algorithm. --backlog BACKLOG Default: 100. Maximum number of pending connections to proxy server --hostname HOSTNAME Default: 127.0.0.1. Server IP address. diff --git a/proxy/core/acceptor/acceptor.py b/proxy/core/acceptor/acceptor.py index edc530f378..8efd1f0bc4 100644 --- a/proxy/core/acceptor/acceptor.py +++ b/proxy/core/acceptor/acceptor.py @@ -43,8 +43,8 @@ type=int, default=int(DEFAULT_LOCAL_EXECUTOR), help='Default: ' + ('1' if DEFAULT_LOCAL_EXECUTOR else '0') + '. ' + - 'Enabled by default. When enabled acceptors will make use of ' + - 'local (same process) executor instead of distributing load across ' + + 'Enabled by default. Use 0 to disable. When enabled acceptors ' + + 'will make use of local (same process) executor instead of distributing load across ' + 'remote (other process) executors. Enable this option to achieve CPU affinity between ' + 'acceptors and executors, instead of using underlying OS kernel scheduling algorithm.', ) From 9685b317ddf501c6b38a06d1680bdb340e1c365b Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Sun, 26 Dec 2021 21:47:38 +0530 Subject: [PATCH 7/9] Check for type --- proxy/proxy.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/proxy/proxy.py b/proxy/proxy.py index d3c1a6b996..dd6e333586 100644 --- a/proxy/proxy.py +++ b/proxy/proxy.py @@ -206,7 +206,8 @@ def shutdown(self) -> None: self._delete_pid_file() def _write_pid_file(self) -> None: - if self.flags.pid_file is not None: + if self.flags.pid_file is not None \ + and type(self.flags.pid_file) == str: if Path(self.flags.pid_file).exists(): raise ValueError( 'pid file {0} already exists'.format( @@ -217,7 +218,9 @@ def _write_pid_file(self) -> None: pid_file.write(bytes_(os.getpid())) def _delete_pid_file(self) -> None: - if self.flags.pid_file and os.path.exists(self.flags.pid_file): + if self.flags.pid_file \ + and type(self.flags.pid_file) == str \ + and os.path.exists(self.flags.pid_file): os.remove(self.flags.pid_file) @property From 193c48f6c3cc5a63320cdb2549e3917dd79e8e69 Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Sun, 26 Dec 2021 21:50:01 +0530 Subject: [PATCH 8/9] Remove pid file check for now --- proxy/proxy.py | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/proxy/proxy.py b/proxy/proxy.py index dd6e333586..1f52fab4df 100644 --- a/proxy/proxy.py +++ b/proxy/proxy.py @@ -13,7 +13,6 @@ import time import logging -from pathlib import Path from typing import List, Optional, Any from .core.acceptor import AcceptorPool, ThreadlessPool, Listener @@ -206,20 +205,12 @@ def shutdown(self) -> None: self._delete_pid_file() def _write_pid_file(self) -> None: - if self.flags.pid_file is not None \ - and type(self.flags.pid_file) == str: - if Path(self.flags.pid_file).exists(): - raise ValueError( - 'pid file {0} already exists'.format( - self.flags.pid_file, - ), - ) + if self.flags.pid_file: with open(self.flags.pid_file, 'wb') as pid_file: pid_file.write(bytes_(os.getpid())) def _delete_pid_file(self) -> None: if self.flags.pid_file \ - and type(self.flags.pid_file) == str \ and os.path.exists(self.flags.pid_file): os.remove(self.flags.pid_file) From 3e368368d616285d3e575befd172f834a77fb2a8 Mon Sep 17 00:00:00 2001 From: Abhinav Singh Date: Mon, 27 Dec 2021 09:54:11 +0530 Subject: [PATCH 9/9] Update `--local-executor` flag usage --- .github/workflows/test-library.yml | 3 ++- Dockerfile | 1 - Makefile | 2 -- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.github/workflows/test-library.yml b/.github/workflows/test-library.yml index 38d9913cbb..46b01bb7c1 100644 --- a/.github/workflows/test-library.yml +++ b/.github/workflows/test-library.yml @@ -754,7 +754,8 @@ jobs: $CONTAINER_TAG --hostname 0.0.0.0 --enable-web-server - --local-executor && ./tests/integration/test_integration.sh 8899 + && + ./tests/integration/test_integration.sh 8899 - name: Push to GHCR run: >- REGISTRY_URL="ghcr.io/abhinavsingh/proxy.py"; diff --git a/Dockerfile b/Dockerfile index 1b99e7151f..dcfb3611b1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -32,5 +32,4 @@ EXPOSE 8899/tcp ENTRYPOINT [ "proxy" ] CMD [ \ "--hostname=0.0.0.0" \ - "--local-executor" \ ] diff --git a/Makefile b/Makefile index 989b642d4c..f5fb25a8e1 100644 --- a/Makefile +++ b/Makefile @@ -143,7 +143,6 @@ lib-profile: --num-workers 1 \ --enable-web-server \ --plugin proxy.plugin.WebServerPlugin \ - --local-executor \ --backlog 65536 \ --open-file-limit 65536 \ --log-file /dev/null @@ -160,7 +159,6 @@ lib-speedscope: --num-workers 1 \ --enable-web-server \ --plugin proxy.plugin.WebServerPlugin \ - --local-executor \ --backlog 65536 \ --open-file-limit 65536 \ --log-file /dev/null