Skip to content
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 cast doing I/O in event loop #12632

Merged
merged 1 commit into from Feb 23, 2018

Conversation

Projects
None yet
3 participants
@OttoWinter
Copy link
Member

commented Feb 23, 2018

Description:

Follow-up on #12474.

We're doing I/O inside async_setup_platform, because the pychromecast.Chromecast constructor calls some http endpoint on the chromecast. We shouldn't do that. I've now looked through the code again, and have not found another instance of this.

Thanks to @rytilahti for pointing this out!

Though this should fix the issue, we could also additionally wrap the whole call in a timeout, to make it even more secure: (Doesn't work since pychromecast.Chromecast isn't a coroutine)

with async_timeout.timeout(30, loop=hass.loop):
    chromecast = yield from hass.async_add_job(
        pychromecast.Chromecast, *want_host)

Example entry for configuration.yaml (if applicable):

media_player:
  - platform: cast

Checklist:

  • The code change is tested and works locally.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works. (Dunno how we can check whether a mock object was called from within the event loop)

@OttoWinter OttoWinter added this to the 0.64.0 milestone Feb 23, 2018

@balloob balloob merged commit c076b80 into home-assistant:dev Feb 23, 2018

5 checks passed

WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 94.077%
Details
hound No violations found. Woof!
@balloob

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

Ugh for constructors doing work

balloob added a commit that referenced this pull request Feb 25, 2018

@balloob balloob referenced this pull request Feb 25, 2018

Merged

0.64.0 #12609

@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.