Skip to content

Fix .as_bytes() producing different results to write() - #14363

Open
Dreamsorcerer wants to merge 14 commits into
masterfrom
asbytes
Open

Dreamsorcerer wants to merge 14 commits into
masterfrom
asbytes

Conversation

@Dreamsorcerer

@Dreamsorcerer Dreamsorcerer commented Nov 11, 2026 •

Copy link
Copy Markdown
Member

Fixes #13495.

@Dreamsorcerer Dreamsorcerer added the backport-3.15 Trigger automatic backporting to the 3.15 release branch by Patchback robot label Nov 11, 2026
@pyba-chronographer pyba-chronographer Bot added bot:chronographer:provided There is a change note present in this PR labels Nov 11, 2026
@codecov

codecov Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.12%. Comparing base (fe09949) to head (3906832).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #14363    +/-   ##
========================================
  Coverage   99.12%   99.12%            
========================================
  Files         135      135            
  Lines       53661    53809   +148     
  Branches     2737     2750    +13     
========================================
+ Hits        53191    53340   +149     
  Misses        356      356            
+ Partials      114      113     -1     
Flag Coverage Δ
Autobahn 21.64% <13.60%> (-0.03%) ⬇️
CI-GHA 99.05% <100.00%> (+<0.01%) ⬆️
OS-Linux 98.73% <100.00%> (+<0.01%) ⬆️
OS-Windows 97.49% <100.00%> (+<0.01%) ⬆️
OS-macOS 98.36% <100.00%> (+<0.01%) ⬆️
Py-3.11 98.29% <100.00%> (+<0.01%) ⬆️
Py-3.12 98.39% <100.00%> (+0.01%) ⬆️
Py-3.13 98.37% <100.00%> (+<0.01%) ⬆️
Py-3.14 98.44% <100.00%> (+<0.01%) ⬆️
Py-3.15 98.43% <100.00%> (+<0.01%) ⬆️
Py-3.15t 98.29% <100.00%> (+<0.01%) ⬆️
Py-pypy-3.12 96.96% <100.00%> (+<0.01%) ⬆️
VM-macos-latest 98.36% <100.00%> (+<0.01%) ⬆️
VM-ubuntu-latest 98.73% <100.00%> (+<0.01%) ⬆️
VM-windows-latest 97.49% <100.00%> (+<0.01%) ⬆️
cython-coverage 83.83% <26.15%> (-0.11%) ⬇️

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

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

@codspeed

codspeed Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 50 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 102 untouched benchmarks
⏩ 83 skipped benchmarks1


Comparing asbytes (3906832) with master (fe09949)

Open in CodSpeed

Footnotes

  1. 83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Comment thread aiohttp/multipart.py Resolved
Comment thread aiohttp/multipart.py Resolved
Comment thread aiohttp/multipart.py Resolved
Comment thread aiohttp/multipart.py
"""Return bytes representation of the multipart data."""
buffer = bytearray()
await self._write(None, True, buffer)
return bytes(buffer)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bdraco Do you think we should just change the return to bytearray and avoid the bytes allocation?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep bytes. Public API documented as bytes. bytearray unhashable, fails isinstance(x, bytes) checks in callers. Final bytes(buffer) copy: one memcpy, negligible next to QP/base64 encoding cost. Alternative if copy matters: collect list[bytes], b"".join at end (single allocation, as bdraco noted).

Comment thread aiohttp/multipart.py Resolved
Comment thread aiohttp/multipart.py Resolved
Comment thread docs/multipart_reference.rst Resolved
Comment thread tests/test_multipart.py Resolved
Dreamsorcerer and others added 2 commits November 14, 2026 18:46
Co-authored-by: Sam Bull <aa6bs0@sambull.org>
@Dreamsorcerer
Dreamsorcerer marked this pull request as ready for review November 14, 2026 18:49
@Dreamsorcerer

Copy link
Copy Markdown
Member Author

@aiolibsbot review

@aiolibsbot

Copy link
Copy Markdown
Contributor

PR Review — Fix .as_bytes() producing different results to write()

Solid fix with one deprecation-leak to close before merge.

The core change is right: routing as_bytes() through the same _write() loop as write() is the correct way to guarantee byte-identity, and the quoted-printable rewrite fixes a genuine CRLF-split corruption (#13495) while bounding the partial-line buffer with fixed-offset blocks so the output stays a pure function of the payload bytes. Test coverage is thorough: chunk-split cases for QP and base64, a 3 MB newline-free body with varied chunk sizes, an explicit memory-bound test, and a reusability test for async-iterable parts. The versionadded:: 3.12.1 claim checks out against CHANGES.rst.

  • 🟡 MultipartWriter.decode() deprecation fires from aiohttp's own web.Response.text getter (web_response.py:653), with the warning attributed to library code. Under filterwarnings = error (as in this repo's pytest.ini) resp.text now raises for multipart bodies; the PR's own filterwarnings marker in test_web_response.py is the symptom. Route the internal caller around the warning or deprecate at the Payload level.
  • 🟢 as_bytes(encoding, errors) now accepts and silently ignores both parameters.
  • 🟢 The writer/buffer dual-mode threading in _write() and MultipartPayloadWriter could collapse into one small in-memory writer object, removing the closure, _write_chunk, and three asserts.
  • 🟢 A 4096-byte block cut can split a CRLF into raw CR + soft break + bare LF. Round-trips and stays consistent, but is non-canonical QP; low probability.
  • 🟢 No multi-chunk Content-Encoding test; zlib should be chunk-independent, zlib_ng/isal unverified here.
  • 🟢 Missing .deprecation.rst fragment and 14363 symlink; confirm 3.15 version markers match the backport target.

🟡 Important

1. Deprecation warning fires from aiohttp's own call path in web.Response.text
aiohttp/multipart.py:1177-1182

MultipartWriter.decode() overrides the still-abstract, still-live Payload.decode() interface, and aiohttp itself calls it: web_response.py:653 does self._body.decode(self.charset or "utf-8") in the Response.text getter. With stacklevel=2 the warning is attributed to web_response.py, not to user code, so a user who reads resp.text on a multipart body gets a DeprecationWarning they cannot act on.

Why it matters:

  • Under -W error::DeprecationWarning (this repo's own pytest.ini uses filterwarnings = error, and many downstream test suites do the same) resp.text now raises for a MultipartWriter body. The PR had to add @pytest.mark.filterwarnings("ignore:MultipartWriter.decode:...") to tests/test_web_response.py:1210 to keep its own suite green, which is the symptom of this.
  • A deprecation that only library-internal code can trigger is not actionable and will generate issues.

Fix options (pick one):

  • Keep the warning in the public decode() but move the body into a private _decode() and have Response.text (and any other internal Payload.decode consumer) call the private one.
  • Or deprecate Payload.decode() at the base-class level with a documented replacement for Response.text, rather than deprecating only this override.

Either way the filterwarnings marker on test_payload_body_get_text should then become unnecessary and be removed.

warnings.warn(
    "MultipartWriter.decode() is deprecated and will be removed "
    "in a future release, use (await writer.as_bytes()).decode() instead",
    DeprecationWarning,
    stacklevel=2,
)

🟢 Suggestions

2. `as_bytes(encoding, errors)` arguments are now silently ignored
aiohttp/multipart.py:1241-1246

The old as_bytes() forwarded encoding/errors to each part.as_bytes(encoding, errors). The new path calls part.as_bytes() with defaults at both line 1241 and line 1246, so the public signature still accepts the two parameters but they have no effect.

In practice the impact is small (most payloads prefer their instance _encoding), but a parameter that is accepted and dropped is a misleading contract. Either thread them through _write() to the two part.as_bytes(...) calls, or document them as ignored.

                    await w.write(await part.as_bytes())
                else:
                    await part.write(w)  # type: ignore[arg-type]
                await w.write_eof()
            elif buffer is not None:
                buffer += await part.as_bytes()
3. Dual-mode `writer`/`buffer` threading could collapse into one tiny writer object
aiohttp/multipart.py:1204-1220

_write() and MultipartPayloadWriter both grow an Optional[bytearray] alternate sink, which costs: the emit closure, _write_chunk, three assert writer is not None guards, two | None types, and two if buffer is not None branches.

A minimal in-memory writer exposing the only method used downstream, async def write(self, chunk: bytes) -> None, would remove all of that: as_bytes() builds one, passes it where writer goes today, and MultipartPayloadWriter stays unchanged from master. The one genuine behavioural difference (buffered mode must use part.as_bytes() so async-iterable parts are cached and unbufferable parts raise early) can stay as a single branch keyed on an explicit flag or isinstance.

The # type: ignore[arg-type] on part.write(w) already shows MultipartPayloadWriter only needs the .write shape, so a small Protocol for "something with async write" would also let that ignore go.

emit: Callable[[bytes], Awaitable[None]]
if buffer is None:
    assert writer is not None
    emit = writer.write
else:
    async def _buffer_emit(chunk: bytes) -> None:
        buffer.extend(chunk)
    emit = _buffer_emit
4. Block boundary can split a CRLF pair into a raw CR, soft break, bare LF
aiohttp/multipart.py:1370-1373

The partial-line flush (and _encode_quoted_printable_line at line 79) cuts at fixed 4096-byte offsets without regard to the bytes at the cut. If a line's \r sits at offset 4095 of a block and its \n is the first byte of the next, the output is b2a_qp(block ending in \r) + =\r\n + b2a_qp(\n...).

From CPython's binascii algorithm (not executed here, the review shell has no Python): a trailing lone \r is emitted raw, and a block starting with \n gets a bare \n line ending because CRLF detection fails. The result decodes correctly with a2b_qp (raw CR passes through, soft break vanishes) and both write() and as_bytes() produce the same bytes, so the PR's invariant holds. But the wire contains a raw CR not followed by LF, which RFC 2045 §6.7 disallows, and mixed \n/\r\n endings in one body.

This only affects lines longer than 4096 bytes with a CRLF landing exactly on a boundary, so it's low probability. If you want to close it, pull the cut back one byte when line[boundary-1] == 0x0D in both the streaming loop and _encode_quoted_printable_line (the rule must match in both places to keep the pure-function property). A parametrized test with b"x" * 4095 + b"\r\n" would pin it either way.

while len(buf) - start > _QP_BLOCK_SIZE:
    parts.append(binascii.b2a_qp(buf[start : start + _QP_BLOCK_SIZE]))
    parts.append(_QP_SOFT_BREAK)
    start += _QP_BLOCK_SIZE
5. No chunk-split test for Content-Encoding, unlike QP and base64
tests/test_multipart.py:1901-1920

test_writer_as_bytes_applies_content_encoding uses a StringPayload, so both write() and as_bytes() feed the compressor a single chunk. The new docs promise byte-identity with the wire, and as_bytes() always feeds one blob while write() streams the payload's chunks.

For stdlib zlib deflate output is independent of input chunking when no intermediate flush happens, and ZLibCompressor.compress doesn't flush per chunk, so this should hold. I could not verify the zlib_ng / isal backends from here (unverified). Since parametrize_zlib_backend already exists, adding an async-iterable multi-chunk case to this test (mirroring the QP/base64 split tests) would make the invariant a counted fact across all three backends instead of an assumption.

writer.append("Time to Relax!", {CONTENT_ENCODING: encoding})
6. Deprecation of `decode()` has no `.deprecation.rst` fragment; PR number not linked
CHANGES/13495.bugfix.1.rst:1-4

Two changelog gaps against the repo's conventions:

  • MultipartWriter.decode() is now deprecated with a runtime warning, but there is no CHANGES/*.deprecation.rst entry. The deprecation towncrier type exists (pyproject.toml, "Deprecations (removal in next major release)") and is the only place users will look for it in the release notes.
  • AGENTS.md asks for both issue and PR numbers (ln -s 13495.bugfix.rst CHANGES/14363.bugfix.rst). Only the issue-numbered files are present.

Also worth a quick check: master is 4.0.0a2.dev0, so the .. deprecated:: 3.15 / .. versionchanged:: 3.15 markers are only correct if this is backported to 3.15.


Checklist

  • No hardcoded secrets or unsafe operations
  • Encoded output is chunk-independent (QP, base64)
  • Partial-line QP buffer is bounded
  • Public API contract preserved — warning #1, suggestion #2
  • Deprecation does not fire from library-internal callers — warning #1
  • Changelog fragments per repo convention — suggestion #6
  • Docs updated for user-visible change
  • New behaviour covered by tests — suggestion #5
  • Structural simplicity — suggestion #3

To rebase and address feedback, mention me: @aiolibsbot rebase critical (fixes 🔴 only), @aiolibsbot rebase important (fixes 🔴 + 🟡), or @aiolibsbot rebase --fix for all. (A bare @aiolibsbot rebase only rebases onto the base branch.)


Automated review by Kōan (Claude) HEAD=3906832 6 min 27s

@Dreamsorcerer

Copy link
Copy Markdown
Member Author

Deprecation warning fires from aiohttp's own call path in web.Response.text

Not clear exactly what to do with this. As an attribute, it can't do async. I can suppress the deprecation for now, but not clear what should happen once .decode() is removed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-3.15 Trigger automatic backporting to the 3.15 release branch by Patchback robot bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MultipartWriter.as_bytes() does not apply part Content-Encoding / Content-Transfer-Encoding, diverging from write()

3 participants