Shield async httpx call in generic by uvjustin · Pull Request #47852 · home-assistant/core

uvjustin · 2021-03-13T12:00:20Z

Proposed change

#46576 changed generic to use the httpx library. However, there are a few outstanding issues with httpx's async api (e.g. https://github.com/encode/httpx/issues/1461) which seem to be causing #47624. This PR uses asyncio.shield as a workaround for the httpx issue.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes No images in Lovelace from generic cameras. #47624
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

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

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

elupus · 2021-03-13T15:36:30Z

If cancellation is the problem. Can't we asyncio.shield a subtask from cancellation instead?

Seems better than switching to sync.

uvjustin · 2021-03-13T15:42:11Z

Good idea, we can try that and see if that fixes the issue instead.

uvjustin · 2021-03-14T04:01:54Z

The user reported that the current version (using asyncio.shield around the async api) works

elupus · 2021-03-14T11:32:23Z

+        return await asyncio.shield(self._async_camera_image())
+
+    async def _async_camera_image(self):
        """Return a still image response from the camera."""


I think it was better when you only shielded the httpx call. Anything inside the shield will continue to execute, but the surrounding code is cancelled. That mean that the setting of last image/url will still end up being set sometime later even though the command was cancelled.

So just shield the "buggy" code and let the rest be cancelled as expected.

That way, we avoid setting last url/image if the httpx stuff ends up completing even though caller wanted it cancelled.

The reason I was concerned about doing it that way was in the event of a cancellation event, the call would continue inside the shield but the response resources might not get freed up afterwards since the response is never read.
If we don't want to set the last_url/image I can still change that. I do think that does make more sense technically (the caller requested a cancel, so cancel), but practically it might make more sense to just update the last_url/image since we have it anyway.

@elupus Any thoughts on the above?

Well the cancel can occur on any await. So there is a risk of introducing a later await that changes behavior.

Being as specific as possible what it is we are solving is better. If cancelled we should throw away anything we get..a new request may have been started.

Ok, the change that was pushed last week does avoid setting the last_url/image. I agree we should only shield one await, but we also need to make sure the response gets closed, and this way satisfies both of those requirements.

@elupus Does what I said make sense? If we only shield the call then we risk not cleaning up the response.
It looks like another user had the same problem, so it would be good to merge a fix before another release.

Yes i understand.

elupus · 2021-03-30T08:14:54Z

+        return await asyncio.shield(self._async_camera_image())
+
+    async def _async_camera_image(self):
        """Return a still image response from the camera."""


Yes i understand.

elupus · 2021-03-30T08:20:45Z

+            return self._last_url, self._last_image
+        finally:
+            if response:
+                response.close()


Wait. Isn't close async here for an async httpx instance? Ie it should await response.aclose() also this is generally not needed unless it's a stream=true as mentioned here
https://www.python-httpx.org/async/#opening-and-closing-clients

You are right, I think it should be async close..I didn't change it when we switched back from the sync API.
Let me check into the close thing. I think when I was looking at the issues over on httpx there were other issues with not closing and freeing up resources...

I think issue 116/147 and pr 117 over there mean we should try to close it ourselves

If you don't have any objections, I'm going to go ahead and merge this as is. Hopefully the underlying httpx issue gets taken care of soon and we can roll back the shielding/closing code.
BTW, in #47624 someone noted that there may be similar issues with rest. It is possible that there are similar problems in other components that use httpx.

rpitera · 2021-03-31T14:49:49Z

@uvjustin - So far at 16 hours uptime, this has solved all my generic cam issues. But it also seems to have solved the REST call issues for the remaining four sensors I didn't move over to NodeRed; perhaps the correction has left more tasks open for the REST processing (since I pared it down to these four still running native in HA). Thanks for pointing me to it! I know it's been merged already but wanted to give you some more data and thank you. This was driving me crazy for months.

homeassistant added the cla-signed label Mar 13, 2021

probot-home-assistant Bot added bugfix integration: generic small-pr PRs with less than 30 lines. labels Mar 13, 2021

uvjustin marked this pull request as draft March 13, 2021 15:42

uvjustin changed the title ~~Use httpx sync api in generic~~ Shield async httpx call in generic Mar 13, 2021

Shield async httpx call

7b377d3

uvjustin force-pushed the troubleshoot-httpx-generic branch from 41d7dc3 to 7b377d3 Compare March 13, 2021 20:54

uvjustin marked this pull request as ready for review March 13, 2021 20:54

uvjustin commented Mar 14, 2021

View reviewed changes

Comment thread homeassistant/components/generic/camera.py Outdated

elupus reviewed Mar 14, 2021

View reviewed changes

uvjustin added 2 commits March 14, 2021 12:32

Don't set last_url/last_image on cancellation

642913e

Add test

1d90e71

elupus reviewed Mar 23, 2021

View reviewed changes

Comment thread homeassistant/components/generic/camera.py

uvjustin added 2 commits March 23, 2021 06:50

Re raise exception

7fe0e68

Pylint rename

1a8dcf9

elupus approved these changes Mar 30, 2021

View reviewed changes

elupus reviewed Mar 30, 2021

View reviewed changes

Use aclose instead of close

3c376b2

uvjustin mentioned this pull request Mar 30, 2021

No images in Lovelace from generic cameras. #47624

Closed

uvjustin merged commit 379843e into home-assistant:dev Mar 31, 2021

rpitera mentioned this pull request Mar 31, 2021

rest sensor unavailable #43600

Closed

uvjustin linked an issue Mar 31, 2021 that may be closed by this pull request

No images in Lovelace from generic cameras. #47624

Closed

github-actions Bot locked and limited conversation to collaborators Apr 1, 2021

uvjustin deleted the troubleshoot-httpx-generic branch November 7, 2021 11:44

Uh oh!

Conversation

uvjustin commented Mar 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

elupus commented Mar 13, 2021

Uh oh!

uvjustin commented Mar 13, 2021

Uh oh!

uvjustin commented Mar 14, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpitera commented Mar 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

uvjustin commented Mar 13, 2021 •

edited

Loading

Example entry for `configuration.yaml`: