I don't get this comment: <https:// | Home Assistant | Page 1

lament schooner Nov 17, 2023, 2:11 PM

#

I explain it a bit more in depth in the comment below, I think.

The trouble is that hass._tasks are moved to a local variable at the beginning of async_stop https://github.com/home-assistant/core/blob/946d8d9fca4a39cb3c740e334e0b89f7bfd86a0a/homeassistant/core.py#L809

#

and you can't alter/mock local variables, which was the issue Erwin was trying to get across

keen knoll Nov 17, 2023, 2:12 PM

#

right but can't you add something to hass._tasks before async_stop gets called?

lament schooner Nov 17, 2023, 2:17 PM

#

i've tried that, actually

async def test_stage_shutdown_generic_error(hass: HomeAssistant, caplog) -> None:
    """Simulate a shutdown, test that a generic error at the final stage doesn't prevent it."""

    never_set = asyncio.Event()

    async def dummy_task() -> None:
        await never_set.wait()
        return

    def wait_and_fail():
        time.sleep(0.05)
        never_set.set()
        raise Exception("final stage task cancellation error")

    task = hass.loop.create_task(dummy_task(), name="dummy_task")
    with patch.object(task, "cancel", side_effect=wait_and_fail()):
        await hass.async_stop()

    assert "final stage task cancellation error" in caplog.text
    assert hass.state == CoreState.stopped

#

i seem to be doing something wrong

FAILED tests/test_core.py::test_stage_shutdown_generic_error - RuntimeError: Detected blocking call to sleep inside the event loop. Use `await hass.async_add_executor_job()`; This is causing stability issues. Please create a bug report at https://github.com/home-assistant/core/issues?q=is%3Aopen+is%3Aissue
FAILED tests/test_core.py::test_stage_shutdown_generic_error - Failed: Lingering task after test <Task pending name='dummy_task' coro=<test_stage_shutdown_generic_error.<locals>.dummy_task() running at /workspaces/ha-core/tests/test_core.py:421> wait_for=<Future pending cb=[Task.task_wakeup()] created a...

keen knoll Nov 17, 2023, 2:19 PM

#

instead of setting the event

#

just raise the exception after sleeping

lament schooner Nov 17, 2023, 2:19 PM

#

yeah, that didn't work either

#

let me try again

#

AILED tests/test_core.py::test_stage_shutdown_generic_error - RuntimeError: Detected blocking call to sleep inside the event loop. Use `await hass.async_add_executor_job()`; This is causing stability issues. Please create a bug report at https://github.com/home-assistant/core/issues?q=is%3Aopen+is%3Aissue
FAILED tests/test_core.py::test_stage_shutdown_generic_error - Failed: Lingering task after test <Task pending name='dummy_task' coro=<test_stage_shutdown_generic_error.<locals>.dummy_task() running at /workspaces/ha-core/tests/test_core.py:421> wait_for=<Future pending cb=[Task.task_wakeup()] created a...

pliant nova Nov 17, 2023, 2:20 PM

#

Great to see more people are helping!

lament schooner Nov 17, 2023, 2:20 PM

#

Raman always did

keen knoll Nov 17, 2023, 2:21 PM

#

yeah ok so the exception is being set in the wrong place

lament schooner Nov 17, 2023, 2:22 PM

#

i just realized that i don't think wait_and_fail should be called when passed as the side_effect, since it's not async

#

so i've removed the calling parentheses () and it's a bit different now

FAILED tests/test_core.py::test_stage_shutdown_generic_error - AssertionError: assert 'final stage task cancellation error' in '2023-11-17 14:22:08.890 DEBUG    MainThread homeassistant.core:core.py:1106 Bus:Handling <Event homeassistant_stoppin...3-11-17 14:22:08.892 DEBUG    MainThread homeassistant.c...
FAILED tests/test_core.py::test_stage_shutdown_generic_error - Failed: Lingering task after test <Task pending name='dummy_task' coro=<test_stage_shutdown_generic_error.<locals>.dummy_task() running at /workspaces/ha-core/tests/test_core.py:421> wait_for=<Future pending cb=[Task.task_wakeup()] created a...

#

that doesn't mean I am able to properly call it

keen knoll Nov 17, 2023, 2:26 PM

#

lament schooner Raman always did

always tried anyway 😉

#

so here's what I am playing with

#

async def test_stage_shutdown_generic_error(hass: HomeAssistant, caplog) -> None:
    """Simulate a shutdown, test that a generic error at the final stage doesn't prevent it."""

    never_set = asyncio.Event()

    async def dummy_task() -> None:
        await never_set.wait()
        return

    task = hass.loop.create_task(dummy_task(), name="dummy_task")

    def wait_and_fail():
        time.sleep(0.05)
        task.set_exception(Exception("final stage task cancellation error"))

    with patch.object(task, "cancel", side_effect=wait_and_fail):
        await hass.async_stop()

    assert "final stage task cancellation error" in caplog.text
    assert hass.state == ha.CoreState.stopped

#

instead of raising an exception in the side effect, I am trying to set the exception on the task

#

this test should work even without your PR right?

lament schooner Nov 17, 2023, 2:30 PM

#

i think so, yes. old code was revealed as not covered

#

i see what you're doing, but doesn't set_exception imply that the task is run instead of cancelled?

keen knoll Nov 17, 2023, 2:31 PM

#

good question

lament schooner Nov 17, 2023, 2:33 PM

#

as far as I can tell, that code above never runs wait_and_fail. i don't know why

#

neither does mine, actually

#

yup...

    with patch.object(task, "cancel", side_effect=wait_and_fail) as patched_cancel:
        await hass.async_stop()
        assert patched_cancel.called

FAILED tests/test_core.py::test_stage_shutdown_generic_error - AssertionError: assert False

keen knoll Nov 17, 2023, 2:38 PM

#

right well you aren't even seeing this warning:

_LOGGER.warning(
                "Task %s was still running after stage 2 shutdown; "
                "Integrations should cancel non-critical tasks when receiving "
                "the stop event to prevent delaying shutdown",
                task,
            )

lament schooner Nov 17, 2023, 2:39 PM

#

hm. true

#

i re-worded that warning due to stages being renamed, but i understand there should have been at least a warning

keen knoll Nov 17, 2023, 2:41 PM

#

are you seeing the lingering task error?

lament schooner Nov 17, 2023, 2:42 PM

#

yes

#

that's because i never set the event

#

if I set it after await hass.async_stop() the warning goes away

keen knoll Nov 17, 2023, 2:44 PM

#

the task never gets added to hass's task list

#

here's what running_tasks looks like before you loop through to cancel: 2023-11-17 09:44:10.079 ERROR MainThread homeassistant.core:core.py:851 set()

lament schooner Nov 17, 2023, 2:46 PM

#

but that's because of https://github.com/home-assistant/core/blob/946d8d9fca4a39cb3c740e334e0b89f7bfd86a0a/homeassistant/core.py#L812

#

right?

keen knoll Nov 17, 2023, 2:46 PM

#

no

lament schooner Nov 17, 2023, 2:46 PM

#

oh, wait, you said running_tasks. 🤨 i don't understand

keen knoll Nov 17, 2023, 2:47 PM

#

it's because you are creating the task directly on the loop

lament schooner Nov 17, 2023, 2:48 PM

#

so what's the way to do it, then? hass.async_add_something?

keen knoll Nov 17, 2023, 2:48 PM

#

you need to call hass.async_create_task

#

it's just a wrapper that adds it to the task list as well

#

here's where I am at so far

#

async def test_stage_shutdown_generic_error(hass: HomeAssistant, caplog) -> None:
    """Simulate a shutdown, test that a generic error at the final stage doesn't prevent it."""

    never_set = asyncio.Event()

    async def dummy_task() -> None:
        await never_set.wait()
        return

    task = hass.async_create_task(dummy_task(), name="dummy_task")

    async def wait_and_fail():
        await asyncio.sleep(0.05)
        return Exception("test_exception")

    with patch.object(task, "_make_cancelled_error", side_effect=wait_and_fail):
        await hass.async_stop()

    assert "test_exception" in caplog.text
    assert hass.state == ha.CoreState.stopped

lament schooner Nov 17, 2023, 2:51 PM

#

right, that's what i just edited. good catch!

#

you have a return Exception. should that be raise Exception?

keen knoll Nov 17, 2023, 2:52 PM

#

well, I was trying to go deeper into the task

#

_make_cancelled_error returns the exception, but I don't think it gets called in this case

lament schooner Nov 17, 2023, 2:54 PM

#

keen knoll `_make_cancelled_error` returns the exception, but I don't think it gets called ...

where is this? who calls it? remember we patched task.cancel

keen knoll Nov 17, 2023, 2:55 PM

#

yeah it only gets called when you try to access the result of the task and it's been cancelled

#

went down the wrong path

#

try this

#

async def test_stage_shutdown_generic_error(hass: HomeAssistant, caplog) -> None:
    """Simulate a shutdown, test that a generic error at the final stage doesn't prevent it."""
    task = asyncio.Future()
    hass._tasks.add(task)

    def wait_and_fail(_):
        task.set_exception(Exception("test_exception"))

    with patch.object(task, "cancel", side_effect=wait_and_fail) as patched_call:
        await hass.async_stop()

    assert "test_exception" in caplog.text
    assert hass.state == ha.CoreState.stopped

#

the issue was that you needed to set the exception directly on the task, raising an exception during the cancel function wouldn't accomplish anything because the task itself didn't have the exception

#

but asyncio.Task doesn't let you set an exception on it externally I geuss? asyncio.Future does

pliant nova Nov 17, 2023, 3:05 PM

#

keen knoll but `asyncio.Task` doesn't let you set an exception on it externally I geuss? `...

I tried this and it didn't work. My feeling says that is the direction to get this working

lament schooner Nov 17, 2023, 3:06 PM

#

keen knoll ```python async def test_stage_shutdown_generic_error(hass: HomeAssistant, caplo...

looks like it's working

#

let's see what the code coverage test says

pliant nova Nov 17, 2023, 3:08 PM

#

lament schooner let's see what the code coverage test says

Fingers crossed

lament schooner Nov 17, 2023, 3:10 PM

#

this is super weird. indeed it says that the branch with generic exceptions is covered, but for some reason, it goes back to saying that the other ones aren't 🙂

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

---------- coverage: platform linux, python 3.11.4-final-0 -----------
Name                    Stmts   Miss  Cover   Missing
-----------------------------------------------------
homeassistant/core.py     919     72    92%   395-403, 412-426, 462-466, 484, 715, 727, 775-779, 798-799, 802, 873, 885, 888, 916, 934, 1068, 1078, 1120-1122, 1126-1127, 1141-1149, 1176, 1211-1219, 1357, 1387, 1485, 1530-1533, 1574, 1617, 1660, 1746, 1850, 1874-1876, 1931, 1971, 2039, 2044-2051, 2095, 2101, 2188, 2303-2304, 2449-2450
-----------------------------------------------------
TOTAL                     919     72    92%

#

873, 885, 888 in particular

#

from here, that is https://github.com/home-assistant/core/blob/946d8d9fca4a39cb3c740e334e0b89f7bfd86a0a/homeassistant/core.py#L873

#

although... that may have been expected, actually 🤔

keen knoll Nov 17, 2023, 3:12 PM

#

yeah you don't change or cover those

lament schooner Nov 17, 2023, 3:12 PM

#

yes, exactly

keen knoll Nov 17, 2023, 3:12 PM

#

your changes are in lines 400-460

#

so I think you are OK from a patch perspective

lament schooner Nov 17, 2023, 3:12 PM

#

lemme push and see

#

thanks loads, Raman!

keen knoll Nov 17, 2023, 3:13 PM

#

sure, fun challenge 🙂

lament schooner Nov 17, 2023, 3:23 PM

#

alrighty, then, I'll leave the CI to do its job here https://github.com/home-assistant/core/pull/91165 and go pick up mini-tetele from the kindergarden. i'll be around later
thanks again, gentlemen!

pliant nova Nov 17, 2023, 3:24 PM

#

lament schooner alrighty, then, I'll leave the CI to do its job here https://github.com/home-ass...

We're all very curious

lament schooner Nov 17, 2023, 3:25 PM

#

Oh, so am i

#I don't get this comment: <https://