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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Amcrest event monitor to non-async #69640

Merged
merged 1 commit into from May 4, 2022

Conversation

flacjacket
Copy link
Contributor

@flacjacket flacjacket commented Apr 8, 2022

Proposed change

Several problems have come up since using the async-based event monitor in the Amcrest component that stem from the differences between requests (which is used for synchronous http requests) and httpx (which is used for asynchronous http requests) and the workarounds that have been put in place to try to handle these differences. The underlying issue is that httpx will not drop a streaming connection when the resource is no longer available (eg the camera goes offline) when the read timeout is set to be infinite, while requests will drop such a connection. Adding a timeout to httpx leads to the issues linked below. httpx is the only http library I know of with async support for digest authentication, which is needed to communicate with Amcrest cameras. This change reverts back to using the non-async requests-based event stream method to get stable behavior back for the integration. I have tested this by running locally and ensuring the event handling works correctly and does not cause any of the error messages that have been reported.

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

Additional information

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:

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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • 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:

@joe248
Copy link
Contributor

joe248 commented Apr 9, 2022

I'm not sure if this was also intended to address #68271, but I've been running this change on 2022.3.5 for about a day and so far have not experienced the issue I was having in that ticket, so this seems to have fixed it.

@flacjacket flacjacket marked this pull request as ready for review April 10, 2022 12:13
@flacjacket
Copy link
Contributor Author

I'm not sure if this was also intended to address #68271, but I've been running this change on 2022.3.5 for about a day and so far have not experienced the issue I was having in that ticket, so this seems to have fixed it.

I am able to still reproduce that error with this change in place. The best way to do that is to unplug the camera while moving it to trigger the motion detection. When the camera is plugged back in and comes back online, it will still be in the motion detected state.

@joe248
Copy link
Contributor

joe248 commented Apr 10, 2022

Ah, okay. I didn't test that case. I was just referring to the issue I was seeing - where the camera was not rebooting but motion detection would get stuck in the on state.

@flacjacket
Copy link
Contributor Author

Ah, yep, this should help cover that case! I think that issue is related to the reconnecting problems that exist currently, which is what this change is addressing.

@KonTiki1957
Copy link

Hi to everyone, sorry for my ignorance but having the same issue on my dahua cameras (5, all configured in yaml) I was wondering if there is something that needs to be done on my part to solve the problem or if there will be a new home assistant release which will take care of everything. Many thanks

@marcelliotnet

This comment was marked as off-topic.

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Apr 18, 2022
@GaryOkie

This comment was marked as off-topic.

@Zoriontsu

This comment was marked as off-topic.

@bdraco
Copy link
Member

bdraco commented Apr 26, 2022

It doesn't look like an issue has been opened with httpx on this problem, or at least I couldn't find one. It would be better to try to get the issues solved in httpx as switching back to sync really isn't the best solution long term.

@GaryOkie

This comment was marked as off-topic.

@KonTiki1957

This comment was marked as off-topic.

@redwngsrul

This comment was marked as off-topic.

@bdraco
Copy link
Member

bdraco commented May 4, 2022

Please move this discussion to the forum as its not discussing the code in this PR.

The path forward will be decided when the OP responds to the comment I made above. As there has been no response I can't predict what the outcome of this PR will be.

@flacjacket
Copy link
Contributor Author

Sorry I missed that. I opened up a discussion at encode/httpx#2203 in the httpx project to get the functionality we need there to be able to effectively use that for streaming the event data. I would definitely want to switch back to async once we can figure out how to get httpx working properly, but with the current integration broken until that happens, it would be great if we could land this revent in the interim.

Dev automation moved this from By Code Owner to Reviewer approved May 4, 2022
@bdraco bdraco added this to the 2022.5.0 milestone May 4, 2022
@bdraco bdraco merged commit 08770d0 into home-assistant:dev May 4, 2022
Dev automation moved this from Reviewer approved to Done May 4, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
by-code-owner cherry-picked cla-signed integration: amcrest smash Indicator this PR is close to finish for merging or closing
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

Amcrest reports 1000s of errors in log after update to 2022.3.5 Amcrest and Dahua Cameras become unavailable
10 participants