From acba6b2b57c1e3cbcdca20bf2ab197c4ff3651ae Mon Sep 17 00:00:00 2001 From: "Hanzhang Zeng (Roger)" Date: Wed, 23 Sep 2020 13:57:37 -0700 Subject: [PATCH 1/4] Add PYTHON_THREADPOOL_THREAD_COUNT app setting --- azure_functions_worker/constants.py | 1 + azure_functions_worker/dispatcher.py | 17 ++++++----------- azure_functions_worker/utils/common.py | 25 +++++++++++++++++++++++++ tests/unittests/test_utilities.py | 25 +++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 11 deletions(-) diff --git a/azure_functions_worker/constants.py b/azure_functions_worker/constants.py index 9373fe128..8c96bb06a 100644 --- a/azure_functions_worker/constants.py +++ b/azure_functions_worker/constants.py @@ -15,6 +15,7 @@ # Feature Flags (app settings) PYTHON_ROLLBACK_CWD_PATH = "PYTHON_ROLLBACK_CWD_PATH" +PYTHON_THREADPOOL_THREAD_COUNT = "PYTHON_THREADPOOL_THREAD_COUNT" # External Site URLs MODULE_NOT_FOUND_TS_URL = "https://aka.ms/functions-modulenotfound" diff --git a/azure_functions_worker/dispatcher.py b/azure_functions_worker/dispatcher.py index b72c11db0..b719d786d 100644 --- a/azure_functions_worker/dispatcher.py +++ b/azure_functions_worker/dispatcher.py @@ -23,9 +23,10 @@ from . import protos from . import constants -from .constants import CONSOLE_LOG_PREFIX +from .constants import CONSOLE_LOG_PREFIX, PYTHON_THREADPOOL_THREAD_COUNT from .logging import error_logger, logger, is_system_log_category from .logging import enable_console_logging, disable_console_logging +from .utils.common import get_app_setting from .utils.tracing import marshall_exception_trace from .utils.wrappers import disable_feature_by from asyncio import BaseEventLoop @@ -62,17 +63,11 @@ def __init__(self, loop: BaseEventLoop, host: str, port: int, self._old_task_factory = None - # A thread-pool for synchronous function calls. We limit - # the number of threads to 1 so that one Python worker can - # only run one synchronous function in parallel. This is - # because synchronous code in Python is rarely designed with - # concurrency in mind, so we don't want to allow users to - # have races in their synchronous functions. Moreover, - # because of the GIL in CPython, it rarely makes sense to - # use threads (unless the code is IO bound, but we have - # async support for that.) + # We allow the customer to change synchronous thread pool count by + # PYTHON_THREADPOOL_THREAD_COUNT app setting. The default value is 1. + thread_count = get_app_setting(PYTHON_THREADPOOL_THREAD_COUNT, '1') self._sync_call_tp = concurrent.futures.ThreadPoolExecutor( - max_workers=1) + max_workers=int(thread_count)) self._grpc_connect_timeout = grpc_connect_timeout # This is set to -1 by default to remove the limitation on msg size diff --git a/azure_functions_worker/utils/common.py b/azure_functions_worker/utils/common.py index c3bd6fd1e..1b45b643c 100644 --- a/azure_functions_worker/utils/common.py +++ b/azure_functions_worker/utils/common.py @@ -1,5 +1,6 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. +from typing import Optional import os @@ -15,3 +16,27 @@ def is_envvar_true(env_key: str) -> bool: return False return is_true_like(os.environ[env_key]) + + +def get_app_setting(setting: str, + default_value: Optional[str] = None) -> Optional[str]: + """Returns the application setting from environment variable. + + Parameters + ---------- + setting: str + The name of the application setting (e.g. FUNCTIONS_RUNTIME_VERSION) + default_value: Optional[str] + The expected return value when the application setting is not found. + + Returns + ------- + Optional[str] + A string value that is set in the application setting + """ + app_setting_value = os.getenv(setting) + + if app_setting_value is None: + return default_value + + return app_setting_value diff --git a/tests/unittests/test_utilities.py b/tests/unittests/test_utilities.py index a796c2935..ff22d7231 100644 --- a/tests/unittests/test_utilities.py +++ b/tests/unittests/test_utilities.py @@ -7,6 +7,7 @@ from azure_functions_worker.utils import common, wrappers +TEST_APP_SETTING_NAME = "TEST_APP_SETTING_NAME" TEST_FEATURE_FLAG = "APP_SETTING_FEATURE_FLAG" FEATURE_DEFAULT = 42 @@ -163,6 +164,30 @@ def test_exception_message_should_not_be_extended_on_other_exception(self): self.assertNotIn('import_error', e.msg) self.assertEqual(type(e), ValueError) + def test_application_setting_not_set_should_return_none(self): + app_setting = common.get_app_setting(TEST_APP_SETTING_NAME) + self.assertIsNone(app_setting) + + def test_application_setting_should_return_value(self): + # Set application setting by os.setenv + os.environ.update({ TEST_APP_SETTING_NAME: '42' }) + + # Try using utility to acquire application setting + app_setting = common.get_app_setting(TEST_APP_SETTING_NAME) + self.assertEqual(app_setting, '42') + + def test_application_setting_not_set_should_return_default_value(self): + app_setting = common.get_app_setting(TEST_APP_SETTING_NAME, 'default') + self.assertEqual(app_setting, 'default') + + def test_application_setting_should_ignore_default_value(self): + # Set application setting by os.setenv + os.environ.update({ TEST_APP_SETTING_NAME: '42' }) + + # Try using utility to acquire application setting + app_setting = common.get_app_setting(TEST_APP_SETTING_NAME, 'default') + self.assertEqual(app_setting, '42') + def _unset_feature_flag(self): try: os.environ.pop(TEST_FEATURE_FLAG) From 7e3511356d8ea7555b90515e192b2dc42d04467b Mon Sep 17 00:00:00 2001 From: "Hanzhang Zeng (Roger)" Date: Thu, 24 Sep 2020 10:49:22 -0700 Subject: [PATCH 2/4] Add unittest cases to check if the worker count is set --- azure_functions_worker/dispatcher.py | 26 ++--- .../show_context/__init__.py | 13 +++ .../show_context/function.json | 15 +++ tests/unittests/test_dispatcher.py | 95 +++++++++++++++++++ tests/unittests/test_utilities.py | 4 +- 5 files changed, 140 insertions(+), 13 deletions(-) create mode 100644 tests/unittests/dispatcher_functions/show_context/__init__.py create mode 100644 tests/unittests/dispatcher_functions/show_context/function.json create mode 100644 tests/unittests/test_dispatcher.py diff --git a/azure_functions_worker/dispatcher.py b/azure_functions_worker/dispatcher.py index b719d786d..de914953e 100644 --- a/azure_functions_worker/dispatcher.py +++ b/azure_functions_worker/dispatcher.py @@ -65,16 +65,18 @@ def __init__(self, loop: BaseEventLoop, host: str, port: int, # We allow the customer to change synchronous thread pool count by # PYTHON_THREADPOOL_THREAD_COUNT app setting. The default value is 1. - thread_count = get_app_setting(PYTHON_THREADPOOL_THREAD_COUNT, '1') - self._sync_call_tp = concurrent.futures.ThreadPoolExecutor( - max_workers=int(thread_count)) + self._sync_tp_max_workers: int = int(get_app_setting( + PYTHON_THREADPOOL_THREAD_COUNT, '1')) + self._sync_call_tp: concurrent.futures.Executor = ( + concurrent.futures.ThreadPoolExecutor( + max_workers=self._sync_tp_max_workers)) - self._grpc_connect_timeout = grpc_connect_timeout + self._grpc_connect_timeout: float = grpc_connect_timeout # This is set to -1 by default to remove the limitation on msg size - self._grpc_max_msg_len = grpc_max_msg_len + self._grpc_max_msg_len: int = grpc_max_msg_len self._grpc_resp_queue: queue.Queue = queue.Queue() self._grpc_connected_fut = loop.create_future() - self._grpc_thread = threading.Thread( + self._grpc_thread: threading.Thread = threading.Thread( name='grpc-thread', target=self.__poll_grpc) @classmethod @@ -85,6 +87,8 @@ async def connect(cls, host: str, port: int, worker_id: str, disp._grpc_thread.start() await disp._grpc_connected_fut logger.info('Successfully opened gRPC channel to %s:%s', host, port) + logger.info('Python worker sync threadpool max workers: %s', + disp._sync_tp_max_workers) return disp async def dispatch_forever(self): @@ -125,13 +129,13 @@ async def dispatch_forever(self): try: await forever finally: - logger.warn('Detaching gRPC logging due to exception.') + logger.warning('Detaching gRPC logging due to exception.') logging_handler.flush() root_logger.removeHandler(logging_handler) # Reenable console logging when there's an exception enable_console_logging() - logger.warn('Switched to console logging due to exception.') + logger.warning('Switched to console logging due to exception.') finally: DispatcherMeta.__current_dispatcher__ = None @@ -205,8 +209,8 @@ def _serialize_exception(exc: Exception): try: message = f'{type(exc).__name__}: {exc}' except Exception: - message = (f'Unhandled exception in function. ' - f'Could not serialize original exception message.') + message = ('Unhandled exception in function. ' + 'Could not serialize original exception message.') try: stack_trace = marshall_exception_trace(exc) @@ -470,7 +474,7 @@ def _change_cwd(self, new_cwd: str): os.chdir(new_cwd) logger.info('Changing current working directory to %s', new_cwd) else: - logger.warn('Directory %s is not found when reloading', new_cwd) + logger.warning('Directory %s is not found when reloading', new_cwd) def __run_sync_func(self, invocation_id, func, params): # This helper exists because we need to access the current diff --git a/tests/unittests/dispatcher_functions/show_context/__init__.py b/tests/unittests/dispatcher_functions/show_context/__init__.py new file mode 100644 index 000000000..17755a716 --- /dev/null +++ b/tests/unittests/dispatcher_functions/show_context/__init__.py @@ -0,0 +1,13 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. +import json +import azure.functions as func + + +def main(req: func.HttpRequest, context: func.Context) -> func.HttpResponse: + result = { + 'function_directory': context.function_directory, + 'function_name': context.function_name + } + return func.HttpResponse(body=json.dumps(result), + mimetype='application/json') diff --git a/tests/unittests/dispatcher_functions/show_context/function.json b/tests/unittests/dispatcher_functions/show_context/function.json new file mode 100644 index 000000000..7239e0fcc --- /dev/null +++ b/tests/unittests/dispatcher_functions/show_context/function.json @@ -0,0 +1,15 @@ +{ + "scriptFile": "__init__.py", + "bindings": [ + { + "type": "httpTrigger", + "direction": "in", + "name": "req" + }, + { + "type": "http", + "direction": "out", + "name": "$return" + } + ] +} \ No newline at end of file diff --git a/tests/unittests/test_dispatcher.py b/tests/unittests/test_dispatcher.py new file mode 100644 index 000000000..dfd5176d1 --- /dev/null +++ b/tests/unittests/test_dispatcher.py @@ -0,0 +1,95 @@ +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. +import os +from azure_functions_worker import protos +from azure_functions_worker import testutils +from azure_functions_worker.constants import PYTHON_THREADPOOL_THREAD_COUNT + + +class TestDispatcher(testutils.AsyncTestCase): + dispatcher_funcs_dir = testutils.UNIT_TESTS_FOLDER / 'dispatcher_functions' + + def setUp(self): + self._pre_env = dict(os.environ) + + def tearDown(self): + os.environ.clear() + os.environ.update(self._pre_env) + + async def test_dispatcher_sync_threadpool_default_worker(self): + '''Test if the sync threadpool has maximum worker count set to 1 + by default + ''' + ctrl = testutils.start_mockhost(script_root=self.dispatcher_funcs_dir) + + async with ctrl as host: + # Ensure the function can be properly loaded + func_id, load_r = await host.load_function('show_context') + self.assertEqual(load_r.response.function_id, func_id) + self.assertEqual(load_r.response.result.status, + protos.StatusResult.Success) + + # Ensure the function can be properly invoked + invoke_id, call_r = await host.invoke_function( + 'show_context', [ + protos.ParameterBinding( + name='req', + data=protos.TypedData( + http=protos.RpcHttp( + method='GET' + ) + ) + ) + ]) + self.assertIsNotNone(invoke_id) + self.assertEqual(call_r.response.result.status, + protos.StatusResult.Success) + + # Ensure the dispatcher sync threadpool count is set to 1 + self.assertEqual(ctrl._worker._sync_tp_max_workers, 1) + + async def test_dispatcher_sync_threadpool_set_worker(self): + '''Test if the sync threadpool maximum worker can be set + ''' + # Configure thread pool max worker + os.environ.update({PYTHON_THREADPOOL_THREAD_COUNT: '5'}) + ctrl = testutils.start_mockhost(script_root=self.dispatcher_funcs_dir) + + async with ctrl as host: + # Ensure the function can be properly loaded + func_id, load_r = await host.load_function('show_context') + self.assertEqual(load_r.response.function_id, func_id) + self.assertEqual(load_r.response.result.status, + protos.StatusResult.Success) + + # Ensure the function can be properly invoked + invoke_id, call_r = await host.invoke_function( + 'show_context', [ + protos.ParameterBinding( + name='req', + data=protos.TypedData( + http=protos.RpcHttp( + method='GET' + ) + ) + ) + ]) + self.assertIsNotNone(invoke_id) + self.assertEqual(call_r.response.result.status, + protos.StatusResult.Success) + + # Ensure the dispatcher sync threadpool count is set to 1 + self.assertEqual(ctrl._worker._sync_tp_max_workers, 5) + + async def test_dispatcher_sync_threadpool_invalid_worker(self): + '''Test when sync threadpool maximum worker is set to an invalid value, + the host should fail to start + ''' + # Configure thread pool max worker to an invalid value + os.environ.update({PYTHON_THREADPOOL_THREAD_COUNT: 'invalid'}) + ctrl = testutils.start_mockhost(script_root=self.dispatcher_funcs_dir) + + with self.assertRaises(ValueError): + async with ctrl as _: + raise Exception('Host shoud fail to start with invalid sync' + ' threadpool worker') diff --git a/tests/unittests/test_utilities.py b/tests/unittests/test_utilities.py index ff22d7231..c20e6e5aa 100644 --- a/tests/unittests/test_utilities.py +++ b/tests/unittests/test_utilities.py @@ -170,7 +170,7 @@ def test_application_setting_not_set_should_return_none(self): def test_application_setting_should_return_value(self): # Set application setting by os.setenv - os.environ.update({ TEST_APP_SETTING_NAME: '42' }) + os.environ.update({TEST_APP_SETTING_NAME: '42'}) # Try using utility to acquire application setting app_setting = common.get_app_setting(TEST_APP_SETTING_NAME) @@ -182,7 +182,7 @@ def test_application_setting_not_set_should_return_default_value(self): def test_application_setting_should_ignore_default_value(self): # Set application setting by os.setenv - os.environ.update({ TEST_APP_SETTING_NAME: '42' }) + os.environ.update({TEST_APP_SETTING_NAME: '42'}) # Try using utility to acquire application setting app_setting = common.get_app_setting(TEST_APP_SETTING_NAME, 'default') From 8a4017340e032fe33f4078cb52cee36b8bfc5152 Mon Sep 17 00:00:00 2001 From: "Hanzhang Zeng (Roger)" Date: Thu, 24 Sep 2020 13:12:39 -0700 Subject: [PATCH 3/4] Add validator in get_app_setting --- azure_functions_worker/dispatcher.py | 30 +++++- azure_functions_worker/utils/common.py | 29 +++++- tests/unittests/test_dispatcher.py | 126 +++++++++++++++---------- tests/unittests/test_utilities.py | 54 ++++++++++- 4 files changed, 177 insertions(+), 62 deletions(-) diff --git a/azure_functions_worker/dispatcher.py b/azure_functions_worker/dispatcher.py index de914953e..035961cba 100644 --- a/azure_functions_worker/dispatcher.py +++ b/azure_functions_worker/dispatcher.py @@ -65,8 +65,7 @@ def __init__(self, loop: BaseEventLoop, host: str, port: int, # We allow the customer to change synchronous thread pool count by # PYTHON_THREADPOOL_THREAD_COUNT app setting. The default value is 1. - self._sync_tp_max_workers: int = int(get_app_setting( - PYTHON_THREADPOOL_THREAD_COUNT, '1')) + self._sync_tp_max_workers: int = self._get_sync_tp_max_workers() self._sync_call_tp: concurrent.futures.Executor = ( concurrent.futures.ThreadPoolExecutor( max_workers=self._sync_tp_max_workers)) @@ -86,9 +85,9 @@ async def connect(cls, host: str, port: int, worker_id: str, disp = cls(loop, host, port, worker_id, request_id, connect_timeout) disp._grpc_thread.start() await disp._grpc_connected_fut - logger.info('Successfully opened gRPC channel to %s:%s', host, port) - logger.info('Python worker sync threadpool max workers: %s', - disp._sync_tp_max_workers) + logger.info('Successfully opened gRPC channel to %s:%s ' + 'with sync threadpool max workers set to %s', + host, port, disp._sync_tp_max_workers) return disp async def dispatch_forever(self): @@ -476,6 +475,27 @@ def _change_cwd(self, new_cwd: str): else: logger.warning('Directory %s is not found when reloading', new_cwd) + def _get_sync_tp_max_workers(self): + def tp_max_workers_validator(value: str): + try: + int_value = int(value) + except ValueError: + logger.warning(f'{PYTHON_THREADPOOL_THREAD_COUNT} must be an ' + 'integer') + return False + + if not 1 <= int_value <= 32: + logger.warning(f'{PYTHON_THREADPOOL_THREAD_COUNT} must be set ' + 'to a value between 1 and 32') + return False + + return True + + return int(get_app_setting( + setting=PYTHON_THREADPOOL_THREAD_COUNT, + default_value='1', + validator=tp_max_workers_validator)) + def __run_sync_func(self, invocation_id, func, params): # This helper exists because we need to access the current # invocation_id from ThreadPoolExecutor's threads. diff --git a/azure_functions_worker/utils/common.py b/azure_functions_worker/utils/common.py index 1b45b643c..a125c284e 100644 --- a/azure_functions_worker/utils/common.py +++ b/azure_functions_worker/utils/common.py @@ -1,6 +1,6 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. -from typing import Optional +from typing import Optional, Callable import os @@ -18,16 +18,25 @@ def is_envvar_true(env_key: str) -> bool: return is_true_like(os.environ[env_key]) -def get_app_setting(setting: str, - default_value: Optional[str] = None) -> Optional[str]: +def get_app_setting( + setting: str, + default_value: Optional[str] = None, + validator: Optional[Callable[[str], bool]] = None +) -> Optional[str]: """Returns the application setting from environment variable. Parameters ---------- setting: str The name of the application setting (e.g. FUNCTIONS_RUNTIME_VERSION) + default_value: Optional[str] - The expected return value when the application setting is not found. + The expected return value when the application setting is not found, + or the app setting does not pass the validator. + + validator: Optional[Callable[[str], bool]] + A function accepts the app setting value and should return True when + the app setting value is acceptable. Returns ------- @@ -36,7 +45,17 @@ def get_app_setting(setting: str, """ app_setting_value = os.getenv(setting) + # If an app setting is not configured, we return the default value if app_setting_value is None: return default_value - return app_setting_value + # If there's no validator, we should return the app setting value directly + if validator is None: + return app_setting_value + + # If the app setting is set with a validator, + # On True, should return the app setting value + # On False, should return the app setting value + if validator(app_setting_value): + return app_setting_value + return default_value diff --git a/tests/unittests/test_dispatcher.py b/tests/unittests/test_dispatcher.py index dfd5176d1..4bdf4c323 100644 --- a/tests/unittests/test_dispatcher.py +++ b/tests/unittests/test_dispatcher.py @@ -1,6 +1,7 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. import os +from unittest.mock import patch from azure_functions_worker import protos from azure_functions_worker import testutils from azure_functions_worker.constants import PYTHON_THREADPOOL_THREAD_COUNT @@ -23,27 +24,7 @@ async def test_dispatcher_sync_threadpool_default_worker(self): ctrl = testutils.start_mockhost(script_root=self.dispatcher_funcs_dir) async with ctrl as host: - # Ensure the function can be properly loaded - func_id, load_r = await host.load_function('show_context') - self.assertEqual(load_r.response.function_id, func_id) - self.assertEqual(load_r.response.result.status, - protos.StatusResult.Success) - - # Ensure the function can be properly invoked - invoke_id, call_r = await host.invoke_function( - 'show_context', [ - protos.ParameterBinding( - name='req', - data=protos.TypedData( - http=protos.RpcHttp( - method='GET' - ) - ) - ) - ]) - self.assertIsNotNone(invoke_id) - self.assertEqual(call_r.response.result.status, - protos.StatusResult.Success) + await self._check_if_function_is_ok(host) # Ensure the dispatcher sync threadpool count is set to 1 self.assertEqual(ctrl._worker._sync_tp_max_workers, 1) @@ -56,40 +37,89 @@ async def test_dispatcher_sync_threadpool_set_worker(self): ctrl = testutils.start_mockhost(script_root=self.dispatcher_funcs_dir) async with ctrl as host: - # Ensure the function can be properly loaded - func_id, load_r = await host.load_function('show_context') - self.assertEqual(load_r.response.function_id, func_id) - self.assertEqual(load_r.response.result.status, - protos.StatusResult.Success) - - # Ensure the function can be properly invoked - invoke_id, call_r = await host.invoke_function( - 'show_context', [ - protos.ParameterBinding( - name='req', - data=protos.TypedData( - http=protos.RpcHttp( - method='GET' - ) - ) - ) - ]) - self.assertIsNotNone(invoke_id) - self.assertEqual(call_r.response.result.status, - protos.StatusResult.Success) + await self._check_if_function_is_ok(host) # Ensure the dispatcher sync threadpool count is set to 1 self.assertEqual(ctrl._worker._sync_tp_max_workers, 5) - async def test_dispatcher_sync_threadpool_invalid_worker(self): + @patch('azure_functions_worker.dispatcher.logger') + async def test_dispatcher_sync_threadpool_invalid_worker_count( + self, + mock_logger + ): '''Test when sync threadpool maximum worker is set to an invalid value, - the host should fail to start + the host should fallback to default value 1 ''' # Configure thread pool max worker to an invalid value os.environ.update({PYTHON_THREADPOOL_THREAD_COUNT: 'invalid'}) ctrl = testutils.start_mockhost(script_root=self.dispatcher_funcs_dir) - with self.assertRaises(ValueError): - async with ctrl as _: - raise Exception('Host shoud fail to start with invalid sync' - ' threadpool worker') + async with ctrl as host: + await self._check_if_function_is_ok(host) + + # Ensure the dispatcher sync threadpool should fallback to 1 + self.assertEqual(ctrl._worker._sync_tp_max_workers, 1) + + mock_logger.warning.assert_any_call( + f'{PYTHON_THREADPOOL_THREAD_COUNT} must be an integer') + + @patch('azure_functions_worker.dispatcher.logger') + async def test_dispatcher_sync_threadpool_below_min_setting( + self, + mock_logger + ): + # Configure thread pool max worker to an invalid value + os.environ.update({PYTHON_THREADPOOL_THREAD_COUNT: '0'}) + ctrl = testutils.start_mockhost(script_root=self.dispatcher_funcs_dir) + + async with ctrl as host: + await self._check_if_function_is_ok(host) + + # Ensure the dispatcher sync threadpool should fallback to 1 + self.assertEqual(ctrl._worker._sync_tp_max_workers, 1) + + mock_logger.warning.assert_any_call( + f'{PYTHON_THREADPOOL_THREAD_COUNT} must be set to a value between ' + '1 and 32') + + @patch('azure_functions_worker.dispatcher.logger') + async def test_dispatcher_sync_threadpool_exceed_max_setting( + self, + mock_logger + ): + # Configure thread pool max worker to an invalid value + os.environ.update({PYTHON_THREADPOOL_THREAD_COUNT: '33'}) + ctrl = testutils.start_mockhost(script_root=self.dispatcher_funcs_dir) + + async with ctrl as host: + await self._check_if_function_is_ok(host) + + # Ensure the dispatcher sync threadpool should fallback to 1 + self.assertEqual(ctrl._worker._sync_tp_max_workers, 1) + + mock_logger.warning.assert_any_call( + f'{PYTHON_THREADPOOL_THREAD_COUNT} must be set to a value between ' + '1 and 32') + + async def _check_if_function_is_ok(self, host): + # Ensure the function can be properly loaded + func_id, load_r = await host.load_function('show_context') + self.assertEqual(load_r.response.function_id, func_id) + self.assertEqual(load_r.response.result.status, + protos.StatusResult.Success) + + # Ensure the function can be properly invoked + invoke_id, call_r = await host.invoke_function( + 'show_context', [ + protos.ParameterBinding( + name='req', + data=protos.TypedData( + http=protos.RpcHttp( + method='GET' + ) + ) + ) + ]) + self.assertIsNotNone(invoke_id) + self.assertEqual(call_r.response.result.status, + protos.StatusResult.Success) diff --git a/tests/unittests/test_utilities.py b/tests/unittests/test_utilities.py index c20e6e5aa..a8d0058ef 100644 --- a/tests/unittests/test_utilities.py +++ b/tests/unittests/test_utilities.py @@ -164,11 +164,11 @@ def test_exception_message_should_not_be_extended_on_other_exception(self): self.assertNotIn('import_error', e.msg) self.assertEqual(type(e), ValueError) - def test_application_setting_not_set_should_return_none(self): + def test_app_settings_not_set_should_return_none(self): app_setting = common.get_app_setting(TEST_APP_SETTING_NAME) self.assertIsNone(app_setting) - def test_application_setting_should_return_value(self): + def test_app_settings_should_return_value(self): # Set application setting by os.setenv os.environ.update({TEST_APP_SETTING_NAME: '42'}) @@ -176,11 +176,11 @@ def test_application_setting_should_return_value(self): app_setting = common.get_app_setting(TEST_APP_SETTING_NAME) self.assertEqual(app_setting, '42') - def test_application_setting_not_set_should_return_default_value(self): + def test_app_settings_not_set_should_return_default_value(self): app_setting = common.get_app_setting(TEST_APP_SETTING_NAME, 'default') self.assertEqual(app_setting, 'default') - def test_application_setting_should_ignore_default_value(self): + def test_app_settings_should_ignore_default_value(self): # Set application setting by os.setenv os.environ.update({TEST_APP_SETTING_NAME: '42'}) @@ -188,6 +188,52 @@ def test_application_setting_should_ignore_default_value(self): app_setting = common.get_app_setting(TEST_APP_SETTING_NAME, 'default') self.assertEqual(app_setting, '42') + def test_app_settings_should_not_trigger_validator_when_not_set(self): + def raise_excpt(value: str): + raise Exception('Should not raise on app setting not found') + + common.get_app_setting(TEST_APP_SETTING_NAME, validator=raise_excpt) + + def test_app_settings_return_default_value_when_validation_fail(self): + def parse_int_no_raise(value: str): + try: + int(value) + return True + except ValueError: + return False + + # Set application setting to an invalid value + os.environ.update({TEST_APP_SETTING_NAME: 'invalid'}) + + app_setting = common.get_app_setting( + TEST_APP_SETTING_NAME, + default_value='1', + validator=parse_int_no_raise + ) + + # Because 'invalid' is not an interger, falls back to default value + self.assertEqual(app_setting, '1') + + def test_app_settings_return_setting_value_when_validation_succeed(self): + def parse_int_no_raise(value: str): + try: + int(value) + return True + except ValueError: + return False + + # Set application setting to an invalid value + os.environ.update({TEST_APP_SETTING_NAME: '42'}) + + app_setting = common.get_app_setting( + TEST_APP_SETTING_NAME, + default_value='1', + validator=parse_int_no_raise + ) + + # Because 'invalid' is not an interger, falls back to default value + self.assertEqual(app_setting, '42') + def _unset_feature_flag(self): try: os.environ.pop(TEST_FEATURE_FLAG) From 2cdaffb11788cfbdff053e66315d0f75454519c9 Mon Sep 17 00:00:00 2001 From: "Hanzhang Zeng (Roger)" Date: Thu, 24 Sep 2020 14:21:28 -0700 Subject: [PATCH 4/4] Fix nits --- azure_functions_worker/constants.py | 7 ++++++- azure_functions_worker/dispatcher.py | 17 ++++++++++++----- azure_functions_worker/utils/common.py | 2 +- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/azure_functions_worker/constants.py b/azure_functions_worker/constants.py index 8c96bb06a..f9c1e0299 100644 --- a/azure_functions_worker/constants.py +++ b/azure_functions_worker/constants.py @@ -13,9 +13,14 @@ # Debug Flags PYAZURE_WEBHOST_DEBUG = "PYAZURE_WEBHOST_DEBUG" -# Feature Flags (app settings) +# Python Specific Feature Flags and App Settings PYTHON_ROLLBACK_CWD_PATH = "PYTHON_ROLLBACK_CWD_PATH" PYTHON_THREADPOOL_THREAD_COUNT = "PYTHON_THREADPOOL_THREAD_COUNT" +# Setting Defaults +PYTHON_THREADPOOL_THREAD_COUNT_DEFAULT = 1 +PYTHON_THREADPOOL_THREAD_COUNT_MIN = 1 +PYTHON_THREADPOOL_THREAD_COUNT_MAX = 32 + # External Site URLs MODULE_NOT_FOUND_TS_URL = "https://aka.ms/functions-modulenotfound" diff --git a/azure_functions_worker/dispatcher.py b/azure_functions_worker/dispatcher.py index 035961cba..5dc5b3e30 100644 --- a/azure_functions_worker/dispatcher.py +++ b/azure_functions_worker/dispatcher.py @@ -23,7 +23,13 @@ from . import protos from . import constants -from .constants import CONSOLE_LOG_PREFIX, PYTHON_THREADPOOL_THREAD_COUNT +from .constants import ( + CONSOLE_LOG_PREFIX, + PYTHON_THREADPOOL_THREAD_COUNT, + PYTHON_THREADPOOL_THREAD_COUNT_DEFAULT, + PYTHON_THREADPOOL_THREAD_COUNT_MIN, + PYTHON_THREADPOOL_THREAD_COUNT_MAX +) from .logging import error_logger, logger, is_system_log_category from .logging import enable_console_logging, disable_console_logging from .utils.common import get_app_setting @@ -475,8 +481,8 @@ def _change_cwd(self, new_cwd: str): else: logger.warning('Directory %s is not found when reloading', new_cwd) - def _get_sync_tp_max_workers(self): - def tp_max_workers_validator(value: str): + def _get_sync_tp_max_workers(self) -> int: + def tp_max_workers_validator(value: str) -> bool: try: int_value = int(value) except ValueError: @@ -484,7 +490,8 @@ def tp_max_workers_validator(value: str): 'integer') return False - if not 1 <= int_value <= 32: + if int_value < PYTHON_THREADPOOL_THREAD_COUNT_MIN or ( + int_value > PYTHON_THREADPOOL_THREAD_COUNT_MAX): logger.warning(f'{PYTHON_THREADPOOL_THREAD_COUNT} must be set ' 'to a value between 1 and 32') return False @@ -493,7 +500,7 @@ def tp_max_workers_validator(value: str): return int(get_app_setting( setting=PYTHON_THREADPOOL_THREAD_COUNT, - default_value='1', + default_value=f'{PYTHON_THREADPOOL_THREAD_COUNT_DEFAULT}', validator=tp_max_workers_validator)) def __run_sync_func(self, invocation_id, func, params): diff --git a/azure_functions_worker/utils/common.py b/azure_functions_worker/utils/common.py index a125c284e..3b2c6f1fb 100644 --- a/azure_functions_worker/utils/common.py +++ b/azure_functions_worker/utils/common.py @@ -55,7 +55,7 @@ def get_app_setting( # If the app setting is set with a validator, # On True, should return the app setting value - # On False, should return the app setting value + # On False, should return the default value if validator(app_setting_value): return app_setting_value return default_value