Skip to content

fix: make DgraphAsyncClient non-blocking to avoid ForkJoinPool.commonPool starvation - #294

Open
mlwelles wants to merge 4 commits into
mainfrom
fix/async-client-non-blocking-retries
Open

fix: make DgraphAsyncClient non-blocking to avoid ForkJoinPool.commonPool starvation#294
mlwelles wants to merge 4 commits into
mainfrom
fix/async-client-non-blocking-retries

Conversation

@mlwelles

@mlwelles mlwelles commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Description

DgraphAsyncClient ran its async work on ForkJoinPool.commonPool() and blocked a pool thread for the full duration of every gRPC call: CompletableFutures.runWithRetries wrapped each call in supplyAsync(...) and then called a blocking .get() on the gRPC future. Under sustained or slow traffic this exhausted the JVM-wide common pool and could hang unrelated work (parallel streams, other CompletableFuture chains). Fixes #293.

This PR:

  • Rewrites runWithRetries to compose on the StreamObserverBridge future (handleAsync plus a single JWT-expiry retry via thenComposeAsync) instead of blocking. No thread is parked for the round trip; the supplied Executor becomes a callback executor. The default commonPool() is now safe because the callbacks never block.
  • Preserves the observed exception exactly: every failure completes the returned future with CompletionException(DgraphException), so .join() in the synchronous DgraphClient still surfaces the typed DgraphException.
  • Fixes an adjacent race in the login/refresh path: the jwt field was written inside a thenAccept callback after the write lock was released, leaving the write unguarded and unpublished. The write now happens in a write-lock-held setter, and the refresh token is read under the read lock.
  • Adds CompletableFuturesTest — server-free unit tests, including a regression test that reproduces the common-pool starvation, plus retry and exception-translation characterization tests.

No public API changes. The Executor constructor parameter's meaning is a compatible superset (callback executor), documented in the constructor Javadoc.

Checklist

  • Code compiles correctly and linting passes locally
  • For all code changes, an entry added to the CHANGELOG.md file describing and linking to
    this PR
  • Tests added for new functionality, or regression tests for bug fixes added as applicable

mlwelles added 3 commits July 23, 2026 08:59
runWithRetries wrapped each gRPC call in supplyAsync and then blocked the
executor thread on .get() for the whole round trip, which starves
ForkJoinPool.commonPool() under load. Compose on the stub future instead so no
thread is parked; the executor becomes a callback executor.

Refs #293
The jwt field was written inside a thenAccept callback after the write lock was
released, leaving the write unguarded and unpublished across threads. Move the
write into a lock-held setter and read the refresh token under the read lock.

Refs #293
Document in the DgraphAsyncClient constructors that the Executor is a callback
executor and that the commonPool default is safe because callbacks never block.
Also dedupe a log message in setJwt and cover the null-future path in
runWithRetries.

Refs #293
@mlwelles
mlwelles requested a review from a team as a code owner July 24, 2026 01:55
@mlwelles

Copy link
Copy Markdown
Contributor Author

Note on the red dgraph4j-tests check: the only failing test, AlterConvenienceTest.testDropType, is a pre-existing failure unrelated to this change.

  • It fails identically on main. The scheduled ci-dgraph4j-tests run on 2026-07-23 (run 29994485774) failed on the same test with the same assertion (Type Person should exist in schema before drop ... expected [true] but found [false]) and the same schema dump — the name predicate is applied but the Person type is absent from schema { types }.
  • Test-count corroboration: main ran 146 tests (1 failed); this PR ran 153 (1 failed). The 7 extra are the new CompletableFuturesTest unit tests added here, all passing. The same single test is red in both.
  • Root cause is server-side. This job builds Dgraph from latest source on every run, and the type-definition visibility behavior changed. This PR only changes client-side threading, not schema requests.

The other 152 integration tests, CodeQL, and Trunk are green. This looks worth tracking separately rather than blocking this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

DgraphAsyncClient blocks ForkJoinPool.commonPool threads and can starve the JVM common pool

1 participant