Skip to content

Consolidate gremlin-python read timeout into a single ReadTimeoutError#3507

Merged
kenhuuu merged 3 commits into
apache:masterfrom
GumpacG:python-fix
Jul 10, 2026
Merged

Consolidate gremlin-python read timeout into a single ReadTimeoutError#3507
kenhuuu merged 3 commits into
apache:masterfrom
GumpacG:python-fix

Conversation

@GumpacG

@GumpacG GumpacG commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

gremlin-python armed two read-timeout mechanisms at the same value - the driver's
async_timeout wrapper (raising asyncio.TimeoutError) and aiohttp's sock_read
(raising SocketTimeoutError). Whichever fired first was a race, so a read timeout
surfaced nondeterministically as one type or the other, which also made
test_client_side_timeout_set_for_aiohttp flaky (it asserted on the exception message).

Change: remove the redundant async_timeout read wrapper and keep aiohttp sock_read
as the single read-timeout source (it also covers the initial-response wait and is
streaming-safe). Normalize its SocketTimeoutError/ServerTimeoutError into a new
driver-owned ReadTimeoutError so a read timeout always surfaces as one deterministic,
transport-agnostic type. ReadTimeoutError subclasses the builtin TimeoutError, so it
stays catchable via except TimeoutError while leaking neither asyncio nor aiohttp types
(addresses review feedback). It is documented as temporary: once the driver is fully
async it should revert to asyncio.TimeoutError (TINKERPOP-2774).

Assisted-by: Kiro: Claude Opus 4.8

@codecov-commenter

codecov-commenter commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.41%. Comparing base (a28cd1f) to head (73d8df2).
⚠️ Report is 204 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3507      +/-   ##
============================================
+ Coverage     76.35%   76.41%   +0.05%     
- Complexity    13424    13924     +500     
============================================
  Files          1012     1027      +15     
  Lines         60341    62630    +2289     
  Branches       7075     7355     +280     
============================================
+ Hits          46076    47856    +1780     
- Misses        11548    11793     +245     
- Partials       2717     2981     +264     

☔ View full report in Codecov by Harness.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@GumpacG GumpacG marked this pull request as draft July 6, 2026 15:42
@GumpacG GumpacG changed the title Fix flaky python test Consolidate gremlin-python read timeout to a single asyncio.TimeoutError Jul 6, 2026
@GumpacG GumpacG marked this pull request as ready for review July 6, 2026 20:21
Comment thread gremlin-python/src/main/python/gremlin_python/driver/aiohttp/transport.py Outdated
@GumpacG GumpacG changed the title Consolidate gremlin-python read timeout to a single asyncio.TimeoutError Consolidate gremlin-python read timeout into a single ReadTimeoutError Jul 6, 2026
@kenhuuu

kenhuuu commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

VOTE +1

@kenhuuu

kenhuuu commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

merging as lazy consensus reached

@kenhuuu kenhuuu merged commit c512ac9 into apache:master Jul 10, 2026
47 of 48 checks passed
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.

3 participants