Skip to content

Conversation

@allenporter
Copy link
Contributor

@allenporter allenporter commented Jun 15, 2023

Proposed change

Update the return signature of service calls:

  • The limit value is removed, and there is no longer special timeout handling
  • The return value has been removed

Previously this would return True on success and False on timeout with a default limit of 10 seconds. Instead, we can handle any timeouts needed in the caller based on the context where they are called from (e.g. script can use an asyncio timeout if it wants a timeout).

Script service call limit has been removed. We can consider adding this back in the future with an explicit limit in the script. The main point is we can reduce a lot of complexity now by not supporting this.

Intent service call timeouts were rewritten to attempt to preserve existing behavior of starting an async call, blocking for a short timeout, then proceeding.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@allenporter allenporter marked this pull request as draft June 15, 2023 14:05
@allenporter
Copy link
Contributor Author

It looks like the only thing that explicitly sets a limit for timeout is intents. It sets a short timeout of 200ms to attempt to pass validation but then doesn't want to block on the result.

@allenporter
Copy link
Contributor Author

Changing the timeout behavior now has these remaining failing tests:

FAILED tests/components/generic_thermostat/test_climate.py::test_heater_input_boolean - AssertionError: assert 'off' == 'on'
  - on
  + off
FAILED tests/components/google_assistant_sdk/test_init.py::test_send_text_command_expired_token_refresh_failure[failure_requires_reauth] - AssertionError: assert False == True
 +  where False = any(<generator object ConfigEntry.async_get_active_flows.<locals>.<genexpr> at 0x7f56a5843290>)
 +    where <generator object ConfigEntry.async_get_active_flows.<locals>.<genexpr> at 0x7f56a5843290> = <bound method ConfigEntry.async_get_active_flows of <tests.common.MockConfigEntry object at 0x7f569e34f840>>(<homeassistant.core.HomeAssistant object at 0x7f56a16362c0>, {'reauth'})
 +      where <bound method ConfigEntry.async_get_active_flows of <tests.common.MockConfigEntry object at 0x7f569e34f840>> = <tests.common.MockConfigEntry object at 0x7f569e34f840>.async_get_active_flows
FAILED tests/components/google_mail/test_services.py::test_reauth_trigger - assert 0 == 1
FAILED tests/components/profiler/test_init.py::test_log_scheduled - AssertionError: assert 'Scheduled' in '2023-06-15 15:07:11.610 INFO     SyncWorker_0 homeassistant.loader:loader.py:638 Loaded profiler from homeassistant.c...:core.py:1032 Bus:Handling <Event call_service[L]: domain=profiler, service=log_event_loop_scheduled, service_data=>\n'

@balloob
Copy link
Member

balloob commented Jun 16, 2023

The reason why awaiting the coro directly in ServiceRegistry.async_call is causing new test failures is because a bunch of integrations use dispatcher to send signals, and those are not awaitable (by design), yet their tests assumed they were done running. Fixing up the last few now.

Copy link
Contributor Author

@allenporter allenporter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed your changes and they look good.

@allenporter allenporter marked this pull request as ready for review June 16, 2023 03:54
@allenporter allenporter requested review from a team and synesthesiam as code owners June 16, 2023 03:54
@MartinHjelmare
Copy link
Member

When we're finished with all the changes around services we should publish a developer blog.

@allenporter
Copy link
Contributor Author

allenporter commented Jun 16, 2023

When we're finished with all the changes around services we should publish a developer blog.

I have home-assistant/developers.home-assistant#1799 (linked from this PR) so we don't forget and we can keep adding to it with the other upcoming service response data changes.

@allenporter allenporter merged commit 12129e9 into home-assistant:dev Jun 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jun 17, 2023
if limit is not None:
# There is a call limit, so just wait for it to finish.
await service_task
try:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This timeout disappeared and i can't tell where it went or why we did this looking at the PR and commit history. Is the lack of timeout here responsible for #98073 ? or are we expecting a timeout somewhere else?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants