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 digest auth rest sensors #28153

Merged
merged 7 commits into from Nov 26, 2019

Conversation

@timmccor
Copy link
Contributor

timmccor commented Oct 23, 2019

Description:

Any REST sensor which is using DIGEST authentication will fail with the following exception stack:

2019-10-03 19:52:51 ERROR (MainThread) [homeassistant.components.sensor] rest: Error on device update!
Traceback (most recent call last):
  File "/usr/home/hass/env/lib/python3.7/site-packages/homeassistant/helpers/entity_platform.py", line 292, in _async_add_entity
    await entity.async_device_update(warning=False)
  File "/usr/home/hass/env/lib/python3.7/site-packages/homeassistant/helpers/entity.py", line 441, in async_device_update
    await self.hass.async_add_executor_job(self.update)
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/usr/home/hass/env/lib/python3.7/site-packages/homeassistant/components/rest/sensor.py", line 175, in update
    self.rest.update()
  File "/usr/home/hass/env/lib/python3.7/site-packages/homeassistant/components/rest/sensor.py", line 226, in update
    self._request, timeout=self._timeout, verify=self._verify_ssl
  File "/usr/home/hass/env/lib/python3.7/site-packages/requests/sessions.py", line 653, in send
    r = dispatch_hook('response', hooks, r, **kwargs)
  File "/usr/home/hass/env/lib/python3.7/site-packages/requests/hooks.py", line 31, in dispatch_hook
    _hook_data = hook(hook_data, **kwargs)
  File "/usr/home/hass/env/lib/python3.7/site-packages/requests/auth.py", line 247, in handle_401
    if self._thread_local.pos is not None:
AttributeError: '_thread._local' object has no attribute 'pos'

Fundamentally this is because the requests library stores a bunch of important information in thread local storage when the request is prepared, and this is needed when the thread is later used.

To fix this I've delayed preparing until we actually use the request - this does have the impact that we now prepare before every send rather than only once, but that doesn't appear to have much of a cost looking through the requests library and avoids this nasty issue.

Related issue (if applicable): fixes (closed aged issue) #13524

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the [development checklist][dev-checklist]
timmccor added 3 commits Oct 24, 2019
Don't rename the data variable, appears several other sensors have been coded to rely on it
@timmccor

This comment has been minimized.

Copy link
Contributor Author

timmccor commented Oct 25, 2019

@balloob I'm going crazy looking at these tests...

They seem to be failing as a method of 'None' is passed to request(). This method came straight from the config, which should be setting a default of 'GET' - so I'm at a loss as to how this could have happened. Looking at the test output it seems that none of the default config which should be there is there for the binary sensor. But no failures from the 'normal' sensor...

RestData is a class within sensor, but binary_sensor refers to it. And both sensor and binary_sensor call it in exactly the same way with exactly the same config as far as I can see. So why does binary_sensor fail?!

I must have missed something obvious. Can you spot it?

@timmccor timmccor requested a review from balloob Oct 25, 2019
timmccor added 2 commits Nov 9, 2019
Fix tests
test_setup_missing_schema:
Change the exception to check for to PlatformNotReady. With the change away from prepared statements, we no longer get the MissingSchema error during setup - we get it when we attempt to call the endpoint. Since the result is PlatformNotReady, which is what we want, and the error log clearly contains a note that the schema is incorrect I think this is fine.

test_setup_failed_connect & test_setup_timeout:
These aren't checking for minimum config, and they're not invoking the correct method to have the default config filled in. Therefore I've just added the correct minimum config so these can continue to test what they're there to test.

test_update_request_exception:
Testing a request exception with the assert on line 404! Excellent. Anyway, we've moved from using the requests Session object to the requests.request function - so the patch needed to be altered to ensure the RequestException was raised.
@timmccor

This comment has been minimized.

Copy link
Contributor Author

timmccor commented Nov 9, 2019

Failing tests are in another component - suggest this is ready to merge @balloob

@timmccor timmccor force-pushed the timmccor:fix-rest-digest-auth-sensors branch 2 times, most recently from a898555 to 715c70b Nov 10, 2019
@home-assistant home-assistant deleted a comment from homeassistant Nov 10, 2019
@home-assistant home-assistant deleted a comment from homeassistant Nov 10, 2019
@balloob balloob merged commit 6c6a5c5 into home-assistant:dev Nov 26, 2019
10 checks passed
10 checks passed
CI Build #20191110.14 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
docs-missing Documentation ok.
Dev automation moved this from Needs review to Done Nov 26, 2019
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Nov 26, 2019

Bueno 🎉

@timmccor timmccor deleted the timmccor:fix-rest-digest-auth-sensors branch Nov 27, 2019
@lock lock bot locked and limited conversation to collaborators Nov 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
4 participants
You can’t perform that action at this time.