Skip to content

Close the async ECS discovery clients on coordinated shutdown - #955

Open
pjfanning wants to merge 2 commits into
apache:mainfrom
pjfanning:aws-async-client-shutdown
Open

Close the async ECS discovery clients on coordinated shutdown#955
pjfanning wants to merge 2 commits into
apache:mainfrom
pjfanning:aws-async-client-shutdown

Conversation

@pjfanning

Copy link
Copy Markdown
Member

Motivation

Follow-up to #907, which added CoordinatedShutdown client-close tasks to the deprecated pekko-discovery-aws-api module but left pekko-discovery-aws-api-async — the module the deprecation warning tells users to migrate to — untouched. Both AsyncEcsServiceDiscovery and AsyncEcsTaskSetDiscovery build an EcsAsyncClient and never close it.

The leak is bigger here than the one that was fixed. AsyncEcsServiceDiscovery does:

val httpClient = NettyNioAsyncHttpClient.create()
EcsAsyncClient.builder().overrideConfiguration(conf).httpClient(httpClient).build()

In AWS SDK v2, a client passed to .httpClient(...) is treated as caller-owned and wrapped in NonManagedSdkAsyncHttpClient, whose close() 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

  • Both classes get an AtomicReference populated only after the client is built, plus a PhaseServiceUnbind task that closes whatever client it finds there and returns Done when 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.)
  • The close runs on pekko.actor.default-blocking-io-dispatcher, since closing a Netty-backed client blocks until its event loops have shut down.
  • AsyncEcsServiceDiscovery switches to .httpClientBuilder(NettyNioAsyncHttpClient.builder()) so the SDK owns the Netty client and shuts its event loops down on close().
  • Client construction moves behind private[ecs] def createEcsClient(), so the spec can supply a stub without AWS credentials, a region, or network access. Both classes are @ApiMayChange and non-final; MiMa is clean.
  • Docs note that the clients are created lazily and closed at service-unbind.

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 at all before this PR.
  • Directionality: the two "close the ECS client on coordinated shutdown" cases assert close() was called, which nothing on main does — there is no shutdown task there. I could not run them verbatim against main because the createEcsClient seam 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" and sbt headerCreateAll — clean.
  • Not exercised against live AWS — no account available. The spec stubs EcsAsyncClient so no credentials, region or network access are needed.

References

Refs #907, Refs #954

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
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.

1 participant