Skip to content

Conversation

@DaveCTurner
Copy link
Contributor

It's possible for another component to request a S3 client after the
node has started to shut down, and today the S3Service will dutifully
attempt to create a fresh client instance even if it is closed. Such
clients will then leak, resulting in test failures.

With this commit we refuse to create new S3 clients once the service has
started to shut down.

It's possible for another component to request a S3 client after the
node has started to shut down, and today the `S3Service` will dutifully
attempt to create a fresh client instance even if it is closed. Such
clients will then leak, resulting in test failures.

With this commit we refuse to create new S3 clients once the service has
started to shut down.
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs auto-backport Automatically create backport pull requests when merged v8.19.0 v9.1.0 labels May 12, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

@elasticsearchmachine elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label May 12, 2025
@DaveCTurner DaveCTurner requested a review from arteam May 12, 2025 09:34
Copy link
Contributor

@arteam arteam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Makes sense to avoid create new SdkHttpClient if S3Service is being shutdown

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 12, 2025
@elasticsearchmachine elasticsearchmachine merged commit 0f9c1ea into elastic:main May 12, 2025
17 checks passed
@DaveCTurner DaveCTurner deleted the 2025/05/12/S3Service-ensure-lifecycle branch May 12, 2025 10:20
DaveCTurner added a commit to DaveCTurner/elasticsearch that referenced this pull request May 12, 2025
It's possible for another component to request a S3 client after the
node has started to shut down, and today the `S3Service` will dutifully
attempt to create a fresh client instance even if it is closed. Such
clients will then leak, resulting in test failures.

With this commit we refuse to create new S3 clients once the service has
started to shut down.
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.19

jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request May 12, 2025
It's possible for another component to request a S3 client after the
node has started to shut down, and today the `S3Service` will dutifully
attempt to create a fresh client instance even if it is closed. Such
clients will then leak, resulting in test failures.

With this commit we refuse to create new S3 clients once the service has
started to shut down.
elasticsearchmachine pushed a commit that referenced this pull request May 12, 2025
It's possible for another component to request a S3 client after the
node has started to shut down, and today the `S3Service` will dutifully
attempt to create a fresh client instance even if it is closed. Such
clients will then leak, resulting in test failures.

With this commit we refuse to create new S3 clients once the service has
started to shut down.
@ywangd
Copy link
Member

ywangd commented May 12, 2025

For my knowledge: Does this issue only matter for the v2 client where we explicitly build a SdkHttpClient? Or is it applicable to v1 client as well and we were just luck enough to not hit it in tests?

@DaveCTurner
Copy link
Contributor Author

In #126843 we dropped the call to IdleConnectionReaper.shutdown(); - the v2 SDK shuts down the connection reaper automatically when all the clients are closed and does not allow it to be shut down otherwise. Also we changed the lifecycle so that the S3Service is a proper LifecycleComponent in its own right, rather than something which closes when the S3RepositoryPlugin closes, which means it may now close after other services that are still trying to use it, particularly the ObjectStoreService.

benchaplin pushed a commit to benchaplin/elasticsearch that referenced this pull request May 20, 2025
It's possible for another component to request a S3 client after the
node has started to shut down, and today the `S3Service` will dutifully
attempt to create a fresh client instance even if it is closed. Such
clients will then leak, resulting in test failures.

With this commit we refuse to create new S3 clients once the service has
started to shut down.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >non-issue Team:Distributed Coordination Meta label for Distributed Coordination team v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants