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 ONVIF Transport #35932

Merged
merged 2 commits into from
May 22, 2020
Merged

Conversation

hunterjm
Copy link
Member

@hunterjm hunterjm commented May 21, 2020

Proposed change

There is an issue that occurs on some devices with re-using the same AsyncTransport across multiple services which appears to occur with request queues on pending requests. The responses appear to be associated with the improper requests when they come back. This was exposed in 0.110.1 when we moved fetching URLs to async_added_to_hass. The library already handles creating a new AsyncTransport and ClientSession if no transport is passed in, so this will let it do so.

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

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@frenck
Copy link
Member

frenck commented May 21, 2020

@hunterjm This reverts my change in #32045

This transport was added in to deal with another bug... 🤷

@hunterjm
Copy link
Member Author

oyi... can you check it out after the refactor? Shutdown is graceful for my devices...

@frenck
Copy link
Member

frenck commented May 21, 2020

yes, I'll check it out locally and test 👍

@frenck frenck self-assigned this May 21, 2020
@hunterjm
Copy link
Member Author

I can also modify the upstream lib to allow passing in client session while still creating a new Transport class. Not sure if that will solve both issues though.

@hunterjm
Copy link
Member Author

I was able to reproduce @frenck:

/home/vscode/.local/lib/python3.8/site-packages/zeep/asyncio/transport.py:61: RuntimeWarning: coroutine 'noop2' was never awaited
  self.session.connector.close()
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

@frenck
Copy link
Member

frenck commented May 21, 2020

We already create a new Transport class for each camera... So that won't help.

@hunterjm
Copy link
Member Author

hunterjm commented May 21, 2020

I think the issue is a transport per service. There are multiple services (event/media/device) that all create their own transport in the parent lib if none is passed in: https://github.com/hunterjm/python-onvif-zeep-async/blob/async/onvif/client.py#L114-L119

@frenck
Copy link
Member

frenck commented May 21, 2020

The current stanza seems to be a single, unique transport for each camera. As transport is used from the ONVIFCamera instance:

image

If that causes the problem, we could instance a new transport in the above and pass in the session to those?

@hunterjm
Copy link
Member Author

yeah, testing something like that now... if I can get my dev environment to play nicely.

@hunterjm
Copy link
Member Author

That didn't fix it unfortunately. Apparently zeep thinks they are calling an underlying sync close, but it's not sync in the version of aiohttp HomeAssistant uses. https://github.com/mvantellingen/python-zeep/blob/master/src/zeep/asyncio/transport.py#L59-L61

@frenck
Copy link
Member

frenck commented May 21, 2020

Meh, indeed, upstream issue :(

@hunterjm
Copy link
Member Author

This change just got bigger, but I was able to handle session creation/closing in a new close method added to the first level upstream. It'd be good if you could validate again with your devices.

@hunterjm
Copy link
Member Author

Upstream change: openvideolibs/python-onvif-zeep-async@c457f73

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

For me, this works like a charm 👍
Tested with 3 different camera's, all not ONVIF certified.

@hunterjm hunterjm merged commit 0e83cfa into home-assistant:dev May 22, 2020
@hunterjm hunterjm deleted the bugfix/onvif-transport branch May 23, 2020 02:13
frenck pushed a commit that referenced this pull request May 24, 2020
* allow lib to create AsyncTransport

* fix transport close issue
@frenck frenck mentioned this pull request May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[onvif] Unable to setup a Hikvision DS-HD1 doorbell camera
5 participants