[fix][client] Prevent client shutdown from leaking event loop threads when DNS resolver close fails#26045
Conversation
… when DNS resolver close fails PulsarClientImpl.shutdown() closed the Netty addressResolver and the local DnsResolverGroup instance without a try/catch, unlike every other resource it closes. When addressResolver.close() throws (e.g. the Netty 'IllegalStateException: channel not registered to an event loop' race that can happen when a DNS lookup is in flight at shutdown), the exception aborts the rest of shutdown() before shutdownEventLoopGroup(), closeCnxPool(), timer.stop() and shutdownExecutors() run. The eventLoopGroup's non-daemon threads are then leaked and the JVM never exits. Wrap both DNS resolver close calls in try/catch and record the throwable, so shutdown always proceeds to release the remaining resources.
|
This is currently blocking the Quarkus upgrade to A released 4.2.x containing this fix would unblock that PR. A backport to |
…solver close failure Verifies that PulsarClientImpl.shutdown() still releases the remaining resources (asserted via timer.stop()) when addressResolver.close() throws, simulating the Netty 'channel not registered to an event loop' race. The test fails without the shutdown fix (timer.stop() is never reached) and passes with it.
|
Thanks for getting this merged and backported to branch-4.2 (commit 2baa1f7) so quickly! 🙏 Is there a planned timeline for cutting a 4.2.3 release? This fix is currently blocking a downstream Quarkus upgrade (quarkusio/quarkus#54516) — the Quarkus If a 4.2.3 release isn't on the near-term roadmap, is there anything I can help with to move it along (e.g. testing a release candidate)? Happy to assist. Thanks again! |
Motivation
PulsarClientImpl.shutdown()closes most resources defensively: eachclose()is wrapped in its owntry/catchand the last failure is recorded inthrowableso that shutdown always continues and releases the remaining resources. Two cleanup steps, however, are not guarded:These run before
shutdownEventLoopGroup(),closeCnxPool(),timer.stop()andshutdownExecutors(). IfaddressResolver.close()throws, the exception propagates straight to the outercatch, and the remaining cleanup never runs — so theEventLoopGroup's non-daemon threads (plus the connection pool, timer and executors) are leaked and the JVM never exits.This is reachable in practice: when a DNS lookup is still in flight as the client shuts down, the Netty address resolver can throw
java.lang.IllegalStateException: channel not registered to an event loop(a known Netty shutdown race). It reproduces consistently when upgrading Quarkus topulsar-client4.2.1 — the test JVM hangs and is eventually killed by the Surefire fork timeout (~20 minutes), with:The DNS resolver group was introduced in #24784, and the unguarded close has been present since then (4.2.0 onward, including 4.2.1/4.2.2 and
master).Modifications
Wrap
addressResolver.close()anddnsResolverGroupLocalInstance.close()intry/catch, logging the failure and recording it inthrowable, exactly like every other resource closed byshutdown(). This guaranteesshutdown()always proceeds to release the event loop group, connection pool, timer and executors even if the DNS resolver fails to close, while still surfacing the original failure to the caller.Verifying this change
This change is a trivial rework / code cleanup without any test coverage. It restores the existing best-effort cleanup contract for two steps that were missing it; the surrounding steps already follow this pattern.
Does this pull request potentially affect one of the following parts: