Skip to content

handle timeout attribute at __init__ instead of when starting the timer.#243

Open
Vizonex wants to merge 3 commits into
aio-libs:masterfrom
Vizonex:timeout
Open

handle timeout attribute at __init__ instead of when starting the timer.#243
Vizonex wants to merge 3 commits into
aio-libs:masterfrom
Vizonex:timeout

Conversation

@Vizonex
Copy link
Copy Markdown
Member

@Vizonex Vizonex commented May 13, 2026

What do these changes do?

This change moves the timeout to a better more optimized location which will ultimately increase the performance of the timeout loop.

Are there changes in behavior for the user?

There aren't any. In fact nothing new needs to be added in which is a bonus.

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@Vizonex Vizonex changed the title handle timeout attribute at __init__ instead of when starting the timer. handle timeout attribute at __init__ instead of when starting the timer. May 13, 2026
@Vizonex Vizonex requested review from bdraco and saghul May 13, 2026 00:29
@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.53%. Comparing base (5ffa111) to head (0724652).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage   98.46%   98.53%   +0.06%     
==========================================
  Files           5        5              
  Lines        1433     1432       -1     
  Branches       74       74              
==========================================
  Hits         1411     1411              
  Misses         16       16              
+ Partials        6        5       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@saghul
Copy link
Copy Markdown
Contributor

saghul commented May 13, 2026

Unlike before, we lose track of the original timeout with your change. I need to check if that has actual implications here.

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Jun 1, 2026

Have you actually tested this? It's broken.

The reason why we used a different timeout for arming our timer is because of some old c-ares bug IIRC, that's why we poll every 100 ms. Probably worth revisiting, since a lot has changed since.

The real timeout needs to be passed to to c-ares unmodified, of course.

Note, however, that our own timer will now only be used in the fallback case, since we use the event thread by default, which doesn't need it.

Copy link
Copy Markdown
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

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

.

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.

2 participants