Response connection is None on 3.9.0 about aiohttp HOT 37 CLOSED

Unfortunately, thats not a lot to go on.

Any chance that you can bisect to find the regression?

from aiohttp.

I fail building it with:
cc1: fatal error: aiohttp/_websocket.c: No such file or directory
Is there a building documentation?

from aiohttp.

#79f5266518b58e1a778450c9b03bf337a7e01901 is the first bad commit

from aiohttp.

So it would seem that it’s likely that fixing #7864 will also fix this problem as well. The good news is that one has a reproducer so it will be a bit easier to track down.

from aiohttp.

Just noticed you linked a PR. Will try that with #7864

from aiohttp.

Looks like #7864 still fails with that change so something more is going on

from aiohttp.

Looks like #7864 still fails with that change so something more is going on

from aiohttp.

My problem is not related to #7864

from aiohttp.

I tried the current master (after #7869) and the issue still exists.
Removing await self._wait_released() from read solves the issue on the current master also.
Why does read need to release the connection?

from aiohttp.

Finally was able to reproduce without aiodocker (by copying the interesting parts).
First you need to create a docker container (I used the official Ubuntu one) and replace container_id with your container id.

import json

import aiohttp
import asyncio

async def main():
    container_id = 'abaf180acb95e32cc60ed699a99b5756ef8a48c22a7d92cffdf98d30ef7b59f2'
    async with aiohttp.ClientSession(connector=aiohttp.UnixConnector('/run/docker.sock')) as session:
        response = await session.request(
            'POST', 
            f'unix://localhost/v1.43/containers/{container_id}/exec',
            headers={'Content-Type': 'application/json'},
            data=json.dumps({
                'Container': container_id, 
                'Privileged': False, 'Tty': False, 'AttachStdin': False, 'AttachStdout': True, 
                'AttachStderr': True,  'Cmd': ['ls'], 'Env': None, 'WorkingDir': '', 'detachKeys': '', 'User': ''
            })
        )
        exec_id = (await response.json())['Id']
        response = await session.request(
            'POST', 
            f'unix://localhost/v1.43/exec/{exec_id}/start',
            headers={'Content-Type': 'application/json', 'Connection': 'Upgrade', 'Upgrade': 'tcp'},
            data=json.dumps({'Detach': False, 'Tty': False}),
            read_until_eof=False
        )
        data = await response.read()
        conn = response.connection
        print(conn)

asyncio.run(main())

The print(conn) should not print None but Connection<ConnectionKey(host='localhost', port=None, is_ssl=False, ssl=None, proxy=None, proxy_auth=None, proxy_headers_hash=None)>

from aiohttp.

I might be wrong, but I just realised 3.9.0 might have broken the all Connection: Upgrade header support

from aiohttp.

Why does read need to release the connection?

I believe it is documented behaviour that has always existed. There is no reason not to release the connection if we have finished reading from it.

The change in 3.9 is more subtle, it now waits for the writer to complete before releasing the connection. It's possible a subtle change in that logic was overlooked and causing some issue.

from aiohttp.

I might be wrong, but I just realised 3.9.0 might have broken the all Connection: Upgrade header support

So is that mean upgrading a connection is no longer available (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Upgrade)?

from aiohttp.

Shouldn't be, there are several tests for upgrade which are still passing.

from aiohttp.

Finally was able to reproduce without aiodocker (by copying the interesting parts). First you need to create a docker container (I used the official Ubuntu one) and replace container_id with your container id.

import json

import aiohttp
import asyncio

async def main():
    container_id = 'abaf180acb95e32cc60ed699a99b5756ef8a48c22a7d92cffdf98d30ef7b59f2'
    async with aiohttp.ClientSession(connector=aiohttp.UnixConnector('/run/docker.sock')) as session:
        response = await session.request(
            'POST', 
            f'unix://localhost/v1.43/containers/{container_id}/exec',
            headers={'Content-Type': 'application/json'},
            data=json.dumps({
                'Container': container_id, 
                'Privileged': False, 'Tty': False, 'AttachStdin': False, 'AttachStdout': True, 
                'AttachStderr': True,  'Cmd': ['ls'], 'Env': None, 'WorkingDir': '', 'detachKeys': '', 'User': ''
            })
        )
        exec_id = (await response.json())['Id']
        response = await session.request(
            'POST', 
            f'unix://localhost/v1.43/exec/{exec_id}/start',
            headers={'Content-Type': 'application/json', 'Connection': 'Upgrade', 'Upgrade': 'tcp'},
            data=json.dumps({'Detach': False, 'Tty': False}),
            read_until_eof=False
        )
        data = await response.read()
        conn = response.connection
        print(conn)

asyncio.run(main())

The print(conn) should not print None but Connection<ConnectionKey(host='localhost', port=None, is_ssl=False, ssl=None, proxy=None, proxy_auth=None, proxy_headers_hash=None)>

So I guess I am using the API incorrectly (and so the author of aiodocker), can you please help me with understanding what I am missing?

from aiohttp.

There may still be a regression, it's just probably a more subtle edge case than you thought. It'd go a long way if you can add a test in a PR which reproduces the issue.

from aiohttp.

Will a test using docker (same as the code above) will be ok? Can you please refer me to the current tests?

from aiohttp.

Probably not, we don't have docker in the CI, and we have downstream packagers who need to run tests without docker.

Test would probably go in
https://github.com/aio-libs/aiohttp/blob/master/tests/test_client_request.py
or
https://github.com/aio-libs/aiohttp/blob/master/tests/test_client_functional.py

from aiohttp.

I tried writing a test. but since I am not familiar with your code it might take me a couple of weeks, your help would really be appreciated (a release without breaking the API but with python3.12 support would also be appreciated)

from aiohttp.

The print(conn) should not print None but Connection<ConnectionKey(host='localhost', port=None, is_ssl=False, ssl=None, proxy=None, proxy_auth=None, proxy_headers_hash=None)>

Wait, I missed this. This should print None, because the connection is closed and removed. Why do you think it shouldn't be? This should happen on older releases too.

from aiohttp.

In 3.8.6 it keeps the connection and aiodocker uses this to upgrade the connection to tcp

from aiohttp.

In 3.8.6 it keeps the connection and aiodocker uses this to upgrade the connection to tcp

We have tests on 3.8 that say it shouldn't:

I think what you want to be printing is client.connector._conns. There should be a connection open there after the read().

from aiohttp.

Wait a moment, I didn't see read_until_eof=False. Maybe this is the problem. Let me have a look at how that's supposed to work...

from aiohttp.

It seems to me that if you use that parameter, then resp.read() is always going to return an empty value, if I understand the code correctly. Not really sure how that's useful, but the fix seems simple, we just need to skip the wait_released when that setting is False.

from aiohttp.

From a quick look I saw that response is not accessible to the read_until_eof setting, do you prefer adding it yourself or shall I add it to response class and open a PR?

from aiohttp.

I'm just sorting out a test at the moment. I think that attribute is accessible, but will get the test sorted first.

from aiohttp.

Hmm, struggling to get a test that validates. Do you know what the server response looks like?

This fails on 3.8:

async def test_no_eof_response_not_released(aiohttp_client) -> None:
    async def handler(request):
        body = await request.read()
        assert b"" == body
        return web.Response(body=b"OK" * 2000)

    app = web.Application()
    app.router.add_route("GET", "/", handler)

    client = await aiohttp_client(app)

    resp = await client.get("/")
    await resp.read()
    assert resp.connection is None
    resp = await client.get("/", read_until_eof=False)
    await resp.read()
    assert resp.connection is not None

from aiohttp.

I am not sure, but when printing raw_headers and the data I get:

((b'Content-Type', b'application/vnd.docker.multiplexed-stream'), (b'Connection', b'Upgrade'), (b'Upgrade', b'tcp'), (b'Api-Version', b'1.43'), (b'Docker-Experimental', b'false'), (b'Ostype', b'linux'), (b'Server', b'Docker/24.0.7 (linux)'))
b''

from aiohttp.

Hmm, I can only get it to fail or to hang...

from aiohttp.

Do you know what the server response looks like?

@matan1008
You can run the code with

strace -f -s 4096 -o calls.log <your script>

Than you can extract the raw read/write calls from the calls.log file to see whats actually being sent

from aiohttp.

This is the request:

POST /v1.43/exec/206c8c5da12eda3c5e9b68435dd81bd3aa8a010cd73ed1305e6a3b090d2deba5/start HTTP/1.1
Host: localhost
Content-Type: application/json
Connection: Upgrade
Upgrade: tcp
Accept: */*
Accept-Encoding: gzip, deflate
User-Agent: Python/3.11 aiohttp/3.8.6
Content-Length: 31

And this is the response:

HTTP/1.1 101 UPGRADED
Content-Type: application/vnd.docker.multiplexed-stream
Connection: Upgrade
Upgrade: tcp
Api-Version: 1.43
Docker-Experimental: false
Ostype: linux
Server: Docker/24.0.7 (linux)

from aiohttp.

OK, it's completely unrelated to that parameter (which I still don't understand what it does). Should have a fix shortly.

from aiohttp.

Nope, I take it back. This test reproduces without extensions:

async def test_upgrade_connection_not_released_after_read(aiohttp_client: Any) -> None:
    async def handler(request: web.Request) -> web.Response:
        body = await request.read()
        assert b"" == body
        return web.Response(status=101, headers={"Connection": "Upgrade"})

    app = web.Application()
    app.router.add_route("GET", "/", handler)

    client = await aiohttp_client(app)

    resp = await client.get("/")
    await resp.read()
    assert resp.connection is not None

But, still fails with extensions. Is it possible you've installed a version without wheels, or otherwise disabled the extensions?
(You can check at runtime by checking what aiohttp.http_parser.HttpResponseParser is, by printing it or whatever. The compiled one is aiohttp._http_parser.HttpResponseParser (with _), the Python one is aiohttp.http_parser.HttpResponseParser).

from aiohttp.

I installed it regularly with pip (through aiodocker)

from aiohttp.

Ah, I think I was right previously. It seems that the C parser only considers the connection upgraded when the Upgrade header is present, whereas the Py parser doesn't care if that header is present. Probably a bug to fix in the Py parser there.

If you can test the change in the PR to double check it solves the problem for you, that'd be great.

from aiohttp.

Will do tomorrow morning, thanks!

from aiohttp.

Works great, Thank you!

from aiohttp.