Close the async ECS discovery clients on coordinated shutdown - #955
Open
pjfanning wants to merge 2 commits into
Open
Close the async ECS discovery clients on coordinated shutdown#955pjfanning wants to merge 2 commits into
pjfanning wants to merge 2 commits into
Conversation
Motivation: apache#907 gave the deprecated `pekko-discovery-aws-api` module a CoordinatedShutdown task that closes its AWS SDK v1 clients, but left `pekko-discovery-aws-api-async` - the module users are told to migrate to - untouched. Both `AsyncEcsServiceDiscovery` and `AsyncEcsTaskSetDiscovery` build an `EcsAsyncClient` and never close it. The leak is larger there than in the module that was fixed. `AsyncEcsServiceDiscovery` passes `NettyNioAsyncHttpClient.create()` to `.httpClient(...)`, which the SDK treats as caller-owned and wraps so that `close()` is a no-op, so its event loop threads outlive the ActorSystem even if the ECS client were closed. Modification: Give both classes an `AtomicReference` that is populated only after the client has been built, and a `PhaseServiceUnbind` task that closes whatever client it finds there and does nothing when it is null - so shutdown never forces, or re-attempts, the lazy initialisation. The close runs on `pekko.actor.default-blocking-io-dispatcher` because it blocks until the underlying HTTP client has shut down. Switch `AsyncEcsServiceDiscovery` from `.httpClient(NettyNioAsyncHttpClient .create())` to `.httpClientBuilder(NettyNioAsyncHttpClient.builder())` so the SDK owns the Netty client and shuts down its event loops when the ECS client is closed. Client construction moves behind `private[ecs] def createEcsClient()` so the new spec can supply a stub without AWS credentials, a region, or network access. Result: Terminating the ActorSystem releases the ECS clients and their Netty event loop threads. Discovery that is configured but never used still creates no client. Tests: - sbt "discovery-aws-api-async/test" - 4 succeeded, 0 failed (new AsyncEcsClientShutdownSpec; this module had no test sources before) - sbt "discovery-aws-api-async/mimaReportBinaryIssues" - success - sbt "discovery-aws-api-async/scalafmt" "discovery-aws-api-async/Test/scalafmt", sbt headerCreateAll - clean - Not run against live AWS - no account available; the spec stubs EcsAsyncClient. References: Refs apache#907
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Follow-up to #907, which added
CoordinatedShutdownclient-close tasks to the deprecatedpekko-discovery-aws-apimodule but leftpekko-discovery-aws-api-async— the module the deprecation warning tells users to migrate to — untouched. BothAsyncEcsServiceDiscoveryandAsyncEcsTaskSetDiscoverybuild anEcsAsyncClientand never close it.The leak is bigger here than the one that was fixed.
AsyncEcsServiceDiscoverydoes:In AWS SDK v2, a client passed to
.httpClient(...)is treated as caller-owned and wrapped inNonManagedSdkAsyncHttpClient, whoseclose()is a no-op. So the Netty event loop threads would survive even if the ECS client were closed..httpClientBuilder(...)is the form that hands ownership to the SDK.Modification
AtomicReferencepopulated only after the client is built, plus aPhaseServiceUnbindtask that closes whatever client it finds there and returnsDonewhen it is null — so shutdown never forces, or re-attempts, the lazy initialisation. (Same shape as Only close AWS discovery clients that were actually built #954, which fixes that ordering bug in the v1 module.)pekko.actor.default-blocking-io-dispatcher, since closing a Netty-backed client blocks until its event loops have shut down.AsyncEcsServiceDiscoveryswitches to.httpClientBuilder(NettyNioAsyncHttpClient.builder())so the SDK owns the Netty client and shuts its event loops down onclose().private[ecs] def createEcsClient(), so the spec can supply a stub without AWS credentials, a region, or network access. Both classes are@ApiMayChangeand non-final; MiMa is clean.service-unbind.Result
Terminating the
ActorSystemreleases the ECS clients and their Netty event loop threads. Discovery that is configured but never used still creates no client.Tests
sbt "discovery-aws-api-async/test"— 4 succeeded, 0 failed. NewAsyncEcsClientShutdownSpec; this module had no test sources at all before this PR.close()was called, which nothing onmaindoes — there is no shutdown task there. I could not run them verbatim againstmainbecause thecreateEcsClientseam they stub does not exist there.sbt "discovery-aws-api-async/mimaReportBinaryIssues"— success.sbt "discovery-aws-api-async/scalafmt" "discovery-aws-api-async/Test/scalafmt"andsbt headerCreateAll— clean.EcsAsyncClientso no credentials, region or network access are needed.References
Refs #907, Refs #954