Skip to content

Update HttpClientSseClientTransport.java #101

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.

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

Closed
wants to merge 2 commits into from

Conversation

chaoice
Copy link

@chaoice chaoice commented Apr 2, 2025

fix: switch HttpClient to HTTP/1.1 to prevent body loss with Python servers

This change modifies the HttpClient configuration to explicitly use HTTP_1_1 instead of the default HTTP_2, resolving a critical compatibility issue with Python-based servers (uvicorn/starlette) that cannot properly handle HTTP/2 upgrade requests with bodies.

The issue occurs because:

  1. Java's HttpClient attempts to upgrade to HTTP/2 (h2c) while simultaneously sending request bodies
  2. Python servers (particularly with HttpToolsProtocol) prioritize protocol upgrade handling, causing the body parsing to be skipped or interrupted
  3. This results in request bodies being lost during transmission

switch HttpClient to HTTP/1.1 to prevent body loss with Python servers

Motivation and Context

Java HTTP clients sending h2c protocol upgrade requests with bodies encounter a critical issue where uvicorn (Python ASGI server) silently ignores the request body, causing data loss.
image

Additional context

https://github.com/microsoft/markitdown/tree/main/packages/markitdown-mcp

fix: switch HttpClient to HTTP/1.1 to prevent body loss with Python servers

This change modifies the HttpClient configuration to explicitly use HTTP_1_1 instead 
of the default HTTP_2, resolving a critical compatibility issue with Python-based 
servers (uvicorn/starlette) that cannot properly handle HTTP/2 upgrade requests with 
bodies.

The issue occurs because:
1. Java's HttpClient attempts to upgrade to HTTP/2 (h2c) while simultaneously 
   sending request bodies
2. Python servers (particularly with HttpToolsProtocol) prioritize protocol upgrade 
   handling, causing the body parsing to be skipped or interrupted
3. This results in request bodies being lost during transmission
@tzolov
Copy link
Contributor

tzolov commented Apr 6, 2025

Hey @chaoice , thanks for highlighting this issue.

You can, already, change the HttpClient.Version to 1.1 like this:

HttpClientSseClientTransport clientTransport = HttpClientSseClientTransport.builder(baseUri)
		.clientBuilder(HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1))
		.build();

I don't think we need to hardcode it as the default value.

Perhaps we need to add documentation tip or warning.

Do you know if this issue affects the WebFluxSseClientTransport's default configuration too?

@tzolov tzolov added the client label Apr 6, 2025
@chaoice
Copy link
Author

chaoice commented Apr 7, 2025

Hey @chaoice , thanks for highlighting this issue.

You can, already, change the HttpClient.Version to 1.1 like this:

HttpClientSseClientTransport clientTransport = HttpClientSseClientTransport.builder(baseUri)
		.clientBuilder(HttpClient.newBuilder().version(HttpClient.Version.HTTP_1_1))
		.build();

I don't think we need to hardcode it as the default value.

Perhaps we need to add documentation tip or warning.

Do you know if this issue affects the WebFluxSseClientTransport's default configuration too?

Thank you for your suggestion. I'd like to provide some additional context about the protocol behavior:

  1. JDK's HttpClient defaults to HTTP/2 and attempts to upgrade from HTTP/1.1 to HTTP/2 via h2c. When sending an HTTP/1.1 request, it includes the upgrade headers along with the request body, which causes parsing issues in some Python servers.
  2. In contrast, WebFlux's default configuration uses HTTP/1.1 protocol. I've verified this through debugging and found that:
  • WebFlux's default client transport uses HTTP/1.1
  • The Cursor IDE's built-in server also defaults to HTTP/1.1
    This explains why the issue only manifests with JDK's HttpClient but not with WebFlux's implementation. Your suggestion to explicitly set HttpClient.Version.HTTP_1_1 is a good workaround for this specific case.
    While this solves the immediate problem, it might be valuable to document this behavior difference between JDK's HttpClient and WebFlux's client transport, especially for users who might be integrating with Python servers.
    Thank you for your guidance on this issue.

@tzolov
Copy link
Contributor

tzolov commented Apr 7, 2025

  1. JDK's HttpClient defaults to HTTP/2 and attempts to upgrade from HTTP/1.1 to HTTP/2 via h2c. When sending an HTTP/1.1 request, it includes the upgrade headers along with the request body, which causes parsing issues in some Python servers.
  2. In contrast, WebFlux's default configuration uses HTTP/1.1 protocol. I've verified this through debugging and found that:
  • WebFlux's default client transport uses HTTP/1.1
  • The Cursor IDE's built-in server also defaults to HTTP/1.1
    This explains why the issue only manifests with JDK's HttpClient but not with WebFlux's implementation. Your suggestion to explicitly set HttpClient.Version.HTTP_1_1 is a good workaround for this specific case.
    While this solves the immediate problem, it might be valuable to document this behavior difference between JDK's HttpClient and WebFlux's client transport, especially for users who might be integrating with Python servers.
    Thank you for your guidance on this issue.

Thank you for the clarification @chaoice! I agree that we should have a consistent default HTTP client version across all MCP clients. Let me circle back on this again.

@tzolov tzolov self-assigned this Apr 7, 2025
@tzolov tzolov added this to the 0.9.0 milestone Apr 7, 2025
@tzolov
Copy link
Contributor

tzolov commented Apr 9, 2025

@chaoice I believe the fab434c resolves this. Feel free to open another issue if the problem is still there.

@tzolov tzolov closed this Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants