search

tchellomello / python-amcrest

A Python 2.7/3.x module for Amcrest and Dahua Cameras using the SDK HTTP API.
GNU General Public License v2.0
216 stars 76 forks source link

Reuse command sessions #104

Closed pnbruckner closed 5 years ago

pnbruckner commented 5 years ago

In command method, don't create a new requests session every time. Rather, keep a session for each value of retries. Also don't try to update default values for retries and timeout for each command because that wasn't thread safe.

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-0.1%) to 32.829% when pulling 5196954861e1c2db190127c9f6302f9a247756fa on pnbruckner:reuse-session into e30817ae7e7c737aad7050945586e069fae0741b on tchellomello:master.

pnbruckner commented 5 years ago

Just thought of another optimization... I know Home Assistant uses this with just one value for retries, so in that case one session will be created and used from then on. But if other users call command with several values for retries over time, especially if they bounce back and forth between a set of values, the session would still get constantly recreated. So, rather than just keeping the latest one around, use a dictionary to save a session per value. This way, once all the various values of retries are used, it will never need to create a new session; it can just switch between the ones that exist. I'll implement that and make another commit soon.

pnbruckner commented 5 years ago

And now that I think about it some more, self._retries_conn and self._timeout_protocol are not thread safe. I'll work on that, too.

pnbruckner commented 5 years ago

This change is ready to go. @tchellomello, can this (and the previous PR) be released in a new version on pypi.org? I'm working on a PR for the amcrest component on HA, and I'd like to have it pull in these changes at the same time.

FYI, I do plan to make one more PR here to add some new commands in support of other enhancements I've made to the HA amcrest component that I will be submitting on other PRs to HA in the near future.

tchellomello commented 5 years ago

@pnbruckner Thanks for your contribution. I'll publish a new version on pypi.org shortly.

tchellomello commented 5 years ago

@pnbruckner version 1.2.4 is now published at https://pypi.org/project/amcrest/1.2.4/

Thanks!