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

Response connection is None on 3.9.0 #7867

Closed
1 task done
matan1008 opened this issue Nov 22, 2023 · 37 comments · Fixed by #7879
Closed
1 task done

Response connection is None on 3.9.0 #7867

matan1008 opened this issue Nov 22, 2023 · 37 comments · Fixed by #7879
Labels

Comments

@matan1008
Copy link

matan1008 commented Nov 22, 2023

Describe the bug

Sorry for the not easily reproducible bug, but since 3.9.0 connection upgrading doesn't work

To Reproduce

I experienced the bug using aiodocker exec, so forgive me for indirect reproduce of the bug:

  • get a running docker container in aiodocker
  • exec_ = await container.exec(command) (the command doesn't matter, can be ls)
  • async with exec_.start(detach=False) as stream:
  • the above line will fail

Expected behavior

I used aiodocker version 0.21.0, when using aiohttp version 3.8.6 it works well (but uninstallable for python3.12), on version 3.9.0 it fails with Status code: 101 (aio-libs/aiodocker#832)

Logs/tracebacks

Unfortunately, I only got aiodocker logs:

aiodocker.exceptions.DockerError: DockerError(500, "Cannot upgrade connection to vendored tcp protocol, the docker server has closed underlying socket. Status code: 101. Headers: <CIMultiDictProxy('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)')>.")


### Python Version

```console
$ python --version
Tested on 3.12 and 3.11

aiohttp Version

$ python -m pip show aiohttp
works on 3.8.6, fails on 3.9.0

multidict Version

$ python -m pip show multidict
6.0.4

yarl Version

$ python -m pip show yarl
1.9.3

OS

Ubuntu

Related component

Client

Additional context

No response

Code of Conduct

  • I agree to follow the aio-libs Code of Conduct
@matan1008 matan1008 added the bug label Nov 22, 2023
@bdraco
Copy link
Member

bdraco commented Nov 22, 2023

Unfortunately, thats not a lot to go on.

Any chance that you can bisect to find the regression?

@matan1008

This comment was marked as resolved.

@matan1008
Copy link
Author

#79f5266518b58e1a778450c9b03bf337a7e01901 is the first bad commit

matan1008 added a commit to matan1008/aiohttp that referenced this issue Nov 22, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@bdraco
Copy link
Member

bdraco commented Nov 22, 2023

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.

@bdraco
Copy link
Member

bdraco commented Nov 22, 2023

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

@bdraco
Copy link
Member

bdraco commented Nov 22, 2023

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

1 similar comment
@bdraco

This comment was marked as duplicate.

@matan1008
Copy link
Author

My problem is not related to #7864

@matan1008
Copy link
Author

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?

@matan1008
Copy link
Author

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)>

@matan1008
Copy link
Author

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

@Dreamsorcerer
Copy link
Member

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.

@matan1008
Copy link
Author

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)?

@Dreamsorcerer
Copy link
Member

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

@matan1008
Copy link
Author

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?

@matan1008 matan1008 changed the title Response connection in None on 3.9.0 Response connection is None on 3.9.0 Nov 23, 2023
@Dreamsorcerer
Copy link
Member

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.

@matan1008
Copy link
Author

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

@Dreamsorcerer
Copy link
Member

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

@matan1008
Copy link
Author

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)

@Dreamsorcerer
Copy link
Member

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.

@matan1008
Copy link
Author

matan1008 commented Nov 23, 2023

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

@Dreamsorcerer
Copy link
Member

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:

res = await response.read()
assert res == b"payload"
assert response._connection is None

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

@Dreamsorcerer
Copy link
Member

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...

@Dreamsorcerer
Copy link
Member

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.

@matan1008
Copy link
Author

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?

@Dreamsorcerer
Copy link
Member

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

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Nov 23, 2023

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

@matan1008
Copy link
Author

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''

@Dreamsorcerer
Copy link
Member

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

@bdraco
Copy link
Member

bdraco commented Nov 23, 2023

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

@matan1008
Copy link
Author

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)

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Nov 25, 2023

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

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Nov 25, 2023

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).

@matan1008
Copy link
Author

I installed it regularly with pip (through aiodocker)

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Nov 25, 2023

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.

@matan1008
Copy link
Author

Will do tomorrow morning, thanks!

@matan1008
Copy link
Author

Works great, Thank you!

Dreamsorcerer added a commit that referenced this issue Nov 26, 2023
Dreamsorcerer added a commit that referenced this issue Nov 26, 2023
Dreamsorcerer added a commit that referenced this issue Nov 26, 2023
Dreamsorcerer added a commit that referenced this issue Nov 26, 2023
xiangxli pushed a commit to xiangxli/aiohttp that referenced this issue Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants