fix: clear WorkloadIdentityAuth refresh state after synchronous failures - #854
fix: clear WorkloadIdentityAuth refresh state after synchronous failures#854abhinavkr26104 wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a80385468c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| fun workloadIdentity(workloadIdentity: WorkloadIdentity?) = apply { | ||
| this.workloadIdentity = workloadIdentity | ||
| this.workloadIdentityAuthResource = null |
There was a problem hiding this comment.
When this builder was created from options that use workload identity, from() keeps the existing WorkloadIdentityCredential in credential. If a caller then does toBuilder().workloadIdentity(null).build(), this line drops the retained workloadIdentityAuthResource even though the old credential is still selected, so the derived options create a fresh owner for the same WorkloadIdentityAuth; closing that derived options/client can close the shared provider, such as the K8s token reader executor, while the original options/client is still active. Clear the workload-identity credential when nulling the config, or keep retaining the existing resource when the credential is unchanged.
Useful? React with 👍 / 👎.
| httpClientResource = clientOptions.resources.httpClient | ||
| httpRequestAuthenticatorResource = clientOptions.resources.httpRequestAuthenticator | ||
| workloadIdentityAuthResource = clientOptions.resources.workloadIdentityAuth | ||
| streamHandlerExecutorResource = clientOptions.resources.streamHandlerExecutor | ||
| sleeperResource = clientOptions.resources.sleeper |
There was a problem hiding this comment.
When a builder returned by toBuilder() is kept after the source ClientOptions becomes unreachable, these copied resource handles are not retained until build() runs. The new cleaner on ClientOptions can therefore release and close the shared handles while the builder is still alive; a later builder.build() will either throw Cannot retain a closed client resource or clone options backed by resources that have already been closed. Retain these handles in from() and release them if the builder replaces them, or otherwise make the builder keep the source options/resources alive until build time.
Useful? React with 👍 / 👎.
| if (checkJacksonVersionCompatibility) { | ||
| checkJacksonVersionCompatibility() | ||
| } | ||
| closeWhenPhantomReachable(this, closeAction) |
There was a problem hiding this comment.
If a caller starts an async request or obtains an AsyncStreamResponse and then drops the client/service before the future or stream is finished, this cleaner can run solely because the ClientOptions object is unreachable. The returned async work still uses the underlying HTTP client, retry sleeper, or stream-handler executor, but those are now closed by closeAction; for example AsyncStreamResponse.toAsync() submits stream consumption to clientOptions.streamHandlerExecutor, so shutting it down before subscribe() can leave the stream callbacks unscheduled. Tie cleanup to the outstanding returned work as well, or avoid closing shared resources from the ClientOptions cleaner while such objects may still own them.
Useful? React with 👍 / 👎.
1 participant
Summary
refreshInFlightFixes #852
Verification
git diff --checkpassed