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

Keep track of TimerHandles independent from event loop #109022

Closed
wants to merge 4 commits into from

Conversation

jpbede
Copy link
Member

@jpbede jpbede commented Jan 28, 2024

Proposed change

Currently we're relaying on the event loop to keep track / get the scheduled TimerHandles. This could / is a problem if we consider switching to uvloop, besides the _scheduled property of the asyncio event loop is a protected property, so this may break anytime (what I don't expect to)

So this PR proposes to keep track of the TimeHandles, they need to be cancelled at shutdown, on our own.
I use a WeakSet here, so we don't have to remove the reference from the set ourselves after a TimeHandle has ended.

I tried also a different solution within uvloop (create a function to return the scheduled TimeHandlers), but lead me to other issues e.g. _args or args weren't available for the uvloop TimeHandlers.

I may have forgotten something, or it may not be the ideal solution, so I'll leave this here for comments.

With this solution the implementation of uvloop would be quite simple, the problem with <broadcast> described in #93173 seems to be solved. I have tried this with a small test and can send packets to the <broadcast> address without any problems

The <broadcast> issue within uvloop mentioned in #93173 should be solved by MagicStack/uvloop#592.

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)
  • Deprecation (breaking change to happen in the future)
  • 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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

return hass.loop.call_at(loop_time, _run_async_call_action, hass, job).cancel
handle = hass.loop.call_at(loop_time, _run_async_call_action, hass, job)
if job.cancel_on_shutdown:
hass.async_track_timer_handle(handle)
Copy link
Member

@bdraco bdraco Jan 28, 2024

Choose a reason for hiding this comment

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

The upside to doing this here is that its not doing for all handles which makes it cheaper. It still means uvloop would fail in tests since async_fire_time_changed in the tests helper needs access to ._scheduled. But since its only in tests, maybe we can figure out how to access whats scheduled based on if its asyncio or uvloop in the test code

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh indeed

@bdraco
Copy link
Member

bdraco commented Jan 28, 2024

I'm not sure the broadcast issue is sorted out MagicStack/uvloop#540

@jpbede
Copy link
Member Author

jpbede commented Jan 28, 2024

Seems to work locally for me, I just wrote a small script and captured the UDP traffic with Wireshark:

image image

@bdraco
Copy link
Member

bdraco commented Jan 28, 2024

Seems to work locally for me, I just wrote a small script and captured the UDP traffic with Wireshark:

Nice. Any chance you can work on which change in uvloop made it work so we can document it in that issue and ask the op to close it ?

@jpbede
Copy link
Member Author

jpbede commented Jan 28, 2024

Any chance you can work on which change in uvloop made it work so we can document it in that issue and ask the op to close it ?

Yes, I'll try it

@jpbede
Copy link
Member Author

jpbede commented Jan 28, 2024

Any chance you can work on which change in uvloop made it work so we can document it in that issue and ask the op to close it ?

Okay, my bad... With socket it works, with an asyncio.DatagramProtocol it does not work.

@jpbede
Copy link
Member Author

jpbede commented Jan 29, 2024

MagicStack/uvloop#592 should fix the broadcast issue. Lets see if they are happy with it.

Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label Mar 29, 2024
@jpbede
Copy link
Member Author

jpbede commented Mar 30, 2024

Do we want to continue with this?

@github-actions github-actions bot removed the stale label Mar 30, 2024
Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes.
Thank you for your contribution!

@github-actions github-actions bot added the stale label May 29, 2024
@github-actions github-actions bot closed this Jun 7, 2024
@bdraco
Copy link
Member

bdraco commented Jun 7, 2024

It looks like uvloop hasn't merged anything in 8 months and needs some cython 3 fixes as well.

It would be nice to use it but its not looking like its being maintained these days.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants