From 80cadf9268c54a05996c529a36d3b10422b298c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Tue, 13 May 2025 18:46:24 +0200 Subject: [PATCH 1/6] remove thread garbage collection --- pkgs/clan-app/clan_app/app.py | 16 ++++------ .../clan-app/clan_app/deps/webview/webview.py | 32 +++---------------- 2 files changed, 11 insertions(+), 37 deletions(-) diff --git a/pkgs/clan-app/clan_app/app.py b/pkgs/clan-app/clan_app/app.py index 4446f14d7..b00616617 100644 --- a/pkgs/clan-app/clan_app/app.py +++ b/pkgs/clan-app/clan_app/app.py @@ -44,13 +44,12 @@ def app_run(app_opts: ClanAppOptions) -> int: ) -> SuccessDataClass[None] | ErrorDataClass: """Cancel a task by its op_key.""" log.info(f"Cancelling task with op_key: {task_id}") - with webview.lock: - if task_id in webview.threads: - future = webview.threads[task_id] - future.stop_event.set() - log.info(f"Task {task_id} cancelled.") - else: - log.warning(f"Task {task_id} not found.") + if task_id in webview.threads: + future = webview.threads[task_id] + future.stop_event.set() + log.info(f"Task {task_id} cancelled.") + else: + log.warning(f"Task {task_id} not found.") return SuccessDataClass( op_key=op_key, data=None, @@ -63,8 +62,7 @@ def app_run(app_opts: ClanAppOptions) -> int: ) -> SuccessDataClass[list[str]] | ErrorDataClass: """List all tasks.""" log.info("Listing all tasks.") - with webview.lock: - tasks = list(webview.threads.keys()) + tasks = list(webview.threads.keys()) return SuccessDataClass( op_key=op_key, data=tasks, diff --git a/pkgs/clan-app/clan_app/deps/webview/webview.py b/pkgs/clan-app/clan_app/deps/webview/webview.py index f47207c13..7cda2a566 100644 --- a/pkgs/clan-app/clan_app/deps/webview/webview.py +++ b/pkgs/clan-app/clan_app/deps/webview/webview.py @@ -2,7 +2,6 @@ import ctypes import json import logging import threading -import time from collections.abc import Callable from dataclasses import dataclass from enum import IntEnum @@ -54,9 +53,6 @@ class Webview: self._handle = _webview_lib.webview_create(int(debug), window) self._callbacks: dict[str, Callable[..., Any]] = {} self.threads: dict[str, WebThread] = {} - self.stopped_threads: set[str] = set() - self.lock = threading.Lock() - self.stop_garbage_collection: threading.Event = threading.Event() if size: self.size = size @@ -94,28 +90,9 @@ class Webview: def navigate(self, url: str) -> None: _webview_lib.webview_navigate(self._handle, _encode_c_string(url)) - def collect_garbage(self) -> None: - while not self.stop_garbage_collection.is_set(): - with self.lock: - for op_key in list(self.threads.keys()): - if op_key in self.stopped_threads: - log.debug(f"Collecting garbage op_key: {op_key}") - del self.threads[op_key] - self.stopped_threads.remove(op_key) - if self.stop_garbage_collection.is_set(): - break - time.sleep(1) - def run(self) -> None: - thread = threading.Thread( - target=self.collect_garbage, name="WebviewGarbageCollector" - ) - thread.start() _webview_lib.webview_run(self._handle) - self.stop_garbage_collection.set() log.info("Shutting down webview...") - if self.lock.locked(): - self.lock.release() self.destroy() def bind_jsonschema_api(self, api: MethodRegistry) -> None: @@ -178,17 +155,16 @@ class Webview: ) self.return_(seq.decode(), FuncStatus.FAILURE, serialized) finally: - self.stopped_threads.add(seq.decode()) + del self.threads[op_key] stop_event = threading.Event() thread = threading.Thread( target=thread_task, args=(stop_event,), name="WebviewThread" ) thread.start() - with self.lock: - self.threads[seq.decode()] = WebThread( - thread=thread, stop_event=stop_event - ) + self.threads[seq.decode()] = WebThread( + thread=thread, stop_event=stop_event + ) c_callback = _webview_lib.CFUNCTYPE( None, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_void_p From bbac9474964d87b83a6f747098fbb4b02be3af4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Wed, 14 May 2025 10:48:33 +0200 Subject: [PATCH 2/6] make listing/cancelling tasks less nosy in the logs --- pkgs/clan-app/clan_app/app.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkgs/clan-app/clan_app/app.py b/pkgs/clan-app/clan_app/app.py index b00616617..999d77c81 100644 --- a/pkgs/clan-app/clan_app/app.py +++ b/pkgs/clan-app/clan_app/app.py @@ -43,11 +43,11 @@ def app_run(app_opts: ClanAppOptions) -> int: task_id: str, *, op_key: str ) -> SuccessDataClass[None] | ErrorDataClass: """Cancel a task by its op_key.""" - log.info(f"Cancelling task with op_key: {task_id}") - if task_id in webview.threads: - future = webview.threads[task_id] + log.debug(f"Cancelling task with op_key: {task_id}") + future = webview.threads.get(task_id) + if future: future.stop_event.set() - log.info(f"Task {task_id} cancelled.") + log.debug(f"Task {task_id} cancelled.") else: log.warning(f"Task {task_id} not found.") return SuccessDataClass( @@ -61,7 +61,7 @@ def app_run(app_opts: ClanAppOptions) -> int: op_key: str, ) -> SuccessDataClass[list[str]] | ErrorDataClass: """List all tasks.""" - log.info("Listing all tasks.") + log.debug("Listing all tasks.") tasks = list(webview.threads.keys()) return SuccessDataClass( op_key=op_key, From 2283987bcdc31977c01d844e068637b6edb9dd32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Wed, 14 May 2025 11:19:52 +0200 Subject: [PATCH 3/6] bind_jsonschema_api: reduce method nesting --- .../clan-app/clan_app/deps/webview/webview.py | 147 ++++++++++-------- 1 file changed, 78 insertions(+), 69 deletions(-) diff --git a/pkgs/clan-app/clan_app/deps/webview/webview.py b/pkgs/clan-app/clan_app/deps/webview/webview.py index 7cda2a566..2ee40b4f7 100644 --- a/pkgs/clan-app/clan_app/deps/webview/webview.py +++ b/pkgs/clan-app/clan_app/deps/webview/webview.py @@ -2,6 +2,7 @@ import ctypes import json import logging import threading +import functools from collections.abc import Callable from dataclasses import dataclass from enum import IntEnum @@ -57,6 +58,77 @@ class Webview: if size: self.size = size + def api_wrapper( + self, + api: MethodRegistry, + wrap_method: Callable[..., Any], + method_name: str, + + seq: bytes, + req: bytes, + arg: int, + ) -> None: + def thread_task(stop_event: threading.Event) -> None: + try: + args = json.loads(req.decode()) + + log.debug(f"Calling {method_name}({args[0]})") + # Initialize dataclasses from the payload + reconciled_arguments = {} + for k, v in args[0].items(): + # Some functions expect to be called with dataclass instances + # But the js api returns dictionaries. + # Introspect the function and create the expected dataclass from dict dynamically + # Depending on the introspected argument_type + arg_class = api.get_method_argtype(method_name, k) + + # TODO: rename from_dict into something like construct_checked_value + # from_dict really takes Anything and returns an instance of the type/class + reconciled_arguments[k] = from_dict(arg_class, v) + + op_key = seq.decode() + reconciled_arguments["op_key"] = op_key + # TODO: We could remove the wrapper in the MethodRegistry + # and just call the method directly + + set_should_cancel(lambda: stop_event.is_set()) + result = wrap_method(**reconciled_arguments) + + serialized = json.dumps( + dataclass_to_dict(result), indent=4, ensure_ascii=False + ) + + log.debug(f"Result for {method_name}: {serialized}") + self.return_(seq.decode(), FuncStatus.SUCCESS, serialized) + except Exception as e: + log.exception(f"Error while handling result of {method_name}") + result = ErrorDataClass( + op_key=seq.decode(), + status="error", + errors=[ + ApiError( + message="An internal error occured", + description=str(e), + location=["bind_jsonschema_api", method_name], + ) + ], + ) + serialized = json.dumps( + dataclass_to_dict(result), indent=4, ensure_ascii=False + ) + self.return_(seq.decode(), FuncStatus.FAILURE, serialized) + finally: + del self.threads[op_key] + + stop_event = threading.Event() + thread = threading.Thread( + target=thread_task, args=(stop_event,), name="WebviewThread" + ) + thread.start() + self.threads[seq.decode()] = WebThread( + thread=thread, stop_event=stop_event + ) + def __enter__(self) -> "Webview": return self @@ -97,75 +169,12 @@ class Webview: def bind_jsonschema_api(self, api: MethodRegistry) -> None: for name, method in api.functions.items(): - - def wrapper( - seq: bytes, - req: bytes, - arg: int, - wrap_method: Callable[..., Any] = method, - method_name: str = name, - ) -> None: - def thread_task(stop_event: threading.Event) -> None: - try: - args = json.loads(req.decode()) - - log.debug(f"Calling {method_name}({args[0]})") - # Initialize dataclasses from the payload - reconciled_arguments = {} - for k, v in args[0].items(): - # Some functions expect to be called with dataclass instances - # But the js api returns dictionaries. - # Introspect the function and create the expected dataclass from dict dynamically - # Depending on the introspected argument_type - arg_class = api.get_method_argtype(method_name, k) - - # TODO: rename from_dict into something like construct_checked_value - # from_dict really takes Anything and returns an instance of the type/class - reconciled_arguments[k] = from_dict(arg_class, v) - - op_key = seq.decode() - reconciled_arguments["op_key"] = op_key - # TODO: We could remove the wrapper in the MethodRegistry - # and just call the method directly - - set_should_cancel(lambda: stop_event.is_set()) - result = wrap_method(**reconciled_arguments) - - serialized = json.dumps( - dataclass_to_dict(result), indent=4, ensure_ascii=False - ) - - log.debug(f"Result for {method_name}: {serialized}") - self.return_(seq.decode(), FuncStatus.SUCCESS, serialized) - except Exception as e: - log.exception(f"Error while handling result of {method_name}") - result = ErrorDataClass( - op_key=seq.decode(), - status="error", - errors=[ - ApiError( - message="An internal error occured", - description=str(e), - location=["bind_jsonschema_api", method_name], - ) - ], - ) - serialized = json.dumps( - dataclass_to_dict(result), indent=4, ensure_ascii=False - ) - self.return_(seq.decode(), FuncStatus.FAILURE, serialized) - finally: - del self.threads[op_key] - - stop_event = threading.Event() - thread = threading.Thread( - target=thread_task, args=(stop_event,), name="WebviewThread" - ) - thread.start() - self.threads[seq.decode()] = WebThread( - thread=thread, stop_event=stop_event - ) - + wrapper = functools.partial( + self.api_wrapper, + api, + name, + method, + ) c_callback = _webview_lib.CFUNCTYPE( None, ctypes.c_char_p, ctypes.c_char_p, ctypes.c_void_p )(wrapper) From a4057d39ae90c91e6cd96a02e2b61dfcddfd5e73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Wed, 14 May 2025 11:24:57 +0200 Subject: [PATCH 4/6] api_wrapper: more descriptive parameter names also only decode the op_key once --- .../clan-app/clan_app/deps/webview/webview.py | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/pkgs/clan-app/clan_app/deps/webview/webview.py b/pkgs/clan-app/clan_app/deps/webview/webview.py index 2ee40b4f7..70ba12cfe 100644 --- a/pkgs/clan-app/clan_app/deps/webview/webview.py +++ b/pkgs/clan-app/clan_app/deps/webview/webview.py @@ -1,8 +1,8 @@ import ctypes +import functools import json import logging import threading -import functools from collections.abc import Callable from dataclasses import dataclass from enum import IntEnum @@ -61,16 +61,17 @@ class Webview: def api_wrapper( self, api: MethodRegistry, - wrap_method: Callable[..., Any], method_name: str, - - seq: bytes, - req: bytes, + wrap_method: Callable[..., Any], + op_key_bytes: bytes, + request_data: bytes, arg: int, ) -> None: + op_key = op_key_bytes.decode() + def thread_task(stop_event: threading.Event) -> None: try: - args = json.loads(req.decode()) + args = json.loads(request_data.decode()) log.debug(f"Calling {method_name}({args[0]})") # Initialize dataclasses from the payload @@ -86,7 +87,6 @@ class Webview: # from_dict really takes Anything and returns an instance of the type/class reconciled_arguments[k] = from_dict(arg_class, v) - op_key = seq.decode() reconciled_arguments["op_key"] = op_key # TODO: We could remove the wrapper in the MethodRegistry # and just call the method directly @@ -99,11 +99,11 @@ class Webview: ) log.debug(f"Result for {method_name}: {serialized}") - self.return_(seq.decode(), FuncStatus.SUCCESS, serialized) + self.return_(op_key, FuncStatus.SUCCESS, serialized) except Exception as e: log.exception(f"Error while handling result of {method_name}") result = ErrorDataClass( - op_key=seq.decode(), + op_key=op_key, status="error", errors=[ ApiError( @@ -116,7 +116,7 @@ class Webview: serialized = json.dumps( dataclass_to_dict(result), indent=4, ensure_ascii=False ) - self.return_(seq.decode(), FuncStatus.FAILURE, serialized) + self.return_(op_key, FuncStatus.FAILURE, serialized) finally: del self.threads[op_key] @@ -125,9 +125,7 @@ class Webview: target=thread_task, args=(stop_event,), name="WebviewThread" ) thread.start() - self.threads[seq.decode()] = WebThread( - thread=thread, stop_event=stop_event - ) + self.threads[op_key] = WebThread(thread=thread, stop_event=stop_event) def __enter__(self) -> "Webview": return self From b491e9cd51b7fc3f356ce303effcb18deffc4fe1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Wed, 14 May 2025 11:41:16 +0200 Subject: [PATCH 5/6] clan-app: don't raise exception when we use ctr-c --- pkgs/clan-app/clan_app/__init__.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkgs/clan-app/clan_app/__init__.py b/pkgs/clan-app/clan_app/__init__.py index 35976eb00..47125c580 100644 --- a/pkgs/clan-app/clan_app/__init__.py +++ b/pkgs/clan-app/clan_app/__init__.py @@ -19,6 +19,10 @@ def main(argv: list[str] = sys.argv) -> int: args = parser.parse_args(argv[1:]) app_opts = ClanAppOptions(content_uri=args.content_uri, debug=args.debug) - app_run(app_opts) + try: + app_run(app_opts) + except KeyboardInterrupt: + log.info("Keyboard interrupt received, exiting...") + return 0 return 0 From b09e61f68d52b862291679f36b64f841601f31a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Thalheim?= Date: Wed, 14 May 2025 11:51:45 +0200 Subject: [PATCH 6/6] clan-app: decode function arguments ahead of launching a thread. If something goew wrong here we rather want to crash the app and get bug reports. --- .../clan-app/clan_app/deps/webview/webview.py | 39 +++++++++---------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/pkgs/clan-app/clan_app/deps/webview/webview.py b/pkgs/clan-app/clan_app/deps/webview/webview.py index 70ba12cfe..126a30fda 100644 --- a/pkgs/clan-app/clan_app/deps/webview/webview.py +++ b/pkgs/clan-app/clan_app/deps/webview/webview.py @@ -68,29 +68,28 @@ class Webview: arg: int, ) -> None: op_key = op_key_bytes.decode() + args = json.loads(request_data.decode()) + log.debug(f"Calling {method_name}({args[0]})") + + # Initialize dataclasses from the payload + reconciled_arguments = {} + for k, v in args[0].items(): + # Some functions expect to be called with dataclass instances + # But the js api returns dictionaries. + # Introspect the function and create the expected dataclass from dict dynamically + # Depending on the introspected argument_type + arg_class = api.get_method_argtype(method_name, k) + + # TODO: rename from_dict into something like construct_checked_value + # from_dict really takes Anything and returns an instance of the type/class + reconciled_arguments[k] = from_dict(arg_class, v) + + reconciled_arguments["op_key"] = op_key + # TODO: We could remove the wrapper in the MethodRegistry + # and just call the method directly def thread_task(stop_event: threading.Event) -> None: try: - args = json.loads(request_data.decode()) - - log.debug(f"Calling {method_name}({args[0]})") - # Initialize dataclasses from the payload - reconciled_arguments = {} - for k, v in args[0].items(): - # Some functions expect to be called with dataclass instances - # But the js api returns dictionaries. - # Introspect the function and create the expected dataclass from dict dynamically - # Depending on the introspected argument_type - arg_class = api.get_method_argtype(method_name, k) - - # TODO: rename from_dict into something like construct_checked_value - # from_dict really takes Anything and returns an instance of the type/class - reconciled_arguments[k] = from_dict(arg_class, v) - - reconciled_arguments["op_key"] = op_key - # TODO: We could remove the wrapper in the MethodRegistry - # and just call the method directly - set_should_cancel(lambda: stop_event.is_set()) result = wrap_method(**reconciled_arguments)