Skip to content

Fix modbus blocking threads#50619

Merged
frenck merged 7 commits intohome-assistant:devfrom
janiversen:modbus_sync_2
May 15, 2021
Merged

Fix modbus blocking threads#50619
frenck merged 7 commits intohome-assistant:devfrom
janiversen:modbus_sync_2

Conversation

@janiversen
Copy link
Copy Markdown
Member

Breaking change

Proposed change

Since long before 2021.3 (as many think is working perfectly) the modbus integration have had a problem that caused threads to be blocked when a modbus device had problems. Pymodbus 2.30 was very "relaxed" and ignored quite a lot, so the problem was mostly ignored. I heard about it a couple of times, which caused a try to run pymodbus async. last year, with little success due to big flaws in pymodbus 2.30.

Pymodbus 2.5.1 (introduced in 2021.5) detected a lot more, and thus HA experienced that a lot more threads got blocked. A big thanks to @MartinHjelmare for pointing at the threading Lock() as a potential problem.

This PR changes the whole modbus integration to be async, however pymodbus is still used synchronously and thus there a switch from async to sync.

There is added a new test "test_thread_lock", that proves the problem:

  • with threading Lock() 3 sensors and firing time events 5 times hangs.
  • with Asyncio Lock() 200 sensors and firing time events 10 times works as expected.

This PR do not stand alone, if there are modbus communication problems they should be identified and solved,
to make that possible 2 new configuration variables are available:

modbus:
    ...
    delay: <seconds, default 0>
    timeout: <seconds, default 10>
    close_comm_on_error: <true/false, default true>

"delay" is used solely to postpone sending requests after connect.
"timeout" asks pymodbus to wait for a response, if a response comes earlier waiting is stopped
"close_comm_on_error" ask pymodbus to keep connections open even in case of errors.

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

Please add this PR (after review) to 2021.6

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:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @adamchengtkc, @vzahradnik, mind taking a look at this pull request as its been labeled with an integration (modbus) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@janiversen janiversen changed the title Solve modbus causing lovelace to be unresponsive. Solve modbus bug causing lovelace to be unresponsive. May 14, 2021
@janiversen
Copy link
Copy Markdown
Member Author

I really like the expression GitHub uses "degraded performance for GitHub Actions." see:
https://www.githubstatus.com

In future PR´s I will use "degraded performance" instead of problems or bugs.

In other words, I will try again later.

Make single sync function to do pymodbus calls.
Add async version of pymodbus calls.
Convert platform to async.
Update/simplify test_delay.
Change Lock() from threading. to asyncio.
Add thread block test.
@MartinHjelmare MartinHjelmare changed the title Solve modbus bug causing lovelace to be unresponsive. Fix modbus blocking threads May 14, 2021
Comment thread homeassistant/components/modbus/binary_sensor.py Outdated
Comment thread homeassistant/components/modbus/modbus.py Outdated
Comment thread homeassistant/components/modbus/modbus.py Outdated
Comment thread tests/components/modbus/test_init.py
@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented May 14, 2021

We also may want to set the PARALLEL_UPDATES constant to 1 in the platforms. This will make sure we don't try to run the service calls in parallel for entities within a platform.

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

If a user would set up many different hubs with many entities for a platform, trying to run service calls in parallel would consume many thread pool threads.

@terminet85
Copy link
Copy Markdown
Contributor

That's fix my issue #50610

@HarrisonPace
Copy link
Copy Markdown
Contributor

Tested this PR, works as expected for both Serial Modbus RTU and TCP Modbus over IP. Maybe a Placebo but it feels like the GUI is more responsive, awesome work @janiversen adding asynchrony to this component.

@janiversen
Copy link
Copy Markdown
Member Author

Added PARALLEL_UPDATES = 1, that should be the last of the requested changes.

Comment thread homeassistant/components/modbus/modbus.py Outdated
Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Comment thread homeassistant/components/modbus/modbus.py Outdated
Comment thread homeassistant/components/modbus/modbus.py
Comment thread homeassistant/components/modbus/modbus.py Outdated
Comment thread homeassistant/components/modbus/modbus.py Outdated
Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@frenck frenck merged commit ad7be91 into home-assistant:dev May 15, 2021
@janiversen janiversen deleted the modbus_sync_2 branch May 15, 2021 17:59
@github-actions github-actions Bot locked and limited conversation to collaborators May 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.