Skip to content

feat(http-client): introduce ServerSentEvent type and spec-compliant SSE parser#853

Merged
kabir merged 2 commits into
a2aproject:mainfrom
ehsavoie:issue-839
May 12, 2026
Merged

feat(http-client): introduce ServerSentEvent type and spec-compliant SSE parser#853
kabir merged 2 commits into
a2aproject:mainfrom
ehsavoie:issue-839

Conversation

@ehsavoie
Copy link
Copy Markdown
Collaborator

@ehsavoie ehsavoie commented May 6, 2026

Replace raw string SSE callbacks with a structured ServerSentEvent record across
all HTTP client implementations (JDK, Vert.x, Android). Add ServerSentEventParser
with full SSE spec compliance (multi-line data, event id/type, retry, DoS limits).
Expand integration tests for typed SSE events and add Android transport variants
for existing JSONRPC and REST reference tests.

Fixes #839 🦕

@ehsavoie ehsavoie requested review from jmesnil and kabir May 6, 2026 15:20
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the SSE handling mechanism by introducing a dedicated ServerSentEvent record and a unified ServerSentEventParser across the JDK, Android, and Vert.x HTTP client implementations. The A2AHttpClient interface and its consumers have been updated to use this new event structure. Additionally, a deadlock issue in Vert.x routes was addressed by disabling ordered execution for blocking handlers. Review feedback highlights that non-SSE responses are incorrectly delivered line-by-line in several client implementations, which could break JSON parsing for multi-line bodies. There is also a concern regarding the ServerSentEventParser dispatching pending data upon flushing, which violates the SSE specification's requirement to discard such data when a stream ends mid-event.

I am having trouble creating individual review comments. Click here to see my feedback.

http-client/src/main/java/org/a2aproject/sdk/client/http/JdkA2AHttpClient.java (161-163)

high

Delivering non-SSE bodies line-by-line to the messageConsumer will cause parsing failures in SSEEventListener if the response (e.g., a JSON error) is multi-line or pretty-printed. For non-SSE responses, the entire body should be buffered and delivered as a single ServerSentEvent to ensure it can be parsed correctly as a single JSON object.

extras/http-client-vertx/src/main/java/org/a2aproject/sdk/client/http/VertxA2AHttpClient.java (613-615)

high

Delivering non-SSE bodies line-by-line to the messageConsumer will cause parsing failures in SSEEventListener if the response is multi-line JSON. Similar to the implementation for error bodies in AndroidA2AHttpClient, the entire non-SSE body should be buffered and delivered as a single ServerSentEvent.

extras/http-client-android/src/main/java/org/a2aproject/sdk/client/http/AndroidA2AHttpClient.java (188-192)

high

Delivering non-SSE bodies line-by-line to the messageConsumer will cause parsing failures in SSEEventListener if the response is multi-line JSON. While error bodies are correctly handled as a single block at line 167, 2xx non-SSE responses are still split by line here. It is recommended to buffer the entire body and deliver it as a single ServerSentEvent.

http-client/src/main/java/org/a2aproject/sdk/client/http/ServerSentEventParser.java (171-173)

medium

The SSE specification (Section 9.2.6) states that "Once the end of the file is reached, any pending data must be discarded." Dispatching the buffered event on flush() deviates from this requirement. This also introduces inconsistent behavior across client implementations, as those relying on standard-compliant parsers (like Vert.x) will not dispatch a partial event if the stream ends without a trailing empty line.

    public void flush() {
        // Per SSE spec: any pending data must be discarded if the stream ends mid-event
        dataBuffer.clear();
        dataBufferChars = 0;
        eventType = null;
        skippingCurrentEvent = false;
    }

@ehsavoie ehsavoie force-pushed the issue-839 branch 2 times, most recently from 14a480f to bc9e6e7 Compare May 7, 2026 06:07
@ehsavoie
Copy link
Copy Markdown
Collaborator Author

ehsavoie commented May 7, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the SSE handling logic across the SDK to use a unified ServerSentEvent record and a dedicated ServerSentEventParser. This change standardizes SSE processing, improves error handling, and introduces support for SSE fields like event types, IDs, and retry intervals. Additionally, the PR adds support for Android and Vert.x HTTP clients and includes necessary configuration updates for testing. My review highlights critical improvements needed for error propagation in the JDK client and security enhancements to prevent potential Denial of Service vulnerabilities through response body size limiting.

Comment thread http-client/src/main/java/org/a2aproject/sdk/client/http/JdkA2AHttpClient.java Outdated
Comment thread http-client/src/main/java/org/a2aproject/sdk/client/http/JdkA2AHttpClient.java Outdated
@kabir
Copy link
Copy Markdown
Collaborator

kabir commented May 12, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the Server-Sent Events (SSE) handling across the SDK by replacing raw string message passing with a structured ServerSentEvent record. A new ServerSentEventParser has been introduced to provide consistent, spec-compliant parsing for the JDK, Android, and Vert.x HTTP client implementations. Furthermore, server-side routes in the Quarkus reference implementations were updated to use non-ordered blocking handlers to avoid deadlocks during request delegation. Feedback was provided to ensure the ServerSentEventParser correctly handles null characters in the id field according to the SSE specification.

Comment on lines +121 to +127
case "id" -> {
// Per SSE spec: ignore the id field if the value contains a U+0000 NULL character.
// An empty value is valid and clears the last event ID buffer on dispatch.
if (value.indexOf('\0') == -1) {
currentEventId = value;
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

According to the SSE specification, if the id field value contains a U+0000 NULL character, the event ID buffer must be set to the empty string, not ignored. The current implementation leaves the buffer unchanged if a null character is present.

        case "id" -> {
            // Per SSE spec: if the value contains a U+0000 NULL character, set the event ID buffer to the empty string.
            // An empty value is valid and clears the last event ID buffer on dispatch.
            if (value.indexOf('\0') != -1) {
                currentEventId = "";
            } else {
                currentEventId = value;
            }
        }

ehsavoie and others added 2 commits May 12, 2026 14:19
…SSE parser

Replace raw string SSE callbacks with a structured ServerSentEvent record across
  all HTTP client implementations (JDK, Vert.x, Android). Add ServerSentEventParser
  with full SSE spec compliance (multi-line data, event id/type, retry, DoS limits).
Expand integration tests for typed SSE events and add Android transport variants
  for existing JSONRPC and REST reference tests.

This fixes a2aproject#839

Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kabir kabir merged commit e693005 into a2aproject:main May 12, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve SSE implementation

2 participants