Skip to content
This repository was archived by the owner on Oct 2, 2026. It is now read-only.

feature/Sync-client-facade · L-260907-91b052 — no asyncio.run wrapper per call site - #27

Closed
lchoquel wants to merge 3 commits into
devfrom
feature/Sync-client-facade
Closed

lchoquel wants to merge 3 commits into
devfrom
feature/Sync-client-facade

Conversation

@lchoquel

@lchoquel lchoquel commented Sep 12, 2026 •

Copy link
Copy Markdown
Member

Adds SyncPipelexAPIClient (pipelex_sdk/sync_client.py), a synchronous facade over PipelexAPIClient, so a script, a batch job or a Django view calls every method without await or an asyncio.run wrapper of its own. It wraps and delegates to one private async client on a daemon event-loop thread rather than adding a second transport, so the connection pool stays warm and one instance can be shared across threads; a call from inside a running event loop raises the new SyncClientInEventLoopError before anything is sent. A parity test pins each sync method's signature to its async twin in both directions, and the docs, README and changelog describe the facade.

Closes L-260907-91b052

🤖 Generated with Claude Code

lchoquel and others added 3 commits September 12, 2026 22:45
…lient

A script, a batch job or a Django view can now call every client method without
await and without writing its own asyncio.run wrapper. The facade wraps and
delegates to one private PipelexAPIClient running on a daemon event-loop thread,
so there is still exactly one HTTP implementation; one instance is shareable
across threads, and a call from inside a running event loop raises
SyncClientInEventLoopError before anything is sent. A parity test pins every
public coroutine's sync twin to an identical signature.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…heir contract under concurrency

The first review round on the sync facade (L-260907-91b052) reproduced these defects, and each now has a regression test that fails on the previous code.

- A call made on another thread while close() ran started a second loop on the HTTP client the first loop was still closing, and failed with "Cannot send a request, as the client has been closed". close() now holds the facade's lock until the old loop is gone. The lock is reentrant, so an abandoned iterator that a garbage collection finalizes under it no longer deadlocks its own thread.
- A KeyboardInterrupt or SystemExit raised on the loop, such as an on_poll callback calling sys.exit(), stopped the loop and left both the caller and close() blocked for good. Both are now carried back to the caller, and the loop survives.
- Ctrl+C during a page fetch surfaced as RuntimeError("aclose(): asynchronous generator is already running") instead of the interrupt. The async generator is now finalized only when its iterator is abandoned between two items.
- A paged iterator left open across close() ended silently, as if the listing were complete. It now raises concurrent.futures.CancelledError at its next item.
- A partly read iterator left in a module global kept the interpreter from exiting, because it waited on a daemon loop thread that had already stopped.
- The in-loop refusal named the wrapped coroutine (MthdsAPIClient.models, _next_item) instead of the facade method that was called.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
…the result

The parity test compared signatures only, so a twin that dropped or swapped an argument passed the whole suite; mutating iterate_methods to forward limit=None proved it. Each twin is now called with a distinct sentinel per parameter and must hand every one to its coroutine under its own name, then return what the coroutine returned.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@lchoquel

Copy link
Copy Markdown
Member Author

Review round 1: profile 4 (deep), bar open.

Reviewers that produced a review: cubic, Codex (review), Codex (adversarial), code-review at high, and gstack's review. gstack's report says it skipped its own Codex passes.

Fixed in c742052, each with a regression test that fails on the previous code:

  • A call made while close() ran started a second loop on the HTTP client the first loop was still closing. close() now holds the facade's lock until the old loop is gone.
  • A KeyboardInterrupt or SystemExit raised on the loop, such as an on_poll callback calling sys.exit(), stopped the loop and left the caller and close() blocked for good. Both now reach the caller, and the loop survives.
  • Ctrl+C during a page fetch surfaced as RuntimeError('aclose(): asynchronous generator is already running') instead of the interrupt.
  • A paged iterator left open across close() ended silently. It now raises concurrent.futures.CancelledError.
  • An abandoned iterator collected by the garbage collector while its facade held the lock deadlocked. The lock is now reentrant.
  • A partly read iterator left in a module global kept the interpreter from exiting.
  • The in-loop refusal named the wrapped coroutine instead of the facade method that was called.

Fixed in d0c0fbc: the parity test also pins that every twin forwards each argument and hands back the result. A twin dropping an argument used to pass the whole suite.

Deferred:

  • A facade dropped without close() keeps its loop thread and descriptors until the process exits: L-260912-a5f857.
  • A facade used in a forked child blocks forever: L-260912-166b5f.

@lchoquel

lchoquel commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

Closing unmerged. The two review rounds showed that sharing one facade across threads through a background event loop does not converge, so the facade will be rebuilt on asyncio.Runner, one facade per thread, in Pipelex/pipelex-sdk once the SDKs move there, rather than re-applied from this branch. The branch is kept as a reference. Tracked as L-260907-91b052; the Python 3.11 cancellation bug in wait_for_result that round 2 surfaced is fixed separately as L-261001-3aae86.

@lchoquel lchoquel closed this Oct 1, 2026
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 1, 2026
@lchoquel
lchoquel deleted the feature/Sync-client-facade branch October 1, 2026 19:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant