Skip to content

Conversation

agronholm
Copy link

  • Removed the asyncio-only parametrization of the anyio_backend except for test_ws, as websockets doesn't support Trio yet
  • Try to close async generators explicitly where possible
  • Changed nesting order for more predictable closing of async resources
  • Refactored __aenter__ and __aexit__ in some cases to exit the task group if there's a problem during initialization
  • Fixed test failures in client/test_auth.py where an async fixture was used in sync tests
  • Fixed subtle bug in SimpleEventStore where retrieving the stream ID was timing-dependent
  • Added typing-extensions to dependencies, as the code already imported it unconditionally

Motivation and Context

This was requested at https://github.com/anthropics/oss-collab/issues/60.

How Has This Been Tested?

No.

Breaking Changes

No.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

* Removed the asyncio-only parametrization of the anyio_backend except for test_ws, as `websockets` doesn't support Trio yet
* Try to close async generators explicitly where possible
* Changed nesting order for more predictable closing of async resources
* Refactored `__aenter__` and `__aexit__` in some cases to exit the task group if there's a problem during initialization
* Fixed test failures in client/test_auth.py where an async fixture was used in sync tests
* Fixed subtle bug in `SimpleEventStore` where retrieving the stream ID was timing-dependent
pyproject.toml Outdated
Comment on lines 127 to 129
# This is to avoid test failures on Trio due to httpx's failure to explicitly close
# async generators
"ignore::pytest.PytestUnraisableExceptionWarning"
Copy link
Member

Choose a reason for hiding this comment

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

Can we solve those? Why this doesn't happen on asyncio?

Copy link
Author

Choose a reason for hiding this comment

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

Trio considers implicit async generator finalization a bad practice and emits a warning. Pytest turns this into an unraisable exception warning.

Copy link
Author

Choose a reason for hiding this comment

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

The only way to fix this is to fix the problem in httpcore.

Copy link
Member

Choose a reason for hiding this comment

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

How would the fix in httpcore look like?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is not resource warning anyway, I'm okay with it. I read it wrongly.

Copy link
Author

@agronholm agronholm Jun 25, 2025

Choose a reason for hiding this comment

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

The httpcore PR is ready, just waiting for review.

EDIT: the PR has been accepted, waiting for merge.

Copy link
Author

Choose a reason for hiding this comment

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

The httpx PR: encode/httpx#3593

Copy link
Author

Choose a reason for hiding this comment

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

Everything seems to fall into place now. With all my local changes applied, the test suite runs flawlessly on Trio.

Copy link

Choose a reason for hiding this comment

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

Great work!

Copy link
Author

Choose a reason for hiding this comment

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

The httpx PR is now passing all tests with 100% coverage. Waiting for a review and merging.

Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

I found another issue in the test suite where I got a RuntimeError: Task got bad yield: <class 'trio._core._traps.CancelShieldedCheckpoint'>. This turned out to be caused by multiprocess forking the process for running a test server, where the sniffio contextvar was inherited from the parent. I fixed that by using spawning instead of forking.

The only things standing in the way of this passing are:

Hi both, thank you for working on this!

Just catching up here on the current state, looks like we're still waiting on these items to be able to land this PR?

Let me know if I can help with any of these!

@felixweinberger felixweinberger requested a review from ihrpr July 15, 2025 15:56
@felixweinberger felixweinberger added pending dependency updates This change depends on updates to dependencies and removed pending dependency updates This change depends on updates to dependencies labels Sep 17, 2025
Copy link
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

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

Marking this as "pending dependency changes" for now - looks like encode/httpx#3593 may have been closed while encode/httpcore#1019 is still pending?

Keen to support where needed to get this improvement landed! I've commented on encode/httpcore#1019 to see if there's anything else needed as it looks approved and able to merge?

@felixweinberger felixweinberger added the pending dependency updates This change depends on updates to dependencies label Sep 17, 2025
@agronholm
Copy link
Author

agronholm commented Sep 17, 2025

Marking this as "pending dependency changes" for now - looks like encode/httpx#3593 may have been closed while encode/httpcore#1019 is still pending?

Keen to support where needed to get this improvement landed! I've commented on encode/httpcore#1019 to see if there's anything else needed as it looks approved and able to merge?

httpcore is abandonware now that httpx 1.0 is under development. I don't expect to ever see another release of httpcore.

@felixweinberger
Copy link
Contributor

felixweinberger commented Sep 17, 2025

Marking this as "pending dependency changes" for now - looks like encode/httpx#3593 may have been closed while encode/httpcore#1019 is still pending?
Keen to support where needed to get this improvement landed! I've commented on encode/httpcore#1019 to see if there's anything else needed as it looks approved and able to merge?

httpcore is abandonware now that httpx 1.0 is under development. I don't expect to ever see another release of httpcore.

Gotcha, thanks for the response! Looking at the discussion here https://github.com/anthropics/oss-collab/issues/60 it sounds like we'd need to update this repo to httpx 1.0 whenever it lands to be able to ship this change?

That seems like it might require some bigger changes / refactors to the SDK, tentatively earmarking that for v2 for now as it seems like it would introduce some breaking changes potentially.

@felixweinberger felixweinberger added v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes needs more eyes Needs alignment among maintainers whether this is something we want to add labels Sep 17, 2025
@felixweinberger
Copy link
Contributor

felixweinberger commented Sep 26, 2025

Converting this to "Draft" for now as I believe we're blocked on the httpx 1.0 upgrade and to remove this one from the team's review queue while we await that.

@felixweinberger felixweinberger marked this pull request as draft September 26, 2025 14:41
@felixweinberger felixweinberger added this to the Python SDK v2.0 milestone Sep 30, 2025
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 pending dependency updates This change depends on updates to dependencies v2 Ideas, requests and plans for v2 of the SDK which will incorporate major changes and fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants