diff --git a/music_assistant/helpers/util.py b/music_assistant/helpers/util.py index 7bf457e88d..080664f1b6 100644 --- a/music_assistant/helpers/util.py +++ b/music_assistant/helpers/util.py @@ -2204,17 +2204,17 @@ async def wrapper(self: SelfT, *args: P.args, **kwargs: P.kwargs) -> R: ), ) ) + # the coroutine is built here rather than passing func and its arguments on: a + # wrapped function is free to name a parameter after one of the task options below, + # which forwarded kwargs would collide with task: asyncio.Task[R] = mass.create_task( - func, - self, - *args, + func(self, *args, **kwargs), task_id=task_id, abort_existing=False, eager_start=True, # every caller awaits the flight below and so sees the failure itself; the # task's own exception log would report a handled error as an unhandled one log_exceptions=False, - **kwargs, ) return await join_task(task) diff --git a/music_assistant/mass.py b/music_assistant/mass.py index 31d6b32227..be23c9a6f9 100644 --- a/music_assistant/mass.py +++ b/music_assistant/mass.py @@ -739,6 +739,7 @@ def create_task( target: Callable[..., Coroutine[Any, Any, _R]] | Awaitable[_R], *args: Any, task_id: str | None = None, + name: str | None = None, abort_existing: bool = False, eager_start: bool = True, log_exceptions: bool = True, @@ -752,6 +753,9 @@ def create_task( :param target: Coroutine function or awaitable to run as a task. :param args: Arguments to pass to the coroutine function. :param task_id: Optional ID to track and deduplicate tasks. + :param name: Optional name identifying the task in log messages. Task ids are not + used for this: they key on arguments such as image urls and search terms, which + do not belong in a log line. Keep a name free of those too. :param abort_existing: If True, cancel existing task with same task_id. :param eager_start: If True (default), start task immediately without waiting for next event loop iteration. This ensures proper ordering @@ -759,7 +763,11 @@ def create_task( :param log_exceptions: Set to False when the caller awaits the task and reports its failures itself; the task then logs at debug level instead of warning. - :param kwargs: Keyword arguments to pass to the coroutine function. + :param kwargs: Keyword arguments to pass to the coroutine function. The options + above take these names for themselves, so a coroutine function with a parameter + of its own called task_id, name, abort_existing, eager_start or log_exceptions + has to be called with that argument positionally, or awaited on a coroutine + built by the caller. """ if task_id and (existing := self._tracked_tasks.get(task_id)) and not existing.done(): # prevent duplicate tasks if task_id is given and already present @@ -783,12 +791,17 @@ def create_task( else: raise RuntimeError("Target is missing") - # Use asyncio.Task directly with eager_start for immediate execution - task: asyncio.Task[_R] = asyncio.Task(coro, loop=self.loop, eager_start=eager_start) - if task_id is None: task_id = uuid4().hex + # asyncio.Task is used directly for eager_start (immediate execution). An eagerly + # started task runs its first step inside the constructor, so the name has to be set + # here: it is what identifies the task in asyncio's own slow-callback warnings and in + # the exception log below. Without one asyncio numbers the task itself. + task: asyncio.Task[_R] = asyncio.Task( + coro, loop=self.loop, eager_start=eager_start, name=name + ) + def task_done_callback(_task: asyncio.Task[Any]) -> None: # done callbacks run one event loop iteration after the task finished, so a # caller may already have replaced the entry with a new task under the same @@ -1479,7 +1492,7 @@ async def _on_provider_loaded() -> None: if provider.default_name != conf.default_name: self.config.set_provider_default_name(provider.instance_id, provider.default_name) - self.create_task(_on_provider_loaded()) + self.create_task(_on_provider_loaded(), name=f"provider_loaded_{provider.instance_id}") # clear any previous error in config and signal update self.config.set(f"{CONF_PROVIDERS}/{conf.instance_id}/last_error", None) diff --git a/tests/core/test_server_base.py b/tests/core/test_server_base.py index 9b404f47c1..4e23a2d900 100644 --- a/tests/core/test_server_base.py +++ b/tests/core/test_server_base.py @@ -96,6 +96,22 @@ async def _boom() -> None: ) +async def test_create_task_names_only_on_request(mass_minimal: MusicAssistant) -> None: + """Test that a task is named only by an explicit name, never by its task_id.""" + + async def _work() -> None: + return + + named = mass_minimal.create_task(_work(), name="provider_loaded_some_provider") + assert named.get_name() == "provider_loaded_some_provider" + + # task ids key on arguments such as image urls, so they must stay out of the name + keyed = mass_minimal.create_task(_work(), task_id="palette_fetch_p1_current_https://x/y?sig=1") + assert "https://x/y?sig=1" not in keyed.get_name() + + await asyncio.gather(named, keyed) + + async def test_create_task_replacement_stays_tracked(mass_minimal: MusicAssistant) -> None: """Test that a finished task does not untrack a replacement with the same task_id.""" task_id = "test_replacement" diff --git a/tests/helpers/test_util.py b/tests/helpers/test_util.py index 195c2a3e04..d1548909af 100644 --- a/tests/helpers/test_util.py +++ b/tests/helpers/test_util.py @@ -722,6 +722,16 @@ async def test_failure_reaches_the_caller_without_being_logged( if record.levelno >= logging.WARNING and "Exception in task" in record.getMessage() ] + @pytest.mark.asyncio + async def test_argument_named_after_a_task_option_reaches_the_wrapped_method( + self, mass_minimal: MusicAssistant + ) -> None: + """A parameter sharing its name with a create_task option is still passed through.""" + caller = _GuardedCaller(mass_minimal) + caller.release.set() + + assert await caller.fetch_named(name="abc", task_id="123") == "abc-123" + @pytest.mark.asyncio async def test_instances_get_their_own_request(self, mass_minimal: MusicAssistant) -> None: """Two objects of the same class each issue their own request.""" @@ -908,6 +918,13 @@ async def fetch(self, item_id: str) -> str: raise self.error return f"result-{item_id}" + @guard_single_request + async def fetch_named(self, name: str, task_id: str) -> str: + """Return the arguments, which are named after options of mass.create_task.""" + self.calls += 1 + await self.release.wait() + return f"{name}-{task_id}" + @guard_single_request async def fetch_item( self,