Skip to content

Fix non-OK HTTP POST responses handling #374

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 4 commits into
base: main
Choose a base branch
from
Open

Fix non-OK HTTP POST responses handling #374

wants to merge 4 commits into from
+165 −80

Conversation

chemicL
Copy link
Member

@chemicL chemicL commented Jul 4, 2025

Currently, JDK HttpClient SSE implementation incorrectly handles a non-2xx HTTP response code for POST. Instead of immediately failing the caller, it ignores the issue and the user awaits until the timeout kicks in.

A related problem happened with the WebClient Streamable HTTP implementation, where the error was swallowed for non-400 responses.

@chemicL
Fix non-OK HTTP POST responses handling
420aaf2 
Currently, JDK HttpClient SSE implementation incorrectly handles a
non-2xx HTTP response code for POST. Instead of immediately failing the
caller, it ignores the issue and the user awaits until the timeout kicks
in.

A related problem happened with the WebClient Streamable HTTP
implementation, where the error was swallowed for non-400 responses.

Signed-off-by: Dariusz Jędrzejczyk <dariusz.jedrzejczyk@broadcom.com>
@chemicL
Copy link
Member Author

chemicL commented Jul 4, 2025

@tzolov I noticed this change fails some cases of HttpClientSseClientTransportTests, which seem to be incorrect, as the server returns a 400 HTTP code due to incorrect input.

Also, I noticed the HttpClientSseClientTransportTests.TestHttpClientSseClientTransport has a Sink which is never consumed. I don't quite get how these tests are working.

I'll leave this in a draft state for you to investigate.

@tzolov
refactor: improve SSE client transpor testing
d99df2b 
- Extract eventStream() method
- Enhance connect() method with improved SSE event processing and error handling
- Introduce MyResponseInfo record and enhanced TestHttpClientSseClientTransport
- Add collections to track JSON-RPC requests, notifications, and responses

Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
@tzolov tzolov marked this pull request as ready for review July 8, 2025 16:26
@tzolov
Copy link
Contributor

tzolov commented Jul 8, 2025

@chemicL , I fixed the tests. The 400 was due to the HTTP version.
I also changed the implementation & tests to make use of the events sink, similar to the WebFluxSseClientTransportTests. Even added a test handler to track the passing messages.
But it seems that the simulateMessageEvent is useless for both the WebFluxSseClientTransportTests and the HttpClientSseClientTransportTests

tzolov added 2 commits July 8, 2025 21:04
@tzolov
clean
dc37f97 
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
@tzolov
clean
bd5699f 
Signed-off-by: Christian Tzolov <christian.tzolov@broadcom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chemicL @tzolov