-
Notifications
You must be signed in to change notification settings - Fork 244
fix: clear WorkloadIdentityAuth refresh state after synchronous failures #854
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,9 +30,59 @@ import java.util.concurrent.Executor | |
| import java.util.concurrent.ExecutorService | ||
| import java.util.concurrent.Executors | ||
| import java.util.concurrent.ThreadFactory | ||
| import java.util.concurrent.atomic.AtomicBoolean | ||
| import java.util.concurrent.atomic.AtomicLong | ||
| import kotlin.jvm.optionals.getOrNull | ||
|
|
||
| private class ClientOptionsResource(private val close: () -> Unit) { | ||
| private var references = 1 | ||
| private var closed = false | ||
|
|
||
| @Synchronized | ||
| fun retain() { | ||
| check(!closed) { "Cannot retain a closed client resource" } | ||
| references++ | ||
| } | ||
|
|
||
| fun release() { | ||
| val shouldClose = | ||
| synchronized(this) { | ||
| check(references > 0) { "Client resource released too many times" } | ||
| references-- | ||
| if (references == 0) { | ||
| closed = true | ||
| true | ||
| } else false | ||
| } | ||
|
|
||
| if (shouldClose) close() | ||
| } | ||
| } | ||
|
|
||
| private class ClientOptionsResources( | ||
| val httpClient: ClientOptionsResource, | ||
| val httpRequestAuthenticator: ClientOptionsResource?, | ||
| val workloadIdentityAuth: ClientOptionsResource?, | ||
| val streamHandlerExecutor: ClientOptionsResource, | ||
| val sleeper: ClientOptionsResource, | ||
| ) { | ||
| fun release() { | ||
| httpRequestAuthenticator?.release() | ||
| workloadIdentityAuth?.release() | ||
| httpClient.release() | ||
| streamHandlerExecutor.release() | ||
| sleeper.release() | ||
| } | ||
| } | ||
|
|
||
| private class ClientOptionsCloseAction(private val resources: ClientOptionsResources) : () -> Unit { | ||
| private val closed = AtomicBoolean(false) | ||
|
|
||
| override fun invoke() { | ||
| if (closed.compareAndSet(false, true)) resources.release() | ||
| } | ||
| } | ||
|
|
||
| /** A class representing the SDK client configuration. */ | ||
| class ClientOptions | ||
| private constructor( | ||
|
|
@@ -141,12 +191,16 @@ private constructor( | |
| private val organization: String?, | ||
| private val project: String?, | ||
| private val webhookSecret: String?, | ||
| private val resources: ClientOptionsResources, | ||
| ) { | ||
|
|
||
| private val closeAction = ClientOptionsCloseAction(resources) | ||
|
|
||
| init { | ||
| if (checkJacksonVersionCompatibility) { | ||
| checkJacksonVersionCompatibility() | ||
| } | ||
| closeWhenPhantomReachable(this, closeAction) | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -217,6 +271,11 @@ private constructor( | |
| private var project: String? = null | ||
| private var webhookSecret: String? = null | ||
| private var workloadIdentity: WorkloadIdentity? = null | ||
| private var httpClientResource: ClientOptionsResource? = null | ||
| private var httpRequestAuthenticatorResource: ClientOptionsResource? = null | ||
| private var workloadIdentityAuthResource: ClientOptionsResource? = null | ||
| private var streamHandlerExecutorResource: ClientOptionsResource? = null | ||
| private var sleeperResource: ClientOptionsResource? = null | ||
|
|
||
| @JvmSynthetic | ||
| internal fun from(clientOptions: ClientOptions) = apply { | ||
|
|
@@ -226,6 +285,11 @@ private constructor( | |
| jsonMapper = clientOptions.jsonMapper | ||
| streamHandlerExecutor = clientOptions.streamHandlerExecutor | ||
| sleeper = clientOptions.sleeper | ||
| httpClientResource = clientOptions.resources.httpClient | ||
| httpRequestAuthenticatorResource = clientOptions.resources.httpRequestAuthenticator | ||
| workloadIdentityAuthResource = clientOptions.resources.workloadIdentityAuth | ||
| streamHandlerExecutorResource = clientOptions.resources.streamHandlerExecutor | ||
| sleeperResource = clientOptions.resources.sleeper | ||
|
Comment on lines
+288
to
+292
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a builder returned by Useful? React with 👍 / 👎. |
||
| clock = clientOptions.clock | ||
| baseUrl = clientOptions.baseUrl | ||
| headers = clientOptions.headers.toBuilder() | ||
|
|
@@ -256,6 +320,7 @@ private constructor( | |
| */ | ||
| fun httpClient(httpClient: HttpClient) = apply { | ||
| this.httpClient = PhantomReachableClosingHttpClient(httpClient) | ||
| this.httpClientResource = null | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -269,6 +334,7 @@ private constructor( | |
| this.httpRequestAuthenticator = | ||
| if (httpRequestAuthenticator == null) null | ||
| else PhantomReachableClosingHttpRequestAuthenticator(httpRequestAuthenticator) | ||
| this.httpRequestAuthenticatorResource = null | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -302,6 +368,7 @@ private constructor( | |
| if (streamHandlerExecutor is ExecutorService) | ||
| PhantomReachableExecutorService(streamHandlerExecutor) | ||
| else streamHandlerExecutor | ||
| this.streamHandlerExecutorResource = null | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -313,7 +380,10 @@ private constructor( | |
| * | ||
| * This class takes ownership of the sleeper and closes it when closed. | ||
| */ | ||
| fun sleeper(sleeper: Sleeper) = apply { this.sleeper = PhantomReachableSleeper(sleeper) } | ||
| fun sleeper(sleeper: Sleeper) = apply { | ||
| this.sleeper = PhantomReachableSleeper(sleeper) | ||
| this.sleeperResource = null | ||
| } | ||
|
|
||
| /** | ||
| * The clock to use for operations that require timing, like retries. | ||
|
|
@@ -393,6 +463,7 @@ private constructor( | |
| fun apiKey(apiKey: String?) = apply { | ||
| this.apiKey = apiKey | ||
| this.credential = apiKey?.let { BearerTokenCredential.create(it) } | ||
| this.workloadIdentityAuthResource = null | ||
| } | ||
|
|
||
| /** Alias for calling [Builder.apiKey] with `apiKey.orElse(null)`. */ | ||
|
|
@@ -406,6 +477,7 @@ private constructor( | |
| fun credential(credential: Credential) = apply { | ||
| this.apiKey = null | ||
| this.credential = credential | ||
| this.workloadIdentityAuthResource = null | ||
| } | ||
|
|
||
| fun azureServiceVersion(azureServiceVersion: AzureOpenAIServiceVersion) = apply { | ||
|
|
@@ -434,6 +506,7 @@ private constructor( | |
|
|
||
| fun workloadIdentity(workloadIdentity: WorkloadIdentity?) = apply { | ||
| this.workloadIdentity = workloadIdentity | ||
| this.workloadIdentityAuthResource = null | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When this builder was created from options that use workload identity, Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| /** Alias for calling [Builder.workloadIdentity] with `workloadIdentity.orElse(null)`. */ | ||
|
|
@@ -701,6 +774,31 @@ private constructor( | |
| val effectiveWorkloadIdentityAuth = | ||
| (credential as? WorkloadIdentityCredential)?.getAuth() | ||
|
|
||
| val resources = | ||
| ClientOptionsResources( | ||
| httpClient = | ||
| httpClientResource?.also { it.retain() } | ||
| ?: ClientOptionsResource { httpClient.close() }, | ||
| httpRequestAuthenticator = | ||
| httpRequestAuthenticatorResource?.also { it.retain() } | ||
| ?: httpRequestAuthenticator?.let { | ||
| ClientOptionsResource { it.close() } | ||
| }, | ||
| workloadIdentityAuth = | ||
| workloadIdentityAuthResource?.also { it.retain() } | ||
| ?: effectiveWorkloadIdentityAuth?.let { | ||
| ClientOptionsResource { it.close() } | ||
| }, | ||
| streamHandlerExecutor = | ||
| streamHandlerExecutorResource?.also { it.retain() } | ||
| ?: ClientOptionsResource { | ||
| (streamHandlerExecutor as? ExecutorService)?.shutdown() | ||
| }, | ||
| sleeper = | ||
| sleeperResource?.also { it.retain() } | ||
| ?: ClientOptionsResource { sleeper.close() }, | ||
| ) | ||
|
|
||
| val loggingDelegate = | ||
| if (httpRequestAuthenticator != null) httpClient | ||
| else | ||
|
|
@@ -756,6 +854,7 @@ private constructor( | |
| organization, | ||
| project, | ||
| webhookSecret, | ||
| resources, | ||
| ) | ||
| } | ||
| } | ||
|
|
@@ -770,11 +869,7 @@ private constructor( | |
| * releases threads and connections if they remain idle, but if you are writing an application | ||
| * that needs to aggressively release unused resources, then you may call this method. | ||
| */ | ||
| fun close() { | ||
| httpClient.close() | ||
| (streamHandlerExecutor as? ExecutorService)?.shutdown() | ||
| sleeper.close() | ||
| } | ||
| fun close() = closeAction() | ||
|
|
||
| @JvmSynthetic | ||
| internal fun securityHeaders(security: SecurityOptions): Headers { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a caller starts an async request or obtains an
AsyncStreamResponseand then drops the client/service before the future or stream is finished, this cleaner can run solely because theClientOptionsobject is unreachable. The returned async work still uses the underlying HTTP client, retry sleeper, or stream-handler executor, but those are now closed bycloseAction; for exampleAsyncStreamResponse.toAsync()submits stream consumption toclientOptions.streamHandlerExecutor, so shutting it down beforesubscribe()can leave the stream callbacks unscheduled. Tie cleanup to the outstanding returned work as well, or avoid closing shared resources from theClientOptionscleaner while such objects may still own them.Useful? React with 👍 / 👎.