OpenAIClient: implement close()

[stargazer33]

The com.openai.client.OpenAIClient has no close() method.
Please implement it!
Better: implement AutoClosable

Additional info:
com.openai.client.okhttp.OkHttpClient has public void close()

Running my code using OpenAIClient I get (sometimes) this text in log:

okhttp3.OkHttpClient           A connection to <url> was leaked. Did you forget to close a response body? To see where this was allocated, set the OkHttpClient logger level to FINE: Logger.getLogger(OkHttpClient.class.getName()).setLevel(Level.FINE);

It can be that my code is not correct: having OpenAIClient.close() would prevent this.

[TomerAberbach]

Which service methods are you using by the way? I'd be interested to understand if we're missing closing a response body somewhere in the SDK.

That seems not entirely related to making OpenAIClient implement AutoCloseable. I don't think we'd want to do that because various IDEs and linters would yell at users for not using try-with-resources, which you usually don't need for something as long-lived as the HTTP client.

I do think having a close method makes sense though (just not through AutoCloseable). We do something similar for AsyncStreamResponse (but for slightly different reasons):

openai-java/openai-java-core/src/main/kotlin/com/openai/core/http/AsyncStreamResponse.kt

Lines 15 to 21 in fb0d4a6

	/**
	* Closes this resource, relinquishing any underlying resources.
	*
	* This is purposefully not inherited from [AutoCloseable] because this response should not be
	* synchronously closed via try-with-resources.
	*/
	fun close()

[stargazer33]

yes, just the close() without AutoClosable will be OK too. Please implement it.

Which service methods are you using by the way? I'd be interested to understand

I would like to understand it too! Bloody async. code, a bit difficult to debug ))

The general idea:
There is a list of "Sessions".
Each Session is a wrapper around OpenAIClient
In case of SOME exceptions thrown by OpenAIClient - HTTP 429 is typical example - the corresponding Session "paused" and a timer set to resume the session in 20 sec.
This timer introduce async. behavior.
And, additionally, god knows what happens inside OpenAIClient - still there is no JavaDoc describing it - I assume there are some threads inside this class, you have to describe all this - this is important

[TomerAberbach]

You didn't answer my question about which methods you're using

[stargazer33]

Methods used:

OpenAIOkHttpClient.Builder clientBldr = OpenAIOkHttpClient.builder();

OpenAIClient client;
...
client = clientBldr
    .baseUrl(opts.baseUrl)
    .credential(BearerTokenCredential.create(apiKey))
    .timeout(Duration.ofMinutes(3))
    .maxRetries(0)
    .build();
...
ChatCompletion chatCompl = client.chat().completions().create( ... );

for (ChatCompletion.Choice choice : chatCompl.choices() ) {
    choice.message().content().get()
    ...
}

Wait! In the past I was using maxRetries(1) and saw A connection to <url> was leaked ...
May be these thing related... I will watch it...

[TomerAberbach]

I stared at the code a bit today and I think we're not properly closing failed responses in all cases when retrying

Planning to write some more tests and fix it this upcoming week

[stargazer33]

A good thing would be to change the log level of this message:

okhttp3.OkHttpClient           A connection to <url> was leaked. Did you forget to close a response body? To see where this was allocated, set the OkHttpClient logger level to FINE: Logger.getLogger(OkHttpClient.class.getName()).setLevel(Level.FINE);

One of the reasons I do not see it any more - I do not run my application with log level set to FINE.
FINE for this class means - a lot of HTTP headers and the complete request+response bodies.
This is too much!

You are controlling everything related to okhttp3.OkHttpClient - pls change the log level to WARNING