New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove usage of "run_until_complete" #16617
Conversation
homeassistant/helpers/signal.py
Outdated
@@ -17,7 +17,9 @@ def async_register_signal_handling(hass: HomeAssistant) -> None: | |||
if sys.platform != 'win32': | |||
@callback | |||
def async_signal_handle(exit_code): | |||
"""Wrap signal handling.""" | |||
"""Wrap signal handling: queue shutdown code, re-instate default handler.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (88 > 79 characters)
homeassistant/core.py
Outdated
|
||
await self.async_start() | ||
if attach_signals: | ||
from homeassistant.helpers.signal import async_register_signal_handling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (83 > 79 characters)
* Use asyncio.run (or our own implementation on Python <3.7) * hass.start is only used by tests * setup_and_run_hass() is now async * Add "main" async hass.run method * move SIGINT handling to helpers/signal.py * add flag to .run to disable hass's signal handlers * Teach async_start and async_stop to not step on each other (more than necessary)
homeassistant/__main__.py
Outdated
@@ -21,8 +21,6 @@ | |||
|
|||
def attempt_use_uvloop() -> None: | |||
"""Attempt to use uvloop.""" | |||
import asyncio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the import is missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh. Me being blind. Will fix.
homeassistant/core.py
Outdated
if self.state == CoreState.not_running: # just ignore | ||
return | ||
if self.state == CoreState.stopping: | ||
_LOGGER.info("async_stop called twice: ignored") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a better idea. Will change.
homeassistant/util/__init__.py
Outdated
@@ -30,6 +30,15 @@ | |||
} | |||
|
|||
|
|||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to util.async_
homeassistant/helpers/signal.py
Outdated
* queue shutdown code | ||
* re-instate default handler | ||
""" | ||
hass.loop.remove_signal_handler(signal.SIGTERM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we do this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the signal handler has initiated an orderly shutdown. Its job is now done; if we get the signal again, the user is impatient / the main loop is stuck, and standard Python signal handling should take over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check
homeassistant/util/async_.py
Outdated
asyncio_run = asyncio.run | ||
except AttributeError: | ||
def asyncio_run(main, *, debug=False): | ||
loop = asyncio.new_event_loop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'asyncio'
homeassistant/util/async_.py
Outdated
@@ -12,6 +12,15 @@ | |||
_LOGGER = logging.getLogger(__name__) | |||
|
|||
|
|||
try: | |||
asyncio_run = asyncio.run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined name 'asyncio'
homeassistant/core.py
Outdated
fire_coroutine_threadsafe(self.async_stop(), self.loop) | ||
|
||
async def async_stop(self, exit_code: int = 0) -> None: | ||
async def async_stop(self, exit_code: int = 0, *, force: bool = False) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line too long (83 > 79 characters)
only useful for testing
Required for keeping requirements_test_all.txt in sync, where it is in turn required to prevent auto-downgrading "attrs" during "pip install"
homeassistant/util/async_.py
Outdated
asyncio_run = asyncio.run # type: ignore | ||
except AttributeError: | ||
_T = TypeVar('_T') | ||
def asyncio_run(main: Awaitable[_T], *, debug: bool = False) -> _T: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected 1 blank line, found 0
Current tests break with undefined "mock_signal" fixture. |
Meh. Finally. (Had a broken local installation.) |
homeassistant/core.py
Outdated
finally: | ||
self.loop.close() | ||
return self.exit_code | ||
|
||
async def run(self, *, attach_signals: bool = True) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is an async method, we should call it async_run
to be in line with the rest of our functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My rationale for not naming it async_run
is that it's never called from within Home Assistant, but I'll change it if you want me to, no problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's change it.
requirements_test.txt
Outdated
@@ -2,6 +2,7 @@ | |||
# make new things fail. Manually update these pins when pulling in a | |||
# new version | |||
asynctest==0.12.2 | |||
attrs==18.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be necessary, it's already part of the core dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem: when I do a pip install -r requirements_test_all.txt
without this entry, attrs
is downgraded to some lower version, probably because of some sub-module's requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the constraints file like Tox does: pip install -r requirements_test_all.txt -c homeassistant/package_constraints.txt
solved by using "pip -c"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok to merge after run renamed to async_run
All done, please merge. |
Did it tested on Windows? I run on Windows Python 3.6.3 got some errors
|
This logic should be moved to main to fix it: |
This should do it: #16737 |
Description:
This patch changes Home Assistant's startup routine so that it will run under
asyncio.run
. It also changes the SIGINT handler to start the shutdown sequence.Advantages:
Disadvantages:
Checklist:
tox
. Your PR cannot be merged unless tests pass