Skip to content

fix: replace raw SSE string handling with SSEEvent model and SSEParser#842

Open
ehsavoie wants to merge 1 commit intoa2aproject:0.3.xfrom
ehsavoie:issue_841
Open

fix: replace raw SSE string handling with SSEEvent model and SSEParser#842
ehsavoie wants to merge 1 commit intoa2aproject:0.3.xfrom
ehsavoie:issue_841

Conversation

@ehsavoie
Copy link
Copy Markdown
Collaborator

Introduce SSEEvent (data, id, event type, retry) and a spec-compliant SSEParser with buffer and line-length guards. Update postAsyncSSE and all SSE listeners across JDK, Android, JSONRPC, and REST transports to consume SSEEvent instead of raw strings. Using proper SSE Events

Fixes #841 🦕

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 introduces a structured SSEEvent class and a dedicated SSEParser to handle Server-Sent Events across JSON-RPC, REST, and Android transport layers, replacing simple string consumers. The feedback identifies several areas in the SSEParser where strict adherence to the SSE specification is missing, specifically regarding state management during event dispatch, the handling of null characters in event IDs, and the requirement to ignore non-numeric retry fields. Additionally, a suggestion was made to improve the parser's resilience to Denial of Service attacks by clearing the buffer upon overflow.

Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java
Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java
Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java
Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java
@kabir
Copy link
Copy Markdown
Collaborator

kabir commented Apr 30, 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 introduces a dedicated SSEEvent model and an SSEParser to improve Server-Sent Events (SSE) handling across the client and transport layers. The changes replace simple string-based data extraction with a spec-compliant parser that supports multi-line data, event types, IDs, and retry intervals. Corresponding updates were made to A2AHttpClient implementations and transport listeners to consume SSEEvent objects. A review comment suggests refining the retry field parsing in SSEParser to strictly follow the SSE specification by ignoring non-ASCII digit values instead of triggering an error handler.

Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java Outdated
@ehsavoie ehsavoie force-pushed the issue_841 branch 3 times, most recently from 76c86d7 to e847ad4 Compare April 30, 2026 14:10
@ehsavoie
Copy link
Copy Markdown
Collaborator Author

/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 introduces a structured SSEEvent class and a dedicated SSEParser to handle Server-Sent Events according to the specification, replacing simple string-based consumers across the HTTP client and transport layers. While the implementation significantly improves SSE handling, feedback suggests several refinements to the SSEParser for better robustness and spec compliance. Specifically, the data buffer should be cleared when line length limits are exceeded to prevent event corruption, and the event ID should persist across subsequent events in a stream. Furthermore, the error handling strategy for buffer overflows could be improved by discarding all fields until the next event boundary to ensure data integrity.

Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java
Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java Outdated
Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java
Introduce SSEEvent (data, id, event type, retry) and a spec-compliant
SSEParser with buffer and line-length guards. Update postAsyncSSE and
all SSE listeners across JDK, Android, JSONRPC, and REST transports to
consume SSEEvent instead of raw strings. Using proper SSE Events

Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
@ehsavoie
Copy link
Copy Markdown
Collaborator Author

/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 introduces a structured SSEEvent class and a dedicated SSEParser to replace raw string handling for Server-Sent Events, ensuring better compliance with the SSE specification. The changes update the A2AHttpClient interface and its implementations, along with the corresponding transport layers and listeners. Feedback suggests using Long.parseLong for the retry field to avoid potential overflows and refining the buffer size limit calculation in SSEParser to account for newline characters added during event dispatch.

Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java
Comment thread http-client/src/main/java/io/a2a/client/http/SSEParser.java
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.

2 participants