Skip to content

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Jul 21, 2025

The current name is not very intuitive considering the class name and the module.

@Kludex Kludex requested review from a team, ochafik and ihrpr July 21, 2025 08:43
ihrpr
ihrpr previously requested changes Jul 21, 2025
Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

The deprecated wrapper is missing the httpx_client_factory and auth parameters. This could break existing code using those parameters.

(it's probably a good time to replace httpx_client_factory with an object)

@Kludex
Copy link
Member Author

Kludex commented Jul 21, 2025

I blame copilot. 😄

@Kludex
Copy link
Member Author

Kludex commented Jul 21, 2025

(it's probably a good time to replace httpx_client_factory with an object)

I think the timeouts, the auth and the httpx_client_factory CAN be replaced. The timeouts I think can be subjective, but the auth and the httpx_client_factory I think need to be replaced.

@ihrpr
Copy link
Contributor

ihrpr commented Jul 21, 2025

(it's probably a good time to replace httpx_client_factory with an object)

I think the timeouts, the auth and the httpx_client_factory CAN be replaced. The timeouts I think can be subjective, but the auth and the httpx_client_factory I think need to be replaced.

should we just replace it now, since we are deprecating one method, would be good to have a nice alternative and not to deprecate it twice?

@Kludex
Copy link
Member Author

Kludex commented Jul 21, 2025

(it's probably a good time to replace httpx_client_factory with an object)

I think the timeouts, the auth and the httpx_client_factory CAN be replaced. The timeouts I think can be subjective, but the auth and the httpx_client_factory I think need to be replaced.

should we just replace it now, since we are deprecating one method, would be good to have a nice alternative and not to deprecate it twice?

I didn't get what you said. I agree that it's a good moment to change the parameters of the new streamable_http_client.

@dsp-ant
Copy link
Member

dsp-ant commented Sep 2, 2025

Should we do this and deprecate the old module?

@felixweinberger felixweinberger self-assigned this Sep 22, 2025
@felixweinberger felixweinberger added the needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention label Sep 22, 2025
@felixweinberger
Copy link
Contributor

FYI for viewers: on my todo list to update this

@felixweinberger felixweinberger added the needs more work Not ready to be merged yet, needs additional changes. label Sep 23, 2025
@felixweinberger felixweinberger removed the needs more work Not ready to be merged yet, needs additional changes. label Oct 1, 2025
Kludex and others added 4 commits October 1, 2025 16:04
Modernize streamable_http_client API by accepting httpx.AsyncClient instances
directly instead of factory functions, following industry standards.

- New API: httpx_client: httpx.AsyncClient | None parameter
- Default client created with recommended timeouts if None
- Deprecated wrapper provides backward compatibility
- Updated examples to show custom client usage
- Add MCP_DEFAULT_TIMEOUT constants to _httpx_utils
@felixweinberger felixweinberger dismissed ihrpr’s stale review October 1, 2025 18:18

outdated - pushed updated version making httpx_client a param instead of factory

@felixweinberger felixweinberger requested review from ihrpr and removed request for ochafik and ihrpr October 1, 2025 18:24
This commit fixes all test failures introduced by the API change from
httpx_client_factory to direct httpx_client parameter:

1. Updated deprecated imports: Changed streamablehttp_client to
   streamable_http_client in test files

2. Fixed 307 redirect errors: Replaced httpx.AsyncClient with
   create_mcp_http_client which includes follow_redirects=True by default

3. Fixed test assertion: Updated test_session_group.py to mock
   create_mcp_http_client and verify the new API signature where
   streamable_http_client receives httpx_client parameter instead of
   individual headers/timeout parameters

4. Removed unused httpx import from main.py after inlining client creation

All tests now pass with the new API.
self.session_id = None
self.protocol_version = None
self.request_headers = {
**self.headers,
Copy link
Contributor

@felixweinberger felixweinberger Oct 1, 2025

Choose a reason for hiding this comment

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

⚠️ Important change 1/2 ⚠️

Previously we would have streamablehttp_client create the httpx.AsyncClient via a factory after initializing the transport. That means we could use these transport.request_headers when creating the httpx.AsyncClient to ensure the client and the transport have the same headers.

If we now accept httpx_client as an argument to streamable_http_client, that client might have custom headers! In fact by default httpx.AsyncClient creates headers here that need to be overriden (hence moving the ACCEPT and CONTENT_TYPE after)

Therefore the transport needs to override these headers now if they're present to be configured correctly.

Added missing type annotation imports that were causing NameError and
preventing test collection:

- RequestResponder from mcp.shared.session
- SessionMessage from mcp.shared.message
- GetSessionIdCallback from mcp.client.streamable_http
- RequestContext from mcp.shared.context

This fixes 4 NameError collection failures and 10 F821 ruff errors,
allowing all 20 tests in the file to be properly collected and executed.
Restore type annotations on test function parameters in
test_streamable_http.py that were accidentally removed during the
function renaming from streamablehttp_client to streamable_http_client.

Added type annotations to:
- Fixture parameters: basic_server, basic_server_url,
  json_response_server, json_server_url, event_server, monkeypatch
- Test function parameters: initialized_client_session

This fixes all 61 pyright errors and ensures type safety matches
the main branch standards.
Remove complex mocking of create_mcp_http_client in the streamablehttp
test case. Instead, let the real create_mcp_http_client execute and
only verify that streamable_http_client receives the correct parameters
including a real httpx.AsyncClient instance.

This simplifies the test by:
- Removing 13 lines of mock setup code
- Removing 14 lines of mock verification code
- Removing 3 lines of mock cleanup code
- Trusting that create_mcp_http_client works (it has its own tests)

The test now focuses on verifying the integration between session_group
and streamable_http_client rather than re-testing create_mcp_http_client.
Comment on lines +507 to +508
# Sync client headers with transport's merged headers (includes MCP protocol requirements)
client.headers.update(transport.request_headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Important change 2/2 ⚠️

We need to sync the headers on the client back so that they match with the transport after the transport has overriden them. If the user passes in a httpx_client they created, that could have arbitrary headers that no longer match the headers on the transport.

Overall I find this quite horrible - but I'm not sure how else to make this work via an object instead of a factory. Open for suggestions...

@felixweinberger
Copy link
Contributor

felixweinberger commented Oct 1, 2025

@Kludex @ihrpr @dsp-ant gave this a shot, trying to replace the factory with a client object.

Results in some ugly header mangling that I don't see how to avoid given the existing structure. Keen for thoughts.

@felixweinberger felixweinberger requested a review from ihrpr October 1, 2025 19:05
@felixweinberger felixweinberger added needs more eyes Needs alignment among maintainers whether this is something we want to add and removed needs maintainer action Potentially serious issue - needs proactive fix and maintainer attention labels Oct 1, 2025
@felixweinberger felixweinberger changed the title Rename streamablehttp_client to streamable_http_client Add streamable_http_client which accepts httpx.AsyncClient instead of httpx_client_factory Oct 1, 2025
# Handle timeout
if timeout is None:
kwargs["timeout"] = httpx.Timeout(30.0)
kwargs["timeout"] = httpx.Timeout(MCP_DEFAULT_TIMEOUT, read=MCP_DEFAULT_SSE_READ_TIMEOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously we'd rely on the transport setting this to 60 * 5 when creating our client, but we can't rely on that anymore as the client may now be created before the transport.

from mcp.client.auth import OAuthClientProvider, TokenStorage
from mcp.client.streamable_http import streamablehttp_client
from mcp.client.streamable_http import streamable_http_client
from mcp.shared._httpx_utils import create_mcp_http_client
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should have private methods being exposed in examples. 🤔

timeout: float | timedelta = 30,
sse_read_timeout: float | timedelta = 60 * 5,
*,
httpx_client: httpx.AsyncClient | None = None,
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
httpx_client: httpx.AsyncClient | None = None,
http_client: httpx.AsyncClient | None = None,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs more eyes Needs alignment among maintainers whether this is something we want to add
Projects
None yet
Development

Successfully merging this pull request may close these issues.

httpx.ConnectError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:1002)
4 participants