Skip to content

Fix blocking call in Freebox integration #143071

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 16 commits into
base: dev
Choose a base branch
from
Draft

Fix blocking call in Freebox integration #143071

wants to merge 16 commits into from

Conversation

samybali
Copy link

@samybali samybali commented Apr 16, 2025 •
edited
Loading

Breaking change

None.

Proposed change

This PR resolves an issue in the Freebox integration where a blocking call (router().open) was made in the event loop, causing warnings in the logs. The blocking call is now wrapped in hass.async_add_executor_job to ensure it runs in a background thread.

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

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.

To help with the load of incoming pull requests:

@samybali samybali requested a review from Quentame as a code owner April 16, 2025 09:02
home-assistant[bot]
home-assistant bot previously requested changes Apr 16, 2025
View reviewed changes
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @samybali

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant home-assistant bot marked this pull request as draft April 16, 2025 09:02
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant
Copy link

Hey there @hacf-fr, @Quentame, mind taking a look at this pull request as it has been labeled with an integration (freebox) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of freebox can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign freebox Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@epenet epenet marked this pull request as ready for review April 16, 2025 10:05
@samybali
Copy link
Author

samybali commented Apr 22, 2025 •
edited
Loading

Hi @Quentame, I see that you are quoted as the code owner for this component. Therefore, would you mind checking if my changes work for you? Moreover, I fixed the mistakes that made the checks fail.

@Quentame
Copy link
Member

Hi @samybali !
Yes, I've just got credentials from a Freebox as mine is out from few weeks ago.
Will check this week.

@samybali
Copy link
Author

samybali commented Apr 22, 2025 •
edited
Loading

Hi @samybali ! Yes, I've just got credentials from a Freebox as mine is out from few weeks ago. Will check this week.

Ok thank you. Do you know if it is possible to run again the checks to see if it works as expected because it tells me that the workflow needs approval ?

@abmantis
Copy link
Member

abmantis commented Apr 22, 2025 •
edited
Loading

Hey @samybali , I understand you are eager to get this merged, but there is no need to ping people, as noted in our docs :)

Maintainers will eventually get to your PR, but as you can see we have a lot of PRs. The best way to help is to also do some reviews 😄

I have approved the CI run now.

abmantis
abmantis reviewed Apr 22, 2025
View reviewed changes
Copy link
Member

@abmantis abmantis left a comment

Choose a reason for hiding this comment

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

I think this needs to be fixed in the freebox_api library, because the api.open method is already async, so it should not be doing blocking calls.

We have a few examples on SSL stuff here: https://developers.home-assistant.io/docs/asyncio_blocking_operations?#load_default_certs

@joostlek joostlek marked this pull request as draft April 25, 2025 16:41
@Xavinooo
Copy link

Xavinooo commented Apr 28, 2025 •
edited
Loading

Hi,

I agree with @abmantis, the fix should be in the api. In fact, there is alreay a PR there to fix this : hacf-fr/freebox-api#799

@github-actions GitHub Actions
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 Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abmantis abmantis abmantis left review comments

@home-assistant home-assistant[bot] home-assistant[bot] left review comments

@Quentame Quentame Awaiting requested review from Quentame Quentame is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@Quentame Quentame

Labels
blocking event loop bugfix cla-signed code-quality has-tests integration: freebox Quality Scale: No score small-pr PRs with less than 30 lines. stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Detected blocking call to set_default_verify_paths for freebox
5 participants
@samybali @Quentame @abmantis @Xavinooo @bdraco