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

Reuse command sessions #104

Merged
merged 4 commits into from
Mar 4, 2019
Merged

Conversation

pnbruckner
Copy link
Collaborator

@pnbruckner pnbruckner commented Mar 2, 2019

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.

src/amcrest/http.py Outdated Show resolved Hide resolved
src/amcrest/http.py Outdated Show resolved Hide resolved
src/amcrest/http.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 2, 2019

Coverage Status

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

@pnbruckner pnbruckner changed the title Reuse requests session in command if possible WIP Reuse requests session in command if possible Mar 2, 2019
@pnbruckner
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

Create a dictionary of sessions, with max_retries as key, so they can be reused once created. Also _retries_conn and _timeout_protocol usage was not thread safe; fix that.
Missed changing one occurence of self._retries_conn to retries in command method.
@pnbruckner pnbruckner changed the title WIP Reuse requests session in command if possible Reuse command sessions Mar 3, 2019
@pnbruckner
Copy link
Collaborator Author

pnbruckner commented Mar 3, 2019

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 tchellomello merged commit 0b3974b into tchellomello:master Mar 4, 2019
@tchellomello
Copy link
Owner

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

@tchellomello tchellomello mentioned this pull request Mar 4, 2019
@tchellomello
Copy link
Owner

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

Thanks!

@pnbruckner pnbruckner deleted the reuse-session branch March 4, 2019 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants