Skip to content

Make KoogHttpClient auto closable and add clientName #1184

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Make KoogHttpClient auto closable and add clientName #1184

wants to merge 3 commits into from

Conversation

@tiginamaria
Copy link
Collaborator

@tiginamaria tiginamaria commented Nov 25, 2025 •
edited
Loading

Motivation and Context

Address comments from #1179

  • Make KoogHttpClient auto closable
  • Rollback renames in AnthropicLLMClient model

Address comments from #1181

  • Add clientName toKoogHttpClient to represent ktor/okhttp/java

Breaking Changes

Type of the changes

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tests improvement
  • Refactoring

Checklist

  • The pull request has a description of the proposed change
  • I read the Contributing Guidelines before opening the pull request
  • The pull request uses develop as the base branch
  • Tests for the changes have been added
  • All new and existing tests passed
Additional steps for pull requests adding a new feature
  • An issue describing the proposed change exists
  • The pull request includes a link to the issue
  • The change was discussed and approved in the issue
  • Docs have been added / updated

@tiginamaria
Address Anthropic client fixes
28a8148 
Make KoogHttpClient auto closable
Rollback renames
@tiginamaria tiginamaria force-pushed the tigina/anthropic-http-client-fixes branch from d61a561 to 23a0237 Compare November 25, 2025 16:17
@tiginamaria tiginamaria force-pushed the tigina/anthropic-http-client-fixes branch from 23a0237 to 33321b0 Compare November 25, 2025 16:50
@github-actions
Copy link

github-actions bot commented Nov 25, 2025

Qodana for JVM

1137 new problems were found

Inspection name Severity Problems
Check Kotlin and Java source code coverage 🔶 Warning 1126
Missing KDoc for public API declaration 🔶 Warning 11 
@@ Code coverage @@
+ 72% total lines covered
16659 lines analyzed, 12063 lines covered
# Calculated according to the filters of your coverage tool

☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

kpavlov
kpavlov reviewed Nov 26, 2025
View reviewed changes
Copy link
Contributor

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

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

Thanks a bunch, @tiginamaria! It’s almost there, but let’s keep the client name parameter and add @EncodeDefault for safety.

@kpavlov kpavlov added the refactoring Code improvements that enhance structure, readability, and quality without changing functionality label Nov 26, 2025
devcrocod
devcrocod reviewed Nov 26, 2025
View reviewed changes
Copy link
Contributor

@devcrocod devcrocod left a comment

Choose a reason for hiding this comment

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

lgtm

I have one question about closing httpClient

http-client/http-client-java/src/main/kotlin/ai/koog/http/client/java/JavaKoogHttpClient.kt
}
}

override fun close() {
Copy link
Contributor

@devcrocod devcrocod Nov 26, 2025

Choose a reason for hiding this comment

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

Why isn’t the httpClient being closed here?

Copy link
Collaborator Author

@tiginamaria tiginamaria Nov 27, 2025

Choose a reason for hiding this comment

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

I haven't found a way how to do it, that's why =))) The only way I found is (httpClient.executor().getOrNull() as? ExecutorService)?.shutdown(), is it correct?

Copy link
Contributor

@kpavlov kpavlov Nov 28, 2025

Choose a reason for hiding this comment

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

In Java 21, the HttpClient is AutoCloseable, but in Java 17, it is not. If the client is using a custom Executor, it should be closed manually. But it's not the case.

kpavlov
kpavlov approved these changes Nov 28, 2025
View reviewed changes
Copy link
Contributor

@kpavlov kpavlov left a comment

Choose a reason for hiding this comment

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

LGTM

http-client/http-client-java/src/main/kotlin/ai/koog/http/client/java/JavaKoogHttpClient.kt
}
}

override fun close() {
Copy link
Contributor

@kpavlov kpavlov Nov 28, 2025

Choose a reason for hiding this comment

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

In Java 21, the HttpClient is AutoCloseable, but in Java 17, it is not. If the client is using a custom Executor, it should be closed manually. But it's not the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kpavlov kpavlov kpavlov approved these changes

@antoniibelyshev antoniibelyshev Awaiting requested review from antoniibelyshev

@devcrocod devcrocod Awaiting requested review from devcrocod

Assignees

No one assigned

Labels

refactoring Code improvements that enhance structure, readability, and quality without changing functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tiginamaria @kpavlov @devcrocod