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

Issue 6749 updated pylgtv to 0.1.6 to fix thread leak in asyncio loop #7199

Merged
merged 8 commits into from
Apr 22, 2017
Merged

Conversation

hmn
Copy link
Contributor

@hmn hmn commented Apr 21, 2017

Description:

updated pylgtv to 0.1.6 to fix thread leak in asyncio loop

Related issue (if applicable): fixes #6749

Checklist:

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.

hmn added 3 commits April 19, 2017 19:59
* dev:
  mvglive bug fixes and improvements (#6953)
  Do not request artwork if not available (#7189)
  Fix auto discovery for Apple TV (#7188)
  Fix for errors on missing preview on LG webos TV (#6755)
  Added light.pwm component. (#7009)
  Add ping binary sensor (#7052)
  opensky sensor (#7061)
  JSON MQTT Device tracker (#7055)
  spotify media player (#6980)
  Update frontend
  Add Bose soundtouch discovery support and upgrade libsoundtouch library (#7005)
  Fix wemo discovery (#7183)
  updated pylgtv module to fix problems with timeouts (#7184)
  Mqtt camera test (#7175)
  Update frontend
- handle new TimeoutError exception from pylgtv
@mention-bot
Copy link

@hmn, thanks for your PR! By analyzing the history of the files in this pull request, we identified @armills, @TheRealLink and @andersonshatch to be potential reviewers.

hmn added 2 commits April 21, 2017 14:09
* dev: (37 commits)
  HassIO API v2 (#7201)
  LIFX light effects (#7145)
  Add HassIO to discovery component (#7195)
  Add support of input registers while querying modbus sensor. (#7082)
  Upgrade py-cpuinfo to 3.2.0 (#7190)
  Fix for zwave RGB setting (#7137)
  Make version number optional and a string to fix identify issue introduced in iOS 1.0.1 (#7141)
  Upgrade aiohttp to 2.0.7 (#7106)
  Fix mysensors callback (#7057)
  Version bump to 0.42.4
  Bugfix slider (#7047)
  Bugfix wait on start event (#7013)
  Plug file leak on LIFX unregister (#7031)
  Fix US states check (fixes #7015) (#7017)
  Bump pyalarmdotcom to support new version of aiohttp (#7021)
  Fix two more instances of JSON parsing synology (#7014)
  Fix Synology camera content type (#7010)
  Version bump to 0.42.3
  Downgrade aiohttp to 205 (#6994)
  version bump to 0.42.2
  ...
@@ -257,7 +258,7 @@ def turn_off(self):
self._state = STATE_OFF
try:
self._client.power_off()
except (OSError, ConnectionClosed):
except (OSError, ConnectionClosed, concurrent.futures._base.TimeoutError):

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

@@ -196,7 +197,7 @@ def update(self):
app = self._app_list[source['appId']]
self._source_list[app['title']] = app

except (OSError, ConnectionClosed):
except (OSError, ConnectionClosed, concurrent.futures._base.TimeoutError):

Choose a reason for hiding this comment

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

line too long (82 > 79 characters)

@@ -99,7 +100,7 @@ def setup_tv(host, mac, name, customize, config, hass, add_devices):
_LOGGER.warning(
"Connected to LG webOS TV %s but not paired", host)
return
except (OSError, ConnectionClosed):
except (OSError, ConnectionClosed, concurrent.futures._base.TimeoutError):

Choose a reason for hiding this comment

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

line too long (86 > 79 characters)

emlove
emlove previously requested changes Apr 21, 2017
@@ -87,6 +87,7 @@ def setup_tv(host, mac, name, customize, config, hass, add_devices):
from pylgtv import WebOsClient
from pylgtv import PyLGTVPairException
from websockets.exceptions import ConnectionClosed
from concurrent import futures
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, import asyncio at the top of the module, and use asyncio.TimeoutError. It's an alias for concurrent.futures.TimeoutError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok thanks, I had a bit a problem figuring out what the right way to do this was

@@ -217,7 +221,7 @@ def is_volume_muted(self):
@property
def volume_level(self):
"""Volume level of the media player (0..1)."""
return self._volume / 100.0
return float(self._volume) / 100.0
Copy link
Contributor

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. Dividing by a float will perform the conversion automatically.

@hmn
Copy link
Contributor Author

hmn commented Apr 21, 2017

the requested changes should be done now

sorry for the messy pull request i'll do better next time :)

@balloob balloob added this to the 0.43 milestone Apr 22, 2017
@balloob balloob dismissed emlove’s stale review April 22, 2017 03:24

Concerns addressed

@balloob balloob merged commit 8e71678 into home-assistant:dev Apr 22, 2017
@balloob
Copy link
Member

balloob commented Apr 22, 2017

Cherry-picked into 0.43

balloob pushed a commit that referenced this pull request Apr 22, 2017
…#7199)

* updated pylgtv module to fix problems with timeouts

* - update pylgtv to 0.1.6
- handle new TimeoutError exception from pylgtv

* used full name for exception handling of concurrent.futures._base.TimeoutError

* the exception handling should now follow the rules

* float typecasting should not be necessary

* use asyncio for TimeoutError it’s an alias for concurrent.futures.TimeoutError
@balloob balloob mentioned this pull request Apr 22, 2017
@hmn hmn deleted the issue-6749 branch April 22, 2017 08:06
@emlove
Copy link
Contributor

emlove commented Apr 22, 2017

It's no problem! The stacktrace prints the reference to concurrent.futures, so you basically just have to know the alias is there in asyncio. We appreciate the contribution!

@balloob balloob mentioned this pull request May 5, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Aug 12, 2017
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.

webostv keeps files open until file limit is reached
7 participants