feat(client): add Android HTTP client and remove fallback in factory#846
feat(client): add Android HTTP client and remove fallback in factory#846sherryfox wants to merge 1 commit intoa2aproject:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Android-specific HTTP client implementation using HttpURLConnection, along with its corresponding SPI provider and tests. The A2AHttpClientFactory has been refactored to cache providers and throw an exception if none are available, and the base http-client module now provides a test-jar for shared integration tests. Feedback highlights a resource leak in asynchronous POST requests, a potential memory exhaustion vulnerability when reading server responses, and a violation of the SSE specification regarding whitespace trimming. Additionally, it is recommended to use constants for connection timeouts.
| while ((line = reader.readLine()) != null) { | ||
| if (sb.length() + line.length() > maxResponseSize) { |
There was a problem hiding this comment.
Using reader.readLine() can lead to OutOfMemoryError if the server sends a very long line without a newline character, as readLine() buffers the entire line. Per repository rules, explicit size validation is necessary to prevent Denial of Service (DoS) via OutOfMemoryError. It is safer to read the stream in chunks while maintaining a running count of the total bytes read to ensure the final in-memory representation does not exceed safety limits.
References
- When a class is designed to hold the entire content of a file in memory, an explicit size validation is necessary to prevent Denial of Service (DoS) via OutOfMemoryError for arbitrarily large files.
sherryfox
force-pushed
the
feat-android-http-client
branch
3 times, most recently
from
dc263d2 to
9f312d6
Compare
ehsavoie
left a comment
ehsavoie
left a comment
There was a problem hiding this comment.
The SSE part will be treated in another PR as ikt affects the JdkHttpClient as well
|
|
||
| @Override | ||
| public int priority() { | ||
| return 100; // Higher priority than JDK |
There was a problem hiding this comment.
Could you switch this priority as it collides with the one from vertx http client
There was a problem hiding this comment.
Added ANDROID_AVAILABLE to make it be highest priority only when running on Android, WDYT?
There was a problem hiding this comment.
nice :) this looks very good to me
| @Override | ||
| public PostBuilder body(String body) { | ||
| this.body = body; | ||
| return this; |
There was a problem hiding this comment.
it should return self(); shouldn't it ?
There was a problem hiding this comment.
AFAIK They do the same thing here. self() is only needed in the base class for generics. In the subclass, return this works perfectly and avoids a method call and cast.
- Port changes from PR 806 to main branch - Add AndroidA2AHttpClient using HttpURLConnection in a new extras module - Update A2AHttpClientFactory to fail fast if no provider is found - Add abstract test classes in http-client and concrete implementations in android module
sherryfox
force-pushed
the
feat-android-http-client
branch
from
9f312d6 to
534e4e5
Compare
2 participants
Description
Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
CONTRIBUTINGGuide.fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.Related to #793 🦕