From 8b28a0325fe51429f6b50c7850dc0108e47e4bbf Mon Sep 17 00:00:00 2001 From: Carey Metcalfe Date: Fri, 8 Jul 2022 08:18:24 -0400 Subject: [PATCH 1/7] Deduplicate ThreadPool worker thread creation This patch refactors the `ThreadPool.start()` function to use the existing `ThreadPool.grow()` method to create the initial workers, reducing code duplication. --- cheroot/workers/threadpool.py | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/cheroot/workers/threadpool.py b/cheroot/workers/threadpool.py index 2a9878dc0f..b583372a93 100644 --- a/cheroot/workers/threadpool.py +++ b/cheroot/workers/threadpool.py @@ -168,17 +168,7 @@ def __init__( def start(self): """Start the pool of threads.""" - for _ in range(self.min): - self._threads.append(WorkerThread(self.server)) - for worker in self._threads: - worker.name = ( - 'CP Server {worker_name!s}'. - format(worker_name=worker.name) - ) - worker.start() - for worker in self._threads: - while not worker.ready: - time.sleep(.1) + self.grow(self.min) @property def idle(self): # noqa: D401; irrelevant for properties @@ -215,8 +205,9 @@ def grow(self, amount): n_new = min(amount, budget) workers = [self._spawn_worker() for i in range(n_new)] - while not all(worker.ready for worker in workers): - time.sleep(.1) + for worker in workers: + while not worker.ready: + time.sleep(.1) self._threads.extend(workers) def _spawn_worker(self): From ed772e9f8f0ddb137a2a3a95a3b69041f1ac36d2 Mon Sep 17 00:00:00 2001 From: Carey Metcalfe Date: Tue, 11 Oct 2022 01:07:47 -0400 Subject: [PATCH 2/7] Add ThreadPool parameter/usage validation - Prevent invalid min and max configurations - Prevent the threadpool from being started multiple times - Add tests for both --- cheroot/test/test_server.py | 80 +++++++++++++++++++++++++++++++++++ cheroot/workers/threadpool.py | 16 ++++++- 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index 5e0a6832ed..7e13d3ccb8 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -5,6 +5,7 @@ import socket import tempfile import threading +import types import uuid import urllib.parse # noqa: WPS301 @@ -17,6 +18,7 @@ from .._compat import bton, ntob from .._compat import IS_LINUX, IS_MACOS, IS_WINDOWS, SYS_PLATFORM from ..server import IS_UID_GID_RESOLVABLE, Gateway, HTTPServer +from ..workers.threadpool import ThreadPool from ..testing import ( ANY_INTERFACE_IPV4, ANY_INTERFACE_IPV6, @@ -439,3 +441,81 @@ def many_open_sockets(request, resource_limit): # Close our open resources for test_socket in test_sockets: test_socket.close() + + +@pytest.mark.parametrize( + ('minthreads', 'maxthreads'), + ( + (1, -2), # the docstring only mentions -1 to mean "no max", but other + # negative numbers should also work + (1, -1), + (1, 1), + (1, 2), + (2, -2), + (2, -1), + (2, 2), + ), +) +def test_threadpool_threadrange_set(minthreads, maxthreads): + """Test setting the number of threads in a ThreadPool. + + The ThreadPool should properly set the min+max number of the threads to use + in the pool if those limits are valid. + """ + tp = ThreadPool( + server=None, + min=minthreads, + max=maxthreads, + ) + assert tp.min == minthreads + assert tp.max == maxthreads + + +@pytest.mark.parametrize( + ('minthreads', 'maxthreads', 'error'), + ( + (-1, -1, 'min=-1 must be > 0'), + (-1, 0, 'min=-1 must be > 0'), + (-1, 1, 'min=-1 must be > 0'), + (-1, 2, 'min=-1 must be > 0'), + (0, -1, 'min=0 must be > 0'), + (0, 0, 'min=0 must be > 0'), + (0, 1, 'min=0 must be > 0'), + (0, 2, 'min=0 must be > 0'), + (1, 0, 'max=0 must be > min=1'), + (2, 0, 'max=0 must be > min=2'), + (2, 1, 'max=1 must be > min=2'), + ), +) +def test_threadpool_invalid_threadrange(minthreads, maxthreads, error): + """Test that a ThreadPool rejects invalid min/max values. + + The ThreadPool should raise an error with the proper message when + initialized with an invalid min+max number of threads. + """ + with pytest.raises(ValueError, match=error): + ThreadPool( + server=None, + min=minthreads, + max=maxthreads, + ) + + +def test_threadpool_multistart_validation(monkeypatch): + """Test for ThreadPool multi-start behavior. + + Tests that when calling start() on a ThreadPool multiple times raises a + :exc:`RuntimeError` + """ + # replace _spawn_worker with a function that returns a placeholder to avoid + # actually starting any threads + monkeypatch.setattr( + ThreadPool, + '_spawn_worker', + lambda _: types.SimpleNamespace(ready=True), + ) + + tp = ThreadPool(server=None) + tp.start() + with pytest.raises(RuntimeError, match='Threadpools can only be started once.'): + tp.start() diff --git a/cheroot/workers/threadpool.py b/cheroot/workers/threadpool.py index b583372a93..b525257297 100644 --- a/cheroot/workers/threadpool.py +++ b/cheroot/workers/threadpool.py @@ -151,12 +151,19 @@ def __init__( server (cheroot.server.HTTPServer): web server object receiving this request min (int): minimum number of worker threads - max (int): maximum number of worker threads + max (int): maximum number of worker threads (-1 for no max) accepted_queue_size (int): maximum number of active requests in queue accepted_queue_timeout (int): timeout for putting request into queue + + :raises ValueError: if the min/max values are invalid """ + if min < 1: + raise ValueError('min={} must be > 0'.format(min)) + if -1 < max < min: + raise ValueError('max={} must be > min={} (or -1 for no max)'.format(max, min)) + self.server = server self.min = min self.max = max @@ -167,7 +174,12 @@ def __init__( self._pending_shutdowns = collections.deque() def start(self): - """Start the pool of threads.""" + """Start the pool of threads. + + :raises RuntimeError: if the pool is already started + """ + if self._threads: + raise RuntimeError('Threadpools can only be started once.') self.grow(self.min) @property From 2ac833df680cbec1696bc52884764752f8354fe4 Mon Sep 17 00:00:00 2001 From: Carey Metcalfe Date: Wed, 15 Feb 2023 23:38:01 -0500 Subject: [PATCH 3/7] Refactor ThreadPool to accept inf as max thread number Accepting -1 is kept for compatibility --- cheroot/test/test_server.py | 15 +++++++++++---- cheroot/workers/threadpool.py | 28 +++++++++++++++++++--------- 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index 7e13d3ccb8..a08a48302e 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -451,9 +451,11 @@ def many_open_sockets(request, resource_limit): (1, -1), (1, 1), (1, 2), + (1, float('inf')), (2, -2), (2, -1), (2, 2), + (2, float('inf')), ), ) def test_threadpool_threadrange_set(minthreads, maxthreads): @@ -468,7 +470,10 @@ def test_threadpool_threadrange_set(minthreads, maxthreads): max=maxthreads, ) assert tp.min == minthreads - assert tp.max == maxthreads + if maxthreads < 0: + assert tp.max == float('inf') + else: + assert tp.max == maxthreads @pytest.mark.parametrize( @@ -482,8 +487,10 @@ def test_threadpool_threadrange_set(minthreads, maxthreads): (0, 0, 'min=0 must be > 0'), (0, 1, 'min=0 must be > 0'), (0, 2, 'min=0 must be > 0'), - (1, 0, 'max=0 must be > min=1'), - (2, 0, 'max=0 must be > min=2'), + (1, 0, 'Expected an integer or the infinity value for the `max` argument but got 0.'), + (1, 0.5, 'Expected an integer or the infinity value for the `max` argument but got 0.5.'), + (2, 0, 'Expected an integer or the infinity value for the `max` argument but got 0.'), + (2, '1', 'Expected an integer or the infinity value for the `max` argument but got \'1\'.'), (2, 1, 'max=1 must be > min=2'), ), ) @@ -493,7 +500,7 @@ def test_threadpool_invalid_threadrange(minthreads, maxthreads, error): The ThreadPool should raise an error with the proper message when initialized with an invalid min+max number of threads. """ - with pytest.raises(ValueError, match=error): + with pytest.raises((ValueError, TypeError), match=error): ThreadPool( server=None, min=minthreads, diff --git a/cheroot/workers/threadpool.py b/cheroot/workers/threadpool.py index b525257297..faebdcce55 100644 --- a/cheroot/workers/threadpool.py +++ b/cheroot/workers/threadpool.py @@ -151,18 +151,33 @@ def __init__( server (cheroot.server.HTTPServer): web server object receiving this request min (int): minimum number of worker threads - max (int): maximum number of worker threads (-1 for no max) + max (int): maximum number of worker threads (-1/inf for no max) accepted_queue_size (int): maximum number of active requests in queue accepted_queue_timeout (int): timeout for putting request into queue :raises ValueError: if the min/max values are invalid + :raises TypeError: if the max is not an integer or inf """ if min < 1: raise ValueError('min={} must be > 0'.format(min)) - if -1 < max < min: - raise ValueError('max={} must be > min={} (or -1 for no max)'.format(max, min)) + + if max == float('inf'): + pass + elif not isinstance(max, int) or max == 0: + raise TypeError( + 'Expected an integer or the infinity value for the `max` ' + 'argument but got {!r}.'.format(max), + ) + elif max < 0: + max = float('inf') + + if max < min: + raise ValueError( + 'max={} must be > min={} (or infinity for no max)' + ''.format(max, min), + ) self.server = server self.min = min @@ -208,12 +223,7 @@ def _clear_dead_threads(self): def grow(self, amount): """Spawn new worker threads (not above self.max).""" - if self.max > 0: - budget = max(self.max - len(self._threads), 0) - else: - # self.max <= 0 indicates no maximum - budget = float('inf') - + budget = max(self.max - len(self._threads), 0) n_new = min(amount, budget) workers = [self._spawn_worker() for i in range(n_new)] From 8a57a221eee1fcafb040654fb08dc261a35a34a0 Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Fri, 17 Mar 2023 16:58:42 +0100 Subject: [PATCH 4/7] Ignore WPS226 flake8 rule in `test_server.py` This rule is more useful in the non-test code. --- .flake8 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.flake8 b/.flake8 index ec124cf3ed..6d68f5157f 100644 --- a/.flake8 +++ b/.flake8 @@ -107,7 +107,7 @@ per-file-ignores = cheroot/test/test_core.py: C815, DAR101, DAR201, DAR401, I003, I004, N805, N806, S101, WPS110, WPS111, WPS114, WPS121, WPS202, WPS204, WPS226, WPS229, WPS302, WPS306, WPS317, WPS323, WPS324, WPS326, WPS421, WPS422, WPS432, WPS437, WPS442 cheroot/test/test_dispatch.py: DAR101, DAR201, S101, WPS111, WPS121, WPS302, WPS422, WPS430 cheroot/test/test_ssl.py: C818, DAR101, DAR201, DAR301, DAR401, E800, I001, I003, I004, I005, S101, S309, S404, S603, WPS100, WPS110, WPS111, WPS114, WPS121, WPS130, WPS201, WPS202, WPS204, WPS210, WPS211, WPS218, WPS219, WPS222, WPS226, WPS231, WPS300, WPS301, WPS317, WPS318, WPS324, WPS326, WPS335, WPS336, WPS337, WPS352, WPS408, WPS420, WPS421, WPS422, WPS432, WPS436, WPS440, WPS441, WPS442, WPS450, WPS509, WPS510, WPS608 - cheroot/test/test_server.py: DAR101, DAR201, DAR301, I001, I003, I004, I005, S101, WPS110, WPS111, WPS118, WPS121, WPS122, WPS130, WPS201, WPS202, WPS210, WPS218, WPS229, WPS300, WPS317, WPS318, WPS324, WPS326, WPS421, WPS422, WPS430, WPS432, WPS433, WPS436, WPS437, WPS442, WPS507, WPS509, WPS608 + cheroot/test/test_server.py: DAR101, DAR201, DAR301, I001, I003, I004, I005, S101, WPS110, WPS111, WPS118, WPS121, WPS122, WPS130, WPS201, WPS202, WPS210, WPS218, WPS226, WPS229, WPS300, WPS317, WPS318, WPS324, WPS326, WPS421, WPS422, WPS430, WPS432, WPS433, WPS436, WPS437, WPS442, WPS507, WPS509, WPS608 cheroot/test/test_conn.py: B007, DAR101, DAR201, DAR301, DAR401, E800, I001, I003, I004, I005, N802, N805, RST304, S101, S310, WPS100, WPS110, WPS111, WPS114, WPS115, WPS120, WPS121, WPS122, WPS201, WPS202, WPS204, WPS210, WPS211, WPS213, WPS214, WPS218, WPS219, WPS226, WPS231, WPS301, WPS306, WPS317, WPS318, WPS323, WPS326, WPS361, WPS420, WPS421, WPS422, WPS425, WPS429, WPS430, WPS432, WPS435, WPS436, WPS437, WPS440, WPS442, WPS447, WPS462, WPS508, WPS509, WPS510, WPS526 cheroot/test/webtest.py: B007, DAR101, DAR201, DAR401, I001, I003, I004, N802, RST303, RST304, S101, S104, WPS100, WPS110, WPS111, WPS115, WPS120, WPS121, WPS122, WPS201, WPS202, WPS204, WPS210, WPS211, WPS213, WPS214, WPS220, WPS221, WPS223, WPS229, WPS230, WPS231, WPS236, WPS301, WPS306, WPS317, WPS323, WPS326, WPS338, WPS361, WPS414, WPS420, WPS421, WPS422, WPS430, WPS432, WPS433, WPS437, WPS440, WPS501, WPS503, WPS505, WPS601 cheroot/testing.py: B014, C815, DAR101, DAR201, DAR301, I001, I003, S104, WPS100, WPS211, WPS229, WPS301, WPS306, WPS317, WPS414, WPS420, WPS422, WPS430, WPS503, WPS526 From 6adafc98533923e054d83d78e5076d36b9b66e6b Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Fri, 17 Mar 2023 17:03:02 +0100 Subject: [PATCH 5/7] Inverse quotes @ test_threadpool_invalid_threadrange param This makes it possible not to escape the internal ones. --- cheroot/test/test_server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index a08a48302e..6c243ffb5b 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -490,7 +490,7 @@ def test_threadpool_threadrange_set(minthreads, maxthreads): (1, 0, 'Expected an integer or the infinity value for the `max` argument but got 0.'), (1, 0.5, 'Expected an integer or the infinity value for the `max` argument but got 0.5.'), (2, 0, 'Expected an integer or the infinity value for the `max` argument but got 0.'), - (2, '1', 'Expected an integer or the infinity value for the `max` argument but got \'1\'.'), + (2, '1', "Expected an integer or the infinity value for the `max` argument but got '1'."), (2, 1, 'max=1 must be > min=2'), ), ) From e6ae44b5d4fcd4f3c4ba10caa72553643c8abc8a Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Fri, 17 Mar 2023 17:10:47 +0100 Subject: [PATCH 6/7] Replace `str.format()` with f-strings They used to be forbidden by the WPS linter rules but are now allowed. This also makes the placeholder names apparent. --- cheroot/workers/threadpool.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/cheroot/workers/threadpool.py b/cheroot/workers/threadpool.py index faebdcce55..3437d9bda9 100644 --- a/cheroot/workers/threadpool.py +++ b/cheroot/workers/threadpool.py @@ -161,22 +161,21 @@ def __init__( :raises TypeError: if the max is not an integer or inf """ if min < 1: - raise ValueError('min={} must be > 0'.format(min)) + raise ValueError(f'min={min!s} must be > 0') if max == float('inf'): pass elif not isinstance(max, int) or max == 0: raise TypeError( 'Expected an integer or the infinity value for the `max` ' - 'argument but got {!r}.'.format(max), + f'argument but got {max!r}.', ) elif max < 0: max = float('inf') if max < min: raise ValueError( - 'max={} must be > min={} (or infinity for no max)' - ''.format(max, min), + f'max={max!s} must be > min={min!s} (or infinity for no max)', ) self.server = server From 2f16a25b6e9a7fddeb00a95ef2faa03ab7375570 Mon Sep 17 00:00:00 2001 From: Sviatoslav Sydorenko Date: Fri, 17 Mar 2023 17:18:57 +0100 Subject: [PATCH 7/7] Flatten `test_threadpool_threadrange_set` It had param-dependent logic in the test which usually leads to mistakes over time. This patch makes the test body simple. --- cheroot/test/test_server.py | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/cheroot/test/test_server.py b/cheroot/test/test_server.py index 6c243ffb5b..cafa7472b6 100644 --- a/cheroot/test/test_server.py +++ b/cheroot/test/test_server.py @@ -444,21 +444,26 @@ def many_open_sockets(request, resource_limit): @pytest.mark.parametrize( - ('minthreads', 'maxthreads'), + ('minthreads', 'maxthreads', 'inited_maxthreads'), ( - (1, -2), # the docstring only mentions -1 to mean "no max", but other - # negative numbers should also work - (1, -1), - (1, 1), - (1, 2), - (1, float('inf')), - (2, -2), - (2, -1), - (2, 2), - (2, float('inf')), + ( + # NOTE: The docstring only mentions -1 to mean "no max", but other + # NOTE: negative numbers should also work. + 1, + -2, + float('inf'), + ), + (1, -1, float('inf')), + (1, 1, 1), + (1, 2, 2), + (1, float('inf'), float('inf')), + (2, -2, float('inf')), + (2, -1, float('inf')), + (2, 2, 2), + (2, float('inf'), float('inf')), ), ) -def test_threadpool_threadrange_set(minthreads, maxthreads): +def test_threadpool_threadrange_set(minthreads, maxthreads, inited_maxthreads): """Test setting the number of threads in a ThreadPool. The ThreadPool should properly set the min+max number of the threads to use @@ -470,10 +475,7 @@ def test_threadpool_threadrange_set(minthreads, maxthreads): max=maxthreads, ) assert tp.min == minthreads - if maxthreads < 0: - assert tp.max == float('inf') - else: - assert tp.max == maxthreads + assert tp.max == inited_maxthreads @pytest.mark.parametrize(