Skip to content

feat: add async pagination support#156

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/async-pagination
Open

feat: add async pagination support#156
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/async-pagination

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Pagination was sync-only. Paginator walks pages over an HttpClient, which means a consumer must block a thread per page. This adds AsyncPaginator — the non-blocking counterpart that drives the same page-to-page walk over an AsyncHttpClient and returns a CompletableFuture, so no thread parks waiting on a page.

What this adds

  • org.dexpace.sdk.core.pagination.AsyncPaginator<T>, mirroring Paginator<T>:
    • forEachAsync(Consumer<in T>): CompletableFuture<Void> — walks every item across every page, invoking the consumer per item; the future completes when the walk finishes or completes exceptionally on transport/parse/consumer failure.
    • collectAllAsync(): CompletableFuture<MutableList<T>> — convenience that buffers all items.
    • Same @JvmOverloads constructor shape as Paginator (client, initialRequest, strategy, optional maxPages).

Design notes

  • Reuses the existing seam. It runs on the unchanged PaginationStrategy / Page wire-convention types, so any strategy written for the sync Paginator (cursor, page-number, Link-header) works here with no changes.
  • Preserves the safety cap and per-page lifecycle. maxPages still bounds the number of HTTP exchanges, and each Response is closed after the strategy parses it — including on the exceptional path, before the result future fails.
  • Stack-safe. The driver is trampolined: synchronously completed page futures (in-memory fakes, cache hits) are processed in a loop rather than via recursive thenCompose, so a long run of already-complete pages does not overflow the stack. A 5,000-page synchronous-completion test covers this.
  • Transport-agnostic, zero new deps. Lives in sdk-core and uses only java.util.concurrent (CompletableFuture). Reactor/coroutines Flow/Flux bridges remain the responsibility of the adapter modules, per the layering in CLAUDE.md.

Tests

A new AsyncPaginatorTest exercises multi-page walks (both synchronously-completed and completed on a background executor), the maxPages cap, transport failures, strategy-parse failures (asserting the response is still closed), consumer-thrown aborts, response-close counting, empty terminal pages, and deep stack-safety. Backed by a new StubAsyncHttpClient fake that can complete inline or via an executor.

Gated build (run locally; root build skipped per repo guidance to avoid R8 + kover aggregate)

./gradlew :sdk-core:ktlintCheck :sdk-core:detekt :sdk-core:test :sdk-core:apiCheck --no-daemon

All green. :sdk-core:apiDump was run and the regenerated sdk-core/api/sdk-core.api is committed.

Dependency note

Issue #34 lists #30 (pagination-stack unification) as a blocker. That unification (PR #145) is still open and not on main, so this targets the current pagination surface (Paginator + PaginationStrategy + Page). If #145 lands, AsyncPaginator should be re-pointed at the unified surface; because it only depends on PaginationStrategy/Page, that follow-up is mechanical.

Closes #34

Pagination was sync-only: Paginator walks pages over an HttpClient and a
consumer must block a thread per page. This adds AsyncPaginator, the
non-blocking counterpart that drives the same page-to-page walk over an
AsyncHttpClient and returns a CompletableFuture, so no thread parks waiting
on a page.

AsyncPaginator reuses the existing PaginationStrategy/Page wire-convention
seam unchanged, so any strategy written for Paginator works here. It exposes
forEachAsync(Consumer) and collectAllAsync(), preserves the maxPages safety
cap, and closes each Response after the strategy parses it (including on the
exceptional path). The driver is trampolined so a long run of
synchronously-completed page futures stays stack-safe.

The surface stays transport-agnostic and in sdk-core with no new
dependencies (java.util.concurrent only); Reactor/coroutines bridges belong
in the adapter modules.
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

This adds a non-blocking AsyncPaginator<T> alongside the existing sync Paginator, walking strategy-driven pages over an AsyncHttpClient via a trampoline of CompletableFutures, with forEachAsync/collectAllAsync and a maxPages cap. The shape is right, the .api snapshot is regenerated, and the test suite is genuinely thorough (sync and background-executor completion, maxPages, transport/parse/consumer failures, response-close counting, empty terminal page, and a 5,000-page stack-safety run). A couple of things to fix before merging, plus a docs nit.

Issues

Cancelling the returned future is a no-opAsyncPaginator.kt:144-148, 193-245. forEachAsync hands back a bare CompletableFuture<Void> that the driver only ever completes; nothing reads its cancellation state. The trampoline never checks result.isCancelled()/isDone() before fetching the next page or invoking the consumer, and the in-flight page/transport future is never chained to it. So a caller that cancels the returned future gets no actual cancellation: the walk keeps issuing HTTP exchanges and calling the consumer, while each later complete/completeExceptionally silently no-ops. For an async, potentially-unbounded paginator that's a real abandonment/resource leak, and AsyncHttpClient already documents cancellation as a supported best-effort transport hook. Worth wiring the in-flight future into result and checking for completion at each trampoline hop, plus a test and a KDoc note on cancellation semantics.

nextPageRequest() can leave the future hung foreverAsyncPaginator.kt:242-257. drainPage guards only the consumer loop in try/catch; the page.nextPageRequest() call on line 255 sits outside it. nextPageRequest() is strategy-defined and may throw for a custom Page. On the async-completion path that throw propagates into the whenComplete callback, where CompletableFuture swallows it, so result never completes and the caller hangs. The built-in SimplePage just returns a field so the common path is safe, but a hung future is a worse failure mode than the sync Paginator (which surfaces the throw to the iterator caller). Pulling nextPageRequest() inside the guard fixes it.

Pagination package docs not updatedAsyncPaginator.kt:121. CLAUDE.md/README still describe the package as "Paginator + 4 strategies". AsyncPaginator is a notable new public entry point; a one-line mention keeps the package overview accurate.

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.

Async pagination support

1 participant