Skip to content

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Jul 10, 2025

What do these changes do?

This PR addresses review comments from PR #11290 by:

  • Removing redundant inner try-except block in IOBasePayload.size property - the exceptions are already handled by the outer try-except
  • Updating stale test comments that referenced the bug as if it was still present
  • Improving test docstrings to reflect that they verify the fix rather than demonstrate a bug

Are there changes in behavior for the user?

No behavioral changes. This is purely a code cleanup that removes redundancy while maintaining the exact same functionality.

Is it a substantial burden for the maintainers to support this?

No, this actually reduces maintenance burden by:

  • Removing redundant code that could cause confusion
  • Making the code flow clearer and easier to understand
  • Ensuring test documentation accurately reflects the current state

Related issue number

Follow-up to #11290 (addressing review comments)
Related to #11270 (original bug fix)

Checklist

  • I think the code is well written
  • Unit tests for the changes exist (all existing tests pass)
  • Documentation reflects the changes (no user-facing docs needed)
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the CHANGES/ folder

Summary of Changes

1. Removed redundant code in aiohttp/payload.py:

# Before:
if self._start_position is None:
    try:
        self._start_position = self._value.tell()
    except (OSError, AttributeError):
        # Can't get position, can't determine size
        return None

# After:
if self._start_position is None:
    self._start_position = self._value.tell()

The inner try-except was redundant because:

  • The outer try-except block in the size property already catches AttributeError and OSError
  • Any exception raised by self._value.tell() will be caught by the outer handler
  • Having nested try-except blocks for the same exceptions adds unnecessary complexity

2. Updated test comments in tests/test_payload.py:

  • Removed comments claiming "This assertion fails!" and "This also fails!" from test_iobase_payload_size_after_reading
  • Updated docstring from "demonstrates the bug" to "verifies that size calculation properly accounts for the initial file position"

3. Updated test comments in tests/test_client_functional.py:

  • Updated docstring in test_file_upload_307_308_redirect from "demonstrates the bug" to "verifies that file payloads maintain correct Content-Length"

All tests continue to pass, confirming the redundant code was indeed unnecessary.

@bdraco bdraco added backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot backport-3.13 Trigger automatic backporting to the 3.13 release branch by Patchback robot bot:chronographer:skip This PR does not need to include a change note labels Jul 10, 2025
@bdraco bdraco marked this pull request as ready for review July 10, 2025 15:52
@bdraco bdraco requested a review from asvetlov as a code owner July 10, 2025 15:52
Copy link

codecov bot commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.84%. Comparing base (3d969a3) to head (f56cd2b).
Report is 11 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11301      +/-   ##
==========================================
- Coverage   98.84%   98.84%   -0.01%     
==========================================
  Files         131      131              
  Lines       43433    43430       -3     
  Branches     2331     2331              
==========================================
- Hits        42933    42930       -3     
  Misses        346      346              
  Partials      154      154              
Flag Coverage Δ
CI-GHA 98.74% <100.00%> (-0.01%) ⬇️
OS-Linux 98.47% <100.00%> (-0.01%) ⬇️
OS-Windows 96.81% <100.00%> (-0.01%) ⬇️
OS-macOS 97.69% <100.00%> (-0.01%) ⬇️
Py-3.10.11 97.34% <100.00%> (-0.01%) ⬇️
Py-3.10.18 97.82% <100.00%> (-0.01%) ⬇️
Py-3.11.13 98.00% <100.00%> (-0.01%) ⬇️
Py-3.11.9 97.53% <100.00%> (-0.01%) ⬇️
Py-3.12.10 97.64% <100.00%> (-0.01%) ⬇️
Py-3.12.11 98.11% <100.00%> (-0.01%) ⬇️
Py-3.13.3 98.37% <100.00%> (-0.01%) ⬇️
Py-3.9.13 97.22% <100.00%> (-0.02%) ⬇️
Py-3.9.23 97.70% <100.00%> (-0.01%) ⬇️
Py-pypy7.3.16 86.73% <100.00%> (-10.57%) ⬇️
VM-macos 97.69% <100.00%> (-0.01%) ⬇️
VM-ubuntu 98.47% <100.00%> (-0.01%) ⬇️
VM-windows 96.81% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

codspeed-hq bot commented Jul 10, 2025

CodSpeed Performance Report

Merging #11301 will not alter performance

Comparing review_comments_11290 (f56cd2b) with master (3d969a3)

Summary

✅ 59 untouched benchmarks

@Dreamsorcerer Dreamsorcerer merged commit 815901f into master Jul 15, 2025
42 of 43 checks passed
@Dreamsorcerer Dreamsorcerer deleted the review_comments_11290 branch July 15, 2025 11:58
Copy link
Contributor

patchback bot commented Jul 15, 2025

Backport to 3.12: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.12/815901f6afb2c460650ccb0b6f0bfa5b55fe5cd9/pr-11301

Backported as #11318

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jul 15, 2025
Copy link
Contributor

patchback bot commented Jul 15, 2025

Backport to 3.13: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.13/815901f6afb2c460650ccb0b6f0bfa5b55fe5cd9/pr-11301

Backported as #11319

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jul 15, 2025
Dreamsorcerer pushed a commit that referenced this pull request Jul 15, 2025
… comments from PR #11290 (#11319)

**This is a backport of PR #11301 as merged into master
(815901f).**

Co-authored-by: J. Nick Koston <nick@koston.org>
Dreamsorcerer pushed a commit that referenced this pull request Jul 15, 2025
… comments from PR #11290 (#11318)

**This is a backport of PR #11301 as merged into master
(815901f).**

Co-authored-by: J. Nick Koston <nick@koston.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3.12 Trigger automatic backporting to the 3.12 release branch by Patchback robot backport-3.13 Trigger automatic backporting to the 3.13 release branch by Patchback robot bot:chronographer:skip This PR does not need to include a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants