aio-libs / aiohttp

Asynchronous HTTP client/server framework for asyncio and Python
https://docs.aiohttp.org
Other
15.07k stars 2.01k forks source link

on_response_chunk_received doesn't trace If body is read from ClientResponse.content #5324

Open afq984 opened 3 years ago

afq984 commented 3 years ago

🐞 Describe the bug

TraceConfig.on_response_chunk_received doesn't trace if the response body is read from ClientResponse.content

This behavior isn't obvious when looking at Tracing Reference, Streaming Response Content, ClientResponse.content

💡 To Reproduce

# test.py
import aiohttp
import asyncio

URL = 'https://raw.githubusercontent.com/aio-libs/aiohttp/master/docs/_static/aiohttp-icon-128x128.png'

async def cb(session, context, params):
    print('cb received', len(params.chunk), 'bytes')

async def main():
    tc = aiohttp.TraceConfig()
    tc.on_response_chunk_received.append(cb)

    async with aiohttp.ClientSession(trace_configs=[tc]) as sess:
        async with sess.get(URL) as response:
            content = await response.content.read()
            print('main received', len(content), 'bytes')

            # async for chunk in response.content.iter_any():
            #     print('main received', len(chunk), 'bytes')

asyncio.run(main())
python test.py
main received 4519 bytes

💡 Expected behavior

cb received 4519 bytes
main received 4519 bytes

📋 Your version of the Python

python -V
Python 3.8.6

📋 Your version of the aiohttp/yarl/multidict distributions

pip show aiohttp multidict yarl
Name: aiohttp
Version: 3.7.3
Summary: Async http client/server framework (asyncio)
Home-page: https://github.com/aio-libs/aiohttp
Author: Nikolay Kim
Author-email: fafhrd91@gmail.com
License: Apache 2
Location: /tmp/tmp.HyMrUwIvm8/lib/python3.8/site-packages
Requires: attrs, multidict, typing-extensions, yarl, chardet, async-timeout
Required-by: 
---
Name: multidict
Version: 5.1.0
Summary: multidict implementation
Home-page: https://github.com/aio-libs/multidict
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /tmp/tmp.HyMrUwIvm8/lib/python3.8/site-packages
Requires: 
Required-by: yarl, aiohttp
---
Name: yarl
Version: 1.6.3
Summary: Yet another URL library
Home-page: https://github.com/aio-libs/yarl/
Author: Andrew Svetlov
Author-email: andrew.svetlov@gmail.com
License: Apache 2
Location: /tmp/tmp.HyMrUwIvm8/lib/python3.8/site-packages
Requires: multidict, idna
Required-by: aiohttp
afq984 commented 3 years ago

The trace callbacks are called in ClientResponse.read: https://github.com/aio-libs/aiohttp/blob/3be8a68d750fd9902106e747d11a0622b8650f2c/aiohttp/client_reqrep.py#L965-L973

I'd like to call the callbacks in StreamReader, but it has non-blocking read_nowait and feed_data. Any suggestions?

gasinvein commented 3 years ago

on_response_chunk_received doesn't seem to be invoked on ClientResponse.content.iter_chunked(), too.

Maybe ClientResponse could have a .read_chunked() method that would wrap .content.iter_chunked(), calling send_response_chunk_received() on each chunk?

prshant70 commented 1 year ago

Hey I see the issue is still open. I am looking to contribute here. Should I raise a PR for this ?

Dreamsorcerer commented 1 year ago

There is already an open PR to add the methods to the client class, listed above your comment.

I've not had a chance to review it yet, but I'd like to avoid having 2 methods to do the same thing, and one of them not being traced. So, if you think you have a good solution that makes tracing work on the existing methods, then please do open a PR.

prshant70 commented 1 year ago

Sure. I will work on it and raise the PR soon.