Skip to content

fix(pool): use time.monotonic() instead of time.time() for connection timestamps#13288

Open
algojogacor wants to merge 1 commit into
sqlalchemy:mainfrom
algojogacor:fix/pool-monotonic-time
Open

fix(pool): use time.monotonic() instead of time.time() for connection timestamps#13288
algojogacor wants to merge 1 commit into
sqlalchemy:mainfrom
algojogacor:fix/pool-monotonic-time

Conversation

@algojogacor

Copy link
Copy Markdown

Description

Replaced all time.time() calls in pool/base.py with time.monotonic() for connection record timestamp tracking. Also updated the comment block that documented the Windows precision caveat.

time.time() on Windows has approximately 16 ms granularity, which can cause soft invalidation checks to fail when _soft_invalidate_time and starttime are captured in the same timer quantum. time.monotonic() is monotonic by definition and provides sub-millisecond precision on all platforms, so the > comparisons between timestamps work correctly regardless of platform timer resolution.

Four sites changed (all in lib/sqlalchemy/pool/base.py):

  • Pool._invalidate_time assignment (line 407)
  • _ConnectionRecord._soft_invalidate_time assignment (line 812)
  • Connection recycle time check in get_connection (line 838)
  • _ConnectionRecord.starttime assignment in __connect (line 893)

All comparisons are relative (timestamp vs timestamp or timestamp delta vs timeout), so switching to time.monotonic() preserves existing behavior while fixing the precision issue.

… timestamps

Replaced all time.time() calls in pool/base.py with time.monotonic()
for connection timestamp tracking. time.monotonic() is monotonic across
all platforms and provides sub-millisecond precision, resolving the
Windows ~16 ms granularity issue where soft invalidation checks could
fail when both timestamps were captured within the same timer quantum.

Fixes: sqlalchemy#13169

@zzzeek zzzeek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to do what I propose here which is dont hardcode time functions that are used in this way:

  1. note that we also use time.time() in queue.py ; modern cpython correctly uses monotonic
  2. other places we use time() for cache invalidation:
    @contextlib.contextmanager
    def skip_if_timeout(seconds: float, cleanup: Any = None):
    now = time.time()
    yield
    sec = time.time() - now
    if sec > seconds:
    try:
    cleanup()
    finally:
    config.skip_test(
    f"test took too long ({sec:.4f} seconds > {seconds})"
    )
    and three places in asyncpg such as
    self._invalidate_schema_cache_asof = time.time()

steps:

  1. add new library function sqlalchemy.util.compat.highres_clock() (or whatever name, but the point is it should be named for the use case, not the implementation)
  2. apply to pool/base.py, postgresql/asyncpg.py, util/queue.py, testing/util.py
  3. this will be for 2.1 and i think we should have a short changelog
  4. thanks!

# the issue where time.time() could have ~16 ms granularity
# on Windows, causing comparisons like _soft_invalidate_time >
# starttime to fail when both timestamps were captured within
# the same timer quantum.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I dont want to lose the note about how >= would eliminate the issue, but would run a theoretical risk of constant re-connects if "time" were not incrementing.

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.

Connection pool soft invalidation uses time.time() comparison that can silently fail on low-resolution clocks (e.g. Windows ~16 ms)

2 participants