Skip to content
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

Broken in with yarl 1.13.0+ working in 3.10.7+, contains test case: aiohttp parses port numbers as part of the host (ValueError: Host x.x.0.0:8080 cannot contain ':') #9360

Closed
1 task done
doctorpangloss opened this issue Sep 30, 2024 · 3 comments
Labels

Comments

@doctorpangloss
Copy link

Describe the bug

Broken in 3.10.6, working in 3.10.8 - maybe this needs to be a regression test case.

Making a request to an aiohttp server which contains an address like http://10.3.39.66:9090 causes an unexpectedly flakey interaction between a middleware and web_request. For some reason, the host is being parsed to contain the port name too.

To Reproduce

# uv pip freeze | grep aiohttp
aiohttp==3.10.6
# uv pip freeze | grep yarl
yarl==1.13.1

This test case reproduces with those versions:

import asyncio
import pytest
import subprocess
from aiohttp import web

async def health_check(request):
    return web.Response(text="HEALTHY")

@web.middleware
async def middleware(request, handler):
    # Access request.url.path to trigger the potential error
    print(f"Accessing path: {request.url.path}")
    response = await handler(request)
    return response

async def run_server():
    app = web.Application(middlewares=[middleware])
    app.router.add_get('/health', health_check)
    runner = web.AppRunner(app)
    await runner.setup()
    site = web.TCPSite(runner, 'localhost', 9090)
    await site.start()
    print("Server started on http://localhost:9090")
    return runner

@pytest.mark.asyncio
async def test_health_check():
    runner = await run_server()
    try:
        # Use asyncio.create_subprocess_exec to run curl command
        proc = await asyncio.create_subprocess_exec(
            'curl', '-s', 'http://localhost:9090/health',
            stdout=asyncio.subprocess.PIPE,
            stderr=asyncio.subprocess.PIPE)

        try:
            stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=5.0)
        except asyncio.TimeoutError:
            print("Curl request timed out")
            proc.kill()
            await proc.wait()
            return

        if proc.returncode != 0:
            print(f"Curl failed with return code {proc.returncode}")
            print(f"stderr: {stderr.decode()}")
        else:
            response = stdout.decode().strip()
            assert response == "HEALTHY", f"Unexpected response: {response}"
            print("Test passed: Received 'HEALTHY' response")
    finally:
        await runner.cleanup()

In 3.10.8, it does not.

Expected behavior

I'm not sure why this breaks sometimes in different aiohttp versions so maybe this should be added as a test case.

Logs/tracebacks

10.3.39.66 [30/Sep/2024:11:09:51 -0800] "GET /health HTTP/1.1" 500 246 "-" "kube-probe/1.29"
Error handling request
Traceback (most recent call last):
  File "C:\Python311\Lib\site-packages\aiohttp\web_protocol.py", line 477, in _handle_request
    resp = await request_handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python311\Lib\site-packages\aiohttp\web_app.py", line 559, in _handle
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python311\Lib\site-packages\aiohttp\web_middlewares.py", line 117, in impl
    return await handler(request)
           ^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python311\Lib\site-packages\comfy\vendor\aiohttp_server_instrumentation.py", line 196, in middleware
    or _excluded_urls.url_disabled(request.url.path)
                                   ^^^^^^^^^^^
  File "aiohttp\\_helpers.pyx", line 30, in aiohttp._helpers.reify.__get__
  File "C:\Python311\Lib\site-packages\aiohttp\web_request.py", line 457, in url
    url = URL.build(scheme=self.scheme, host=self.host)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python311\Lib\site-packages\yarl\_url.py", line 385, in build
    netloc = cls._make_netloc(
             ^^^^^^^^^^^^^^^^^
  File "C:\Python311\Lib\site-packages\yarl\_url.py", line 1057, in _make_netloc
    ret = cls._encode_host(host)
          ^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python311\Lib\site-packages\yarl\_url.py", line 1038, in _encode_host
    _host_validate(host)
  File "C:\Python311\Lib\site-packages\yarl\_url.py", line 1607, in _host_validate
    raise ValueError(
ValueError: Host '10.3.39.72:9090' cannot contain ':' (at position 10)

Python Version

$ python --version
Python 3.11.7

aiohttp Version

$ python -m pip show aiohttp
$ python -m pip show aiohttp
Name: aiohttp
Version: 3.10.6
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author:
Author-email:
License: Apache 2
Location: C:\Users\bberman\Documents\appmana\.venv\Lib\site-packages
Requires: aiohappyeyeballs, aiosignal, attrs, frozenlist, multidict, yarl
Required-by: aiobotocore, comfyui, datasets, facebook_business, pytest-aiohttp, s3fs

multidict Version

$ python -m pip show multidict
Name: multidict
Version: 6.1.0
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache 2
Location: C:\Users\bberman\Documents\appmana\.venv\Lib\site-packages
Requires:
Required-by: aiohttp, yarl

yarl Version

$ python -m pip show yarl
Name: yarl
Version: 1.13.1
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl
Author: Andrew Svetlov
Author-email: [email protected]
License: Apache-2.0
Location: C:\Users\bberman\Documents\appmana\.venv\Lib\site-packages
Requires: idna, multidict
Required-by: aio-pika, aiohttp, aiormq, gql

OS

Windows 2022

Related component

Server

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@bdraco
Copy link
Member

bdraco commented Sep 30, 2024

This looks like a duplicate of #9307 which was fixed via #9309

@doctorpangloss
Copy link
Author

Appears so, I even looked at this PR and it was hard to see it was fixed! Thank you for pointing this out.

@bdraco bdraco changed the title Broken in 3.10.6, working in 3.10.8, contains test case: aiohttp parses port numbers as part of the host (ValueError: Host x.x.0.0:8080 cannot contain ':') Broken in with yarl 1.13.0+ working in 3.10.7+, contains test case: aiohttp parses port numbers as part of the host (ValueError: Host x.x.0.0:8080 cannot contain ':') Sep 30, 2024
@bdraco
Copy link
Member

bdraco commented Sep 30, 2024

In case it makes a difference in how you deploy.

The breakage wasn't a regression in 3.10.6, as it was a bug in how aiohttp built the URL with yarl, and has existed for a long time.

It wasn't until yarl 1.13.0 added validation via aio-libs/yarl#954 that the error was caught and corrected in 3.10.7+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants