aio-libs / aiobotocore

asyncio support for botocore library using aiohttp
https://aiobotocore.rtfd.io
Apache License 2.0
1.14k stars 179 forks source link

Leverage aiohttp internal handling of `chunked` argument #1096

Closed jakob-keller closed 5 days ago

jakob-keller commented 5 months ago

Description of Change

https://github.com/aio-libs/aiobotocore/pull/1077#pullrequestreview-1912935492

Assumptions

Replace this text with any assumptions made (if any)

Checklist for All Submissions

Checklist when updating botocore and/or aiohttp versions

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.94%. Comparing base (aec730a) to head (cdd466e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1096 +/- ## ========================================== - Coverage 86.11% 85.94% -0.17% ========================================== Files 62 62 Lines 5933 5928 -5 ========================================== - Hits 5109 5095 -14 - Misses 824 833 +9 ``` | [Flag](https://app.codecov.io/gh/aio-libs/aiobotocore/pull/1096/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/aio-libs/aiobotocore/pull/1096/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs) | `85.94% <100.00%> (-0.17%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aio-libs#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thehesiod commented 1 week ago

is this change covered by tests? AFK for a bit to check the coverage

jakob-keller commented 1 week ago

is this change covered by tests? AFK for a bit to check the coverage

I am not sure and we touched upon that here: https://github.com/aio-libs/aiobotocore/pull/1077#pullrequestreview-1912935492.

For our purposes, aiobotocore's chunked() duplicates logic already provided by aiohttp's update_transfer_encoding(): https://github.com/aio-libs/aiohttp/blob/7571eb9f65fde72fd6ccadcc5bfe2dc81f3a126c/aiohttp/client_reqrep.py#L439

I believe it is safe to drop chunked() when modern releases of aiohttp are used, i.e. any release we support in requirements.

jakob-keller commented 5 days ago

@Dreamsorcerer: Could you advise us on this PR, please? I believe that aiohttp will handle chunked requests correctly, without having to pass the chunked argument explicitely. Any guidance?

Dreamsorcerer commented 5 days ago

From reading the code, it looks like in the code, if the body has no size and no Content-Length header is set, then chunked will be set regardless of what you pass in: https://github.com/aio-libs/aiohttp/blob/3de518a0a324b082cca3ed94ef5a9338b4745759/aiohttp/client_reqrep.py#L503

Then it looks like it will handle chunked as per: https://github.com/aio-libs/aiohttp/blob/3de518a0a324b082cca3ed94ef5a9338b4745759/aiohttp/client_reqrep.py#L444-L457

From my reading of that, the code expects to set the Transfer-Encoding header itself. Which is probably why the code you're deleting removed the Transfer-Encoding header. So, I'd suggest spending a bit more time understanding why a Transfer-Encoding header would have been set in first place, seems to me that there should never be a Transfer-Encoding header passed in here.

jakob-keller commented 5 days ago

From my reading of that, the code expects to set the Transfer-Encoding header itself. Which is probably why the code you're deleting removed the Transfer-Encoding header. So, I'd suggest spending a bit more time understanding why a Transfer-Encoding header would have been set in first place, seems to me that there should never be a Transfer-Encoding header passed in here.

Thank you, that helped a lot!

Transfer-Encoding headers are set by botocore: https://github.com/boto/botocore/blob/149ae67cc8909ab03c11337392e676711a438d90/botocore/awsrequest.py#L405

We are removing them to satisfy expectations of aiohttp.

Everything works as it should.