Avoid ClassLoader leak from static default async executor (#3178)#3394
Avoid ClassLoader leak from static default async executor (#3178)#3394seonwooj0810 wants to merge 2 commits into
Conversation
AsyncFeign held its default ExecutorService in a static singleton (LazyInitializedExecutorService) that was never shut down. Inside a servlet container, the daemon thread pool retained a strong reference to the initial application's ContextClassLoader, so each redeployment leaked the previous application's ClassLoader and threads, eventually causing a Metaspace OutOfMemoryError. Replace the static singleton with an instance-scoped default executor created per built client, so it (and its threads) become eligible for GC once the client is discarded. Also add AsyncBuilder.executorService(...) so callers can supply and own a managed, shut-downable executor. Fixes OpenFeign#3178 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: seonwoo_jung <79202163+seonwooj0810@users.noreply.github.com>
velo
left a comment
There was a problem hiding this comment.
Thanks for the fix and the executor coverage. I cannot approve this while the required CircleCI pr-build check is failing.\n\nGate results:\n1. Test coverage: PASS.\n2. Backwards compatibility: PASS.\n3. Security: PASS.\n\nBlocking issue:\n- Required check ci/circleci: pr-build is currently failing for this PR. Please get the build green before this can be approved or merged.
|
I tracked the failing I pushed ea6c1db to update the expected field count. CI has been retriggered on the new head. |
ea6c1db to
268d8e9
Compare
|
Follow-up: the first push only exposed the next assertion in the same test. The final fix is 268d8e941055393fbefed2aecc98a83f7fd91ee0: |
velo
left a comment
There was a problem hiding this comment.
Thanks for updating the branch. The per-client default executor and caller-managed executor option are covered by regression tests, preserve existing API behavior, and introduce no security concern. Required checks are green.
Fixes #3178
Problem
AsyncFeignheld its defaultExecutorServicein a private static singleton (LazyInitializedExecutorService) that is never shut down:Inside a servlet container, marking the threads as daemon only lets the JVM exit — it does not help on redeploy. The static cached thread pool retains a strong reference to the initial application's
ContextClassLoader, so each redeployment leaks the previous application'sClassLoaderand its threads, eventually leading to a MetaspaceOutOfMemoryError.Change
defaultExecutorService()). Once the built client is discarded, the executor and its (daemon, idle-timing-out) threads become eligible for garbage collection, releasing theClassLoader.AsyncBuilder.executorService(ExecutorService)so callers can supply and own a managed, shut-downable executor — the recommended way to control this lifecycle explicitly. It is ignored when a customclient(AsyncClient)is provided.Default behavior is preserved for callers who don't configure anything: a cached daemon thread pool is still used; it is simply no longer a process-wide static singleton.
Note:
feign.kotlin.CoroutineFeigncontains the same static-singleton pattern. I kept this PR scoped toAsyncFeign(the reported class); happy to follow up on the Kotlin module if you'd like it addressed here too.Tests
Added to
core/src/test/java/feign/AsyncFeignTest.java:usesProvidedExecutorService— builds anAsyncFeignwith a spiedExecutorService, performs a request, and verifies the provided executor actually ran the work (submit(...)invoked).defaultExecutorServiceStillExecutesRequests— guards that the default (no executor configured) path still works end-to-end after removing the static singleton.Test evidence
./mvnw -pl core -am test -Dtest=AsyncFeignTest→Tests run: 54, Failures: 0, Errors: 0, Skipped: 0. Code format validated withgit-code-format:validate-code-format(BUILD SUCCESS).Verification done: confirmed no in-flight PR referencing #3178; reproduced the static-singleton pattern still present on
master(AsyncFeign.java); maintainer invited a PR with automated tests on the issue; fix touches only.javafiles; build + targeted tests + format check pass locally on JDK 25.