Add Pyodide/Emscripten client support via FetchConnector (continuation of #7803)#13057
Add Pyodide/Emscripten client support via FetchConnector (continuation of #7803)#13057bendichter wants to merge 4 commits into
Conversation
FetchConnector dispatches requests through the JavaScript fetch() API, decoding the serialized request with HttpRequestParser and re-serializing the fetch() response into HTTP/1.1 bytes for the standard ResponseHandler, so ClientSession needs no platform-specific changes beyond connector selection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- tests/test_fetch_connector.py: unit tests for the whole fetch pipeline using an injected stub fetch(), runnable on any platform. - tests/test_pyodide.py: pytest-pyodide integration tests running the built wasm wheel in Node.js against a local HTTP server. - CI: test-pyodide job building the cp313 Emscripten wheel with cibuildwheel and running the integration tests with --rt node. - aiohttp/_llhttp_wasm_shim.c: no-op definitions for llhttp's JavaScript-oriented wasm_on_* callbacks so the C extensions load under Emscripten. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
psf-chronographer
Bot
added
the
bot:chronographer:provided
for more information, see https://pre-commit.ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13057 +/- ##
========================================
Coverage 98.96% 98.96%
========================================
Files 131 133 +2
Lines 48156 48469 +313
Branches 2499 2514 +15
========================================
+ Hits 47656 47968 +312
Misses 376 376
- Partials 124 125 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
- Add 'js' (from Node.js) to the docs spelling wordlist. - Exclude tests/test_pyodide.py from codecov (it only executes inside the Pyodide runtime, where coverage is not collected). - Add unit tests covering the remaining FetchConnector lines: platform default-connector selection, large-body flow control, transports writelines/abort/double-close, fetch abort on close, and responses without Headers.getSetCookie(). - Sanitize the echoed Content-Type in the test HTTP server (CodeQL response-splitting warning). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
If you've picked up from that code, could you actually fork from that branch and keep the original commits? Otherwise attribution to the original author is missing, which is needed for copyright handling. |
| self._abort_controller = None | ||
| if self._fetch_task is not None and not self._fetch_task.done(): | ||
| self._fetch_task.cancel() | ||
| if not self._connection_lost_called: |
There was a problem hiding this comment.
Just looking through the coverage. Currently no test reaches the situation that connection_lost_called is true. There are also 3 no cover comments below which I'm not clear why they shouldn't be covered?
There was a problem hiding this comment.
Two separate things here.
The 3 # pragma: no cover: all guard the IS_EMSCRIPTEN path — from js import ... and AbortController. Only importable inside Pyodide. Normal CI runs on CPython, cannot reach them. Real coverage comes from test_pyodide.py.
The _connection_lost_called branch: dead in this flow. _FetchTransport.close() already guards via _closing, and nothing else calls connection_lost(), so the flag is always False. Drop the guard (call unconditionally) or add # pragma: no branch.
There was a problem hiding this comment.
Ideally this whole file should be excluded from coverage when tests are run natively and it should have to be covered in the Pyodide tests. See here for run_in_pyodide_coverage used in httpx2:
https://github.com/pydantic/httpx2/pull/1022/changes#diff-5e8856b9064bafa3712edb7239b9ff3da7d898b2178e2b4bcb3bd7dfcbe9c355R10
Each test makes it's own separate coverage file and they have to be combined. There's some tricky details about fixing up the file paths:
https://github.com/pydantic/httpx2/pull/1022/changes#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R89
There was a problem hiding this comment.
Each test makes it's own separate coverage file and they have to be combined. There's some tricky details about fixing up the file paths:
That code looks like it just ends up calling coverage as normal:
https://github.com/pyodide/pytest-pyodide/blob/25f4c87780b984bfe44c8bd54ef2081e52071de7/pytest_pyodide/_decorator_in_pyodide.py#L153
So, I'd assume that if the correct arguments are passed, it should be fine. Maybe it makes more sense to run coverage directly ourselves (I'm unclear why the normal method of executing coverage wouldn't work here?). Otherwise, I'd expect it to still be possible to configure it such that it just appends to the same file as we do elsewhere (e.g.
aiohttp/.github/workflows/ci-cd.yml
Line 271 in 7f2ffa6
|
This is substantial PR, so I'll loop back round to it before the 3.15 release. @aiolibsbot review |
Valid copyright concern. CHANGES credit alone does not preserve authorship. Recommend rebasing this work onto #7803's branch so @hoodmane's original commits stay in history, then layer the new changes on top. |
PR Review — Add Pyodide/Emscripten client support via FetchConnector (continuation of #7803)Solid, well-insulated design; blocked mainly on a redirect-behavior gap and the outstanding attribution/copyright request. Strengths worth calling out:
Needs attention:
🟡 Important
1. `allow_redirects=False` / `max_redirects` are silently ignored
|
|
|
||
| __all__ = ("FetchConnector",) | ||
|
|
||
| IS_EMSCRIPTEN: Final = sys.platform == "emscripten" |
There was a problem hiding this comment.
Shouldn't this whole file be emscripten only?
|
|
||
| # NOTE: makefile cythonizes all Cython modules | ||
|
|
||
| if USE_SYSTEM_DEPS: |
There was a problem hiding this comment.
Should we fail or ignore it if USE_SYSTEM_DEPS and sys.platform == "emscripten"?
| # JavaScript-provided wasm_on_* callbacks; provide no-op stubs so the | ||
| # extension has no undefined symbols (aiohttp uses llhttp_init() with | ||
| # its own callbacks instead). | ||
| if sys.platform == "emscripten" or "emscripten" in os.environ.get( |
There was a problem hiding this comment.
You should only need sys.platform == "emcripten". But also, the entirety of _llhttp_wasm_shim.c is wrapped in an #ifdef __wasm__, so you could include it unconditionally.
| # When targeting Emscripten (Pyodide), llhttp's api.c expects | ||
| # JavaScript-provided wasm_on_* callbacks; provide no-op stubs so the | ||
| # extension has no undefined symbols (aiohttp uses llhttp_init() with | ||
| # its own callbacks instead). |
There was a problem hiding this comment.
Comment seems redundant with the comment in __llhttp_wasm_shim.c, maybe just keep the one there.
| # The run_in_pyodide decorator comes from pytest.importorskip() because | ||
| # pytest-pyodide is only installed for the dedicated Pyodide CI job. | ||
| [mypy-test_pyodide] | ||
| disallow_any_decorated = False | ||
| disallow_untyped_decorators = False |
There was a problem hiding this comment.
Does it fix this type error to install pytest-pyodide in the mypy job?
| [mypy-js] | ||
| ignore_missing_imports = True | ||
|
|
||
| [mypy-pyodide.*] | ||
| ignore_missing_imports = True |
There was a problem hiding this comment.
Does it fix these type errors to install pyodide-py in the mypy job?
| submodules: true | ||
| - name: Setup Python ${{ matrix.pyver }} | ||
| id: python-install | ||
| # important: do not use system python |
There was a problem hiding this comment.
Would be nice to know why it's important.
| PYODIDE_VERSION: 0.29.3 | ||
| strategy: | ||
| matrix: | ||
| pyver: ["cp313"] |
There was a problem hiding this comment.
It would be great to also build and test for Pyodide 314.0.2 / Python 314.
| - name: Install test dependencies | ||
| # aiohttp itself is needed on the host because tests/conftest.py | ||
| # imports it; the pure-Python build is sufficient. | ||
| run: AIOHTTP_NO_EXTENSIONS=1 uv pip install -e . pytest-pyodide==0.59.2 pytest pytest-aiohttp pytest-timeout coverage |
There was a problem hiding this comment.
Why do we pin pytest-pyodide? I would think we'd either want to pin all the deps or only ones that actually need a pin.
|
Thanks @bendichter for working on this! |
| @@ -0,0 +1,365 @@ | |||
| """Tests for aiohttp.pyodide.FetchConnector using a stubbed fetch(). | |||
There was a problem hiding this comment.
It would be really nice to delete the mocks and get coverage working in the real Pyodide tests, since I think any code path that is not exercised with the real fetch is unreliable.
| def test_get_json(selenium_with_aiohttp: Any, echo_server_url: str) -> None: | ||
| _get_json(selenium_with_aiohttp, echo_server_url) |
There was a problem hiding this comment.
Inline the test bodies:
| def test_get_json(selenium_with_aiohttp: Any, echo_server_url: str) -> None: | |
| _get_json(selenium_with_aiohttp, echo_server_url) | |
| @run_in_pyodide | |
| def test_get_json(selenium_with_aiohttp: Any, echo_server_url: str) -> None: | |
| # ... actual test code here |
| try: | ||
| await session.get(base_url + "/missing", raise_for_status=True) | ||
| except aiohttp.ClientResponseError as e: | ||
| assert e.status == 404 | ||
| else: | ||
| raise AssertionError("expected ClientResponseError") |
| try: | ||
| await session.get("http://127.0.0.1:2/") | ||
| except aiohttp.ClientConnectionError: | ||
| pass | ||
| else: | ||
| raise AssertionError("expected ClientConnectionError") |
| enable-cache: true | ||
| - name: Install build tooling and cython | ||
| run: | | ||
| uv pip install -U pip wheel setuptools build twine -r requirements/cython.in -c requirements/cython.txt |
There was a problem hiding this comment.
I don't see where we use wheel, setuptools, build, or twine?
| uv pip install -U pip wheel setuptools build twine -r requirements/cython.in -c requirements/cython.txt | |
| uv pip install -U pip -r requirements/cython.in -c requirements/cython.txt |
| run: uv pip install cibuildwheel==3.4.1 | ||
| - name: Build wheel | ||
| # See the comment in test-mobile about unsetting GITHUB_ACTIONS. | ||
| run: env -u GITHUB_ACTIONS cibuildwheel --output-dir dist |
There was a problem hiding this comment.
@henryiii should cibuildwheel have some explicit way to opt out of folding?
cibuildwheel normally uses grouping in its outputs for its build/test steps. But the loading time when expanding large groups in GitHub Actions is very high. So by unsetting GITHUB_ACTIONS, cibuildwheel does not know that it is running in a GitHub Action and thus does not use groups.
5 participants
What do these changes do?
Adds client support for running under Pyodide / Emscripten (WebAssembly in browsers and Node.js), picking up where #7803 left off. I'd really like to be able to use aiohttp from Pyodide (e.g. pyodide/pyodide#4998), and since #7803 invited someone to continue the work, I took a stab at it.
Full disclosure up front: AI (Claude) was used heavily to write this. I've reviewed it and tested it end-to-end and it looks reasonable to me, but I'm very aware this could be more noise than help for maintainers — if it isn't useful, please feel free to close it, no hard feelings. If the overall direction is right, I'm happy to keep working on it based on your feedback.
Design
Following the suggestion in #7803 (comment) (encapsulate the logic in a custom connector so
_request()needs minimal changes), everything lives in a newaiohttp/pyodide.pymodule providingFetchConnector, whichClientSessionselects as the default connector whensys.platform == "emscripten"— that default selection is the only change to the client flow.Rather than subclassing
ClientRequest/ClientResponse(as #7803 did against the 3.x codebase), the connector uses the HTTP/1.1 wire format as its interface:HttpRequestParser(so cookies, auto-added headers, and chunked bodies are all handled by existing code),fetch()API,ResponseHandler/HttpResponseParser.The fetch-specific details handled: framing/hop-by-hop headers are stripped in both directions (fetch delivers bodies already decompressed/unframed, so e.g. a stale
Content-Encodingwould corrupt parsing), repeatedSet-Cookieheaders are recovered viaHeaders.getSetCookie(), cancellation/timeouts abort the fetch viaAbortController,Expect: 100-continueis answered locally, and proxies /CONNECT/ upgrades (WebSockets) raise a clearClientConnectionError.Two side discoveries:
FetchConnector(fetch=...)accepts an injected fetch implementation, so nearly the whole pipeline is unit-testable on regular CPython without a WebAssembly runtime (tests/test_fetch_connector.py) — this should help with the coverage concerns from ENH Add Pyodide support #7803.api.creferences JavaScript-providedwasm_on_*callbacks whenever__wasm__is defined, leaving undefined symbols in the extension.aiohttp/_llhttp_wasm_shim.cprovides no-op definitions (aiohttp usesllhttp_init()with its own callbacks and never touches that path).Testing
tests/test_fetch_connector.py: 18 unit tests with a stubbedfetch()(GET/POST/chunked async-generator bodies, header filtering both directions, multiSet-Cookie→ cookie jar, 204/HEAD, error propagation,expect100,fetch_options, timeout-cancels-fetch, concurrency). These run in the normal test matrix.tests/test_pyodide.py: pytest-pyodide integration tests that run the built wasm wheel (with compiled C extensions) in Node.js against a live local HTTP server — real POSTs, redirects, cookies, 404s, concurrency, and connection errors. Run by the newtest-pyodideCI job (cibuildwheel--platform pyodide, modeled on thetest-mobilejob).raise_for_status— plainClientSession()+session.get(...)works unmodified.Are there changes in behavior for the user?
None on existing platforms. Under Emscripten/Pyodide (previously unusable —
getaddrinforaisesNotImplementedError),ClientSessionnow works out of the box viafetch(). Platform limitations are documented indocs/client_reference.rst: no proxies or WebSockets, redirects are followed transparently byfetch()before aiohttp sees the response,sslarguments are ignored, and browsers enforce their own cookie/CORS policies.Is it a substantial burden for the maintainers to support this?
I've tried to minimize it: the feature is a single self-contained module plus a 2-line default-connector selection; no existing code paths change. Because it talks to the rest of the client through the HTTP wire format and the public parser/protocol interfaces, it should be fairly insulated from internal refactors. The main ongoing costs are the extra CI job (one Ubuntu runner building a cp313 wasm wheel) and keeping the pinned Pyodide/cibuildwheel versions fresh. That said, you're the best judges of this — and if the answer is "yes, too much", closing this is a completely fair outcome.
Related issue number
Related to #7253 and continues #7803 (original work by @hoodmane, who is credited in the CHANGES fragment). Also relevant to pyodide/pyodide#4998.
Checklist
CONTRIBUTORS.txtCHANGES/folder🤖 Generated with Claude Code