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

Fix Windows loop #16737

Merged
merged 3 commits into from Sep 22, 2018

Conversation

Projects
None yet
5 participants
@balloob
Member

balloob commented Sep 20, 2018

Description:

Fix the event loop under Windows after #16617.

@balloob balloob requested a review from home-assistant/core as a code owner Sep 20, 2018

@wafflebot wafflebot bot added the in progress label Sep 20, 2018

@balloob balloob referenced this pull request Sep 20, 2018

Merged

Remove usage of "run_until_complete" #16617

2 of 2 tasks complete

@balloob balloob requested a review from awarecan Sep 20, 2018

@awarecan

This comment has been minimized.

Contributor

awarecan commented Sep 20, 2018

HA can start up in Windows Python 3.6.3 now.

However press Ctrl+C cannot stop process, have to kill it through task manager.
First Ctrl+C will output following

Traceback (most recent call last):
  File "c:\awarecan\home-assistant\homeassistant\__main__.py", line 380, in <module>
    sys.exit(main())
  File "c:\awarecan\home-assistant\homeassistant\__main__.py", line 372, in main
    exit_code = asyncio_run(setup_and_run_hass(config_dir, args))
  File "c:\awarecan\home-assistant\homeassistant\util\async_.py", line 29, in asyncio_run
    return loop.run_until_complete(main)
  File "C:\Python\Python36\lib\asyncio\base_events.py", line 454, in run_until_complete
    self.run_forever()
  File "C:\Python\Python36\lib\asyncio\base_events.py", line 421, in run_forever
    self._run_once()
  File "C:\Python\Python36\lib\asyncio\base_events.py", line 1390, in _run_once
    event_list = self._selector.select(timeout)
  File "C:\Python\Python36\lib\selectors.py", line 323, in select
    r, w, _ = self._select(self._readers, self._writers, [], timeout)
  File "C:\Python\Python36\lib\selectors.py", line 314, in _select
    r, w, x = select.select(r, w, w, timeout)
KeyboardInterrupt

The following Ctrl+C didn't got any response

After kill through task manager, get another stack trace output

Traceback (most recent call last):
  File "C:\Python\Python36\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "C:\Python\Python36\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "c:\awarecan\home-assistant\homeassistant\__main__.py", line 380, in <module>
    sys.exit(main())
  File "c:\awarecan\home-assistant\homeassistant\__main__.py", line 372, in main
    exit_code = asyncio_run(setup_and_run_hass(config_dir, args))
  File "c:\awarecan\home-assistant\homeassistant\util\async_.py", line 29, in asyncio_run
    return loop.run_until_complete(main)
  File "C:\Python\Python36\lib\asyncio\base_events.py", line 454, in run_until_complete
    self.run_forever()
  File "C:\Python\Python36\lib\asyncio\base_events.py", line 421, in run_forever
    self._run_once()
  File "C:\Python\Python36\lib\asyncio\base_events.py", line 1426, in _run_once
    handle._run()
  File "C:\Python\Python36\lib\asyncio\events.py", line 127, in _run
    self._callback(*self._args)
  File "c:\awarecan\home-assistant\homeassistant\__main__.py", line 256, in setup_and_run_hass
    subprocess.check_call(nt_args)
  File "C:\Python\Python36\lib\subprocess.py", line 286, in check_call
    retcode = call(*popenargs, **kwargs)
  File "C:\Python\Python36\lib\subprocess.py", line 269, in call
    return p.wait(timeout=timeout)
  File "C:\Python\Python36\lib\subprocess.py", line 1055, in wait
    timeout_millis)
KeyboardInterrupt
except ImportError:
pass
else:
asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())

This comment has been minimized.

@pvizeli

pvizeli Sep 20, 2018

Member

Could be simple:

try:
   import uvloop
   asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())
except ..:
   pass

This comment has been minimized.

@smurfix

smurfix Sep 21, 2018

Contributor

Yes it's simpler, but the part that's supposed to raise an ImportError is the actual import. I don't want to guard the policy call because if uvloop has an internal problem which happens to raise an ImportError the system may be left in an inconsistent state and the error won't be reported.

All of this is probably not relevant in this particular case, but I got into the habit of following this pattern as a matter of principle because it's good coding practice and I did hit that set of circumstances often enough to be wary of it.

@balloob

This comment has been minimized.

Member

balloob commented Sep 20, 2018

@smurfix

This comment has been minimized.

Contributor

smurfix commented Sep 21, 2018

See

# Run a simple daemon runner process on Windows to handle restarts

if os.name == 'nt' and '--runner' not in sys.argv:

Apparently this runner does no longer get a subprocess.CalledProcessError when hass is killed, probably because it's now in the async part of the code, where it doesn't belong.

Could you check whether the latest change in https://github.com/M-o-a-T/home-assistant/tree/aio_run (git remote add smurfix https://github.com/M-o-a-T/home-assistant; git fetch smurfix; git cherry-pick 02a76f228d58) fixes this problem?

@awarecan

This comment has been minimized.

Contributor

awarecan commented Sep 21, 2018

No change, same error. Even I comment out the whole --runner section, I still cannot use Ctrl+C to shutdown the process, have to kill it by task manager

@smurfix

This comment has been minimized.

Contributor

smurfix commented Sep 21, 2018

Hmm, the other problem seems to be that the signal handler isn't even registered, sorry that I missed that.

See homeassistant/helpers/signal.py -- please change that so that it reads

@callback
@bind_hass
def async_register_signal_handling(hass: HomeAssistant) -> None:
    """Register system signal handler for core."""
    @callback
    def async_signal_handle(exit_code):
        """Wrap signal handling.

        * queue call to shutdown task
        * re-instate default handler
        """
        hass.loop.remove_signal_handler(signal.SIGTERM)
        hass.loop.remove_signal_handler(signal.SIGINT)
        hass.async_create_task(hass.async_stop(exit_code))

    if sys.platform != 'win32':
        try:
            hass.loop.add_signal_handler(
                signal.SIGHUP, async_signal_handle, RESTART_EXIT_CODE)
        except ValueError:
            _LOGGER.warning("Could not bind to SIGHUP")

    try:
        hass.loop.add_signal_handler(
            signal.SIGTERM, async_signal_handle, 0)
    except ValueError:
        _LOGGER.warning("Could not bind to SIGTERM")
    
    try:
        hass.loop.add_signal_handler(
            signal.SIGINT, async_signal_handle, 0)
    except ValueError:
        _LOGGER.warning("Could not bind to SIGINT")

i.e. the SIGINT and SIGTERM parts are no longer guarded by the != win32.

@awarecan

This comment has been minimized.

Contributor

awarecan commented Sep 21, 2018

add_signal_handler() is Unix only

@smurfix

This comment has been minimized.

Contributor

smurfix commented Sep 21, 2018

OK. Use signal.signal then. Sorry about me not having any Python-on-Windows experience …

    if sys.platform != 'win32':
        … original code …
    else:
        old_sigiterm = None
        old_sigint = None

        @callback
        def async_signal_handle(exit_code, frame):
            """Wrap signal handling.

            * queue call to shutdown task
            * re-instate default handler
            """
            signal.signal(signal.SIGTERM, old_sigterm)
            signal.signal(signal.SIGINT, old_sigint)
            hass.async_create_task(hass.async_stop(exit_code))

        try:
            old_sigterm = signal.signal(signal.SIGTERM, async_signal_handle)
        except ValueError:
            _LOGGER.warning("Could not bind to SIGTERM")

        try:
            old_sigint = signal.signal(signal.SIGINT, async_signal_handle)
        except ValueError:
            _LOGGER.warning("Could not bind to SIGINT")
@awarecan

This comment has been minimized.

Contributor

awarecan commented Sep 21, 2018

Looks good this time. I can clean up the change and make a push later

@smurfix

This comment has been minimized.

Contributor

smurfix commented Sep 21, 2018

Phew. ;-) Thanks.

@awarecan

This comment has been minimized.

Contributor

awarecan commented Sep 21, 2018

hass can start and stop on Windows now. but I cannot connect to http://localhost:8123. Investigating...

EDIT: false alarm, I forget I had forward my localhost 8123 port to my dev VM. LOL

@smurfix

This comment has been minimized.

Contributor

smurfix commented Sep 21, 2018

Could you please pull from https://github.com/M-o-a-T/home-assistant/tree/winfix (and push that onto this pull request)?

This change adds the change from comment #16737 to this pull request. Having that code in the async part blocks one of the original goals of moving towards asyncio.run (composeability).

@awarecan

This comment has been minimized.

Contributor

awarecan commented Sep 21, 2018

Restart tested on Windows

@balloob balloob merged commit 5613816 into dev Sep 22, 2018

6 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls First build on fix-windows at 93.559%
Details

@wafflebot wafflebot bot removed the in progress label Sep 22, 2018

@balloob

This comment has been minimized.

Member

balloob commented Sep 22, 2018

Great work everyone

@balloob balloob referenced this pull request Sep 28, 2018

Merged

0.79.0 #16940

@balloob balloob referenced this pull request Oct 2, 2018

Merged

0.79.3 #17067

@Danielhiversen Danielhiversen deleted the fix-windows branch Oct 2, 2018

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