Skip to content

Bump screenlogicpy to 0.2.2#48427

Closed
dieselrabbit wants to merge 1 commit intohome-assistant:devfrom
dieselrabbit:screenlogic-lock
Closed

Bump screenlogicpy to 0.2.2#48427
dieselrabbit wants to merge 1 commit intohome-assistant:devfrom
dieselrabbit:screenlogic-lock

Conversation

@dieselrabbit
Copy link
Copy Markdown
Contributor

@dieselrabbit dieselrabbit commented Mar 28, 2021

API fixes shared socket collision when commands are called by multiple threads.

Proposed change

Bump screenlogicpy to 0.2.2.
API library adds threading locks to fix shared socket collisions when commands are called by multiple threads. ie. multiple switches are listed in a single call to switch.toggle/turn_on/turn_off
Library diff here: dieselrabbit/screenlogicpy@630dc33

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

  • This PR fixes or closes issue: fixes #
  • 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:

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:

API fixes shared socket collision when commands are called by multiple threads.
@probot-home-assistant probot-home-assistant Bot added bugfix dependency Pull requests marked as a dependency upgrade integration: screenlogic small-pr PRs with less than 30 lines. dependency-bump Pull requests that update a dependency file by-code-owner labels Mar 28, 2021
@MartinHjelmare MartinHjelmare changed the title Screenlogic: Bump screenlogicpy to 0.2.2. Bump screenlogicpy to 0.2.2 Mar 28, 2021
@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Mar 28, 2021

We don't allow threading locks to be used with our thread pool. If synchronization is needed it should be done with asyncio. Otherwise the thread pool risks exhaustion.

We offer a convenience setting for this within the same platform exposed via the constant PARALLEL_UPDATES. It will apply also for services called. If it's set to 1, only 1 service call will be done in sequence. The other calls will wait until the first call is done. As said, this synchronization only works within the same platform, eg within a switch platform for one integration.

https://developers.home-assistant.io/docs/integration_fetching_data/#request-parallelism

@bdraco
Copy link
Copy Markdown
Member

bdraco commented Mar 28, 2021

Maybe add the asyncio.Lock() to ScreenlogicDataUpdateCoordinator and every time you see a call to hass.async_add_executor_job wrap it with async with ...coordinator.data_lock:

@dieselrabbit
Copy link
Copy Markdown
Contributor Author

Interesting. While I think PARALLEL_UPDATES would solve the specific problem I was seeing, I think there's still a risk to other service calls impacting the currently shared socket ref in the library.

The asyncio.Lock() method seems that would be safer in terms of protection.

Thank you, both. I will close this PR and open a new one for changes in the integration.

@dieselrabbit
Copy link
Copy Markdown
Contributor Author

New PR: #48457

@github-actions github-actions Bot locked and limited conversation to collaborators Mar 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bugfix by-code-owner cla-signed dependency Pull requests marked as a dependency upgrade dependency-bump Pull requests that update a dependency file integration: screenlogic small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants