diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 405be76..2b32a45 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -264,9 +264,18 @@ jobs: - name: Fuzz run: | + # `-a` turns on debug assertions, which is what makes the deferred + # modes' retention checkable at all (#239): a leaf keeps offsets into + # the buffer it was parsed from, and `Retained::of` falls back to a copy + # when the bytes it is handed are not in that buffer. The fallback is + # deliberate -- it cannot read out of bounds -- but it is silent, so + # without the assertion a slip in the offsets degrades to the copying + # this issue removed and nothing says so. The deep-fuzz workflow stays + # on the release build, where more executions per second matter more + # than this one invariant. for target in $FUZZ_TARGETS; do echo "::group::$target" - cargo fuzz run "$target" -- \ + cargo fuzz run -a "$target" -- \ -max_total_time="$FUZZ_SECONDS" \ -seed=1 \ -print_final_stats=1 diff --git a/CHANGELOG.md b/CHANGELOG.md index c3ae251..ec7b458 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,30 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Lazy modes borrow the payload instead of copying every deferred part** (#239). Deferring + a decode used to copy the part's encoded bytes out of the message, which is the opposite + of what deferring is for: parsing a 96 MiB single-attachment message in `mode="lazy"` cost + 96 MiB *on top of* the payload the caller still held, and base64 is 1.33x what it encodes, + so a retained part could cost more than the decoded bytes it avoided producing. A deferred + part now keeps its offsets in the buffer it was parsed from, and the result keeps that + buffer alive. Measured on a 96 MiB message with one unread attachment: peak RSS 192.3 MiB + before, 96.2 MiB after. Counted in the core, on the attachment-heavy fixture: a lazy tree + peaked at 798,111 bytes and now peaks at 14,332 -- the same as a metadata tree, because a + range is not a copy -- and a flat lazy parse dropped from 790,993 to 23,398. + + **This changes the memory contract, and is the reason to read this entry.** A `PyLazyMail` + attachment or a `PyLazyMimePart` leaf pins the payload it was parsed from for as long as it + is reachable, so keeping one attachment out of a mailbox keeps that whole message rather + than just the part. The pin is per message even in `parse_many`, so one slot never holds + another's payload. A caller who wants the bytes without the message reads `content`, which + is a decoded copy, and drops the attachment. Nothing about the values changes: every mode + returns what it returned, including for a message whose header block had to be repaired + (#150), where the offsets index the rebuilt copy that now travels with the result. + + One exception, invisible from Python: the leaves *inside* a `message/rfc822` node still hold + copies. Their bytes were produced by decoding that node's body, so they are in no caller + buffer to point into. + - **The parsing core is a crate, and has Rust tests for the first time** (#236). It was a `#[path]`-included file: the binding declared it as a module, and both fuzz targets reached across the tree to include the same source again under different cfg. Nothing diff --git a/Readme.md b/Readme.md index 74c15b0..5fd2b85 100644 --- a/Readme.md +++ b/Readme.md @@ -487,10 +487,14 @@ runner; the ratios are what to read. Two things to know before choosing it: -**It trades memory for decoding.** The encoded bytes of every attachment are -retained until the message is dropped, and base64 is about 1.33x the size of what -it encodes — so a retained part costs *more* than the decoded bytes it avoids -producing. Right for one attachment out of twenty; wrong for all twenty. +**It pins the payload.** A deferred attachment does not copy itself out of the +message; it remembers where it sits in the payload you passed in, and keeps that +payload alive for as long as it is reachable. So the result holds one message's +worth of memory rather than two — parsing a 96 MiB message now costs 96 MiB and +used to cost 192 MiB — but the payload is not released when you drop your own +reference to it. Keeping one attachment out of a mailbox keeps that message, not +its attachment. If you want the bytes without the message, read `content`, which +is a decoded copy, and drop the attachment. **A `DecodeError` moves.** A part whose `Content-Transfer-Encoding` cannot be decoded fails the whole parse in full mode, and fails on `content` here — so a @@ -574,8 +578,9 @@ data = pdf.content # decoded here, and only this one `mode="metadata"` decodes nothing *and retains nothing*: a node reports `encoded_size` in place of `content` and there is no way to ask for the bytes. -That is the difference between the two — lazy mode keeps a copy of every leaf so -it can decode one later, metadata mode keeps none and is the cheaper sweep. +That is the difference between the two — a lazy leaf remembers where it sits in +the payload so it can decode itself later, which keeps that payload alive; +metadata mode remembers nothing and is the sweep that holds nothing. On the attachment-heavy fixture (767 KiB), median of three interleaved rounds on the CI runner: full tree 0.556 ms, `mode="lazy"` with nothing read 0.077 ms, @@ -595,6 +600,12 @@ has to deliver eagerly. Its node therefore arrives with `is_decoded` already `True`, and unlike `parse_email(mode="metadata")` a deferred tree *can* raise `DecodeError` for such a part. Nothing else is decoded. +It is also the one place a lazy leaf holds bytes of its own. The leaves *inside* +an embedded message were parsed out of that decoded body rather than out of your +payload, so they keep copies; everywhere else a leaf keeps offsets. The +difference is not visible from Python — a leaf decodes the same way either way — +but it is why a lazy tree of a bounce is not quite free. + `walk` accepts a node from any mode and yields nodes of the same type. #### Which API when diff --git a/bench/src/main.rs b/bench/src/main.rs index ca38606..67592d1 100644 --- a/bench/src/main.rs +++ b/bench/src/main.rs @@ -92,12 +92,14 @@ fn main() { "tree-lazy" => Box::new(|| { mail_parser::parse_tree_deferred(&payload, true) .expect("the fixture must parse") + .root .children .len() }), "tree-metadata" => Box::new(|| { mail_parser::parse_tree_deferred(&payload, false) .expect("the fixture must parse") + .root .children .len() }), diff --git a/bench/tests/allocs.rs b/bench/tests/allocs.rs index c6552d5..2ab7785 100644 --- a/bench/tests/allocs.rs +++ b/bench/tests/allocs.rs @@ -4,6 +4,13 @@ //! sweep does not decode bodies, lazy mode so an untouched attachment is not //! copied. Those claims have been prose. This counts them. //! +//! `peak` is the number to read for lazy mode. Since #239 a deferred leaf keeps +//! offsets into the buffer it was parsed from rather than a copy of itself, so +//! what a lazy result holds no longer scales with the bodies it deferred -- and +//! the table is where that stops being a claim. What it cannot show is the +//! payload the caller still holds, which those offsets now pin; the Python side +//! measures that, because the pin lives in the binding. +//! //! Exact equality against a committed table, not bounds. A bound absorbs a //! regression silently until it crosses the bound; an exact number turns any //! change in allocation behaviour into a diff someone has to look at and either @@ -190,17 +197,37 @@ fn the_modes_allocate_what_they_promise() { "an untouched lazy parse must hold less than a full one \ (lazy {lazy:?}, full {full:?})" ); - // Deliberately asserted only here. Lazy mode retains each part's *encoded* - // bytes, and base64 is 4/3 of what it decodes to -- so on a small - // attachment-bearing message a lazy tree genuinely holds MORE than a decoded - // one (measured: 6696 against 6323 bytes on attachment_message.eml). The - // trade only pays once the bodies are large, which is the case the mode was - // written for and the only case where this ordering is a promise. assert!( tree_lazy.peak < tree.peak, "on a message with real attachments, a lazy tree must hold less than a \ decoded one (lazy {tree_lazy:?}, decoded {tree:?})" ); + // The #239 claim, and the one worth guarding: what a deferred leaf retains is + // a range, so a lazy tree's footprint is its structure and not its bodies. + // Until #239 this assertion was false by a wide margin -- a lazy tree of this + // fixture peaked at 798,111 bytes against the 14,332 it peaks at now -- and + // on a small attachment-bearing message a lazy tree held *more* than a fully + // decoded one, because base64 is 4/3 of what it decodes to and the mode kept + // the encoded side. + assert!( + tree_lazy.peak < payload.len() / 4, + "a lazy tree peaked at {} bytes on a {} byte message; it retains offsets \ + rather than bytes, so it should be far under a quarter of the input", + tree_lazy.peak, + payload.len() + ); + // The flat mode's version of the same claim. Weaker on purpose: flat lazy + // decodes every body part, so its floor is the bodies, and only the + // attachments are deferred. A quarter of a full parse is the margin that + // holds for a message whose weight is in its attachments -- which is the + // message this mode is for. + assert!( + lazy.peak * 4 < full.peak, + "an untouched lazy parse held {} bytes against a full parse's {}; the \ + attachments it deferred should not be in either number", + lazy.peak, + full.peak + ); assert!( metadata.peak < payload.len() / 4, "metadata mode peaked at {} bytes on a {} byte message; it decodes no \ diff --git a/crates/fast_mail_parser_core/src/lib.rs b/crates/fast_mail_parser_core/src/lib.rs index 3947a21..22c70c5 100644 --- a/crates/fast_mail_parser_core/src/lib.rs +++ b/crates/fast_mail_parser_core/src/lib.rs @@ -773,6 +773,87 @@ fn metadata_from_payload(payload: &[u8]) -> Result /// encodes, so a retained part costs *more* than the decoded bytes it avoids /// producing -- right for finding the one PDF in a mailbox, wrong for decoding /// everything anyway. +/// Where a deferred leaf's encoded bytes live. +/// +/// Lazy mode used to copy every deferred part out of the message -- which is the +/// opposite of what deferring is for. On a mailbox sweep looking for one PDF, +/// the copies *are* the cost. The bytes are already in the buffer the caller +/// handed us, so a range into that buffer is enough, provided the buffer outlives +/// the result. That is the contract change: a `LazyMail` now pins its payload. +/// +/// `Owned` is the exception rather than the rule, and there are exactly two of +/// them: a leaf inside a decoded `message/rfc822` body, whose bytes were produced +/// by decoding and are in no caller buffer, and a message whose header block had +/// to be repaired (#150), where the parse ran over a rebuilt copy. The repaired +/// copy is returned alongside the result so a range can still index it. +#[derive(Debug)] +pub enum Retained { + /// A sub-range of the buffer the message was parsed from. + Range(std::ops::Range), + /// An owned copy, for bytes that are in no caller buffer. + Owned(Vec), +} + +impl Retained { + /// Retain `part`: as offsets when it is a subslice of `base`, as a copy when + /// it is not. + /// + /// mailparse's `raw_bytes` is a subslice of the buffer given to `parse_mail` + /// -- it is produced by slicing and never by copying -- so the range arm is + /// the one that is taken for every part of a message parsed in one piece. + /// The copy arm exists for the parts that genuinely are not in that buffer: + /// a leaf below a `message/rfc822` node, whose enclosing bytes had to be + /// transfer-decoded into a fresh `Vec` before the message inside could be + /// parsed at all. + /// + /// Checked rather than asserted. Computing offsets from a pointer that is not + /// in `base` would produce a range that indexes out of bounds later, far from + /// the mistake; the bounds test that avoids it is two comparisons against a + /// parse that has already walked every byte of the part. The debug assertion + /// still fires in the tests, so a new caller that expected to borrow and + /// silently copies is a test failure rather than a performance mystery. + pub fn of(base: &[u8], part: &[u8]) -> Retained { + let (base_start, part_start) = (base.as_ptr() as usize, part.as_ptr() as usize); + let within = part_start >= base_start && part_start + part.len() <= base_start + base.len(); + debug_assert!( + within, + "retained bytes are not a subslice of the parsed buffer" + ); + if within { + let start = part_start - base_start; + Retained::Range(start..start + part.len()) + } else { + Retained::Owned(part.to_vec()) + } + } + + /// The bytes, given the buffer they were parsed from. + /// + /// `base` must be the buffer the parse ran over -- the caller's payload, or + /// the repaired copy the result carries when one was made. Indexing a + /// `Range` into anything else is a logic error that would silently return + /// the wrong bytes, which is why the repaired buffer travels with the result + /// instead of being dropped. + pub fn slice<'a>(&'a self, base: &'a [u8]) -> &'a [u8] { + match self { + Retained::Range(range) => &base[range.clone()], + Retained::Owned(bytes) => bytes, + } + } + + /// The number of bytes retained, without needing the base buffer. + pub fn len(&self) -> usize { + match self { + Retained::Range(range) => range.len(), + Retained::Owned(bytes) => bytes.len(), + } + } + + pub fn is_empty(&self) -> bool { + self.len() == 0 + } +} + #[derive(Debug)] pub struct LazyAttachment { pub mimetype: String, @@ -782,7 +863,9 @@ pub struct LazyAttachment { /// Bytes the part's body occupies before transfer-decoding -- the same value /// and the same name as in metadata mode. pub encoded_size: usize, - pub raw: Vec, + /// Where the part's encoded bytes are, relative to the buffer the parse ran + /// over: the caller's payload, or `LazyMail::repaired` when that is set. + pub raw: Retained, } /// A message with its bodies decoded and its attachments deferred (#97). @@ -793,6 +876,13 @@ pub struct LazyAttachment { /// which is what lets `strict=True` mean the same thing in both modes. #[derive(Debug)] pub struct LazyMail { + /// The rebuilt payload, when the header block had to be repaired (#150). + /// + /// Returned rather than dropped because the attachments' `Retained::Range` + /// offsets are relative to whatever buffer the parse ran over, and for a + /// repaired message that is this copy, not the caller's payload. Dropping it + /// would leave ranges indexing a buffer that no longer matches. + pub repaired: Option>, pub subject: String, pub text_plain: Vec, pub text_html: Vec, @@ -841,6 +931,9 @@ pub fn parse_email_lazy(payload: &[u8]) -> Result { if repaired.is_some() { warn_separator(&mut mail.warnings); } + // The ranges in `mail.attachments` index whichever buffer was parsed, so the + // repaired copy has to travel with the result. + mail.repaired = repaired; Ok(mail) } @@ -920,7 +1013,10 @@ fn lazy_from_payload(payload: &[u8]) -> Result { encoded_size: encoded_size(&body), // The one copy this mode makes, and what makes it a trade rather // than a free win. It is the encoded part, not the decoded one. - raw: part.raw_bytes.to_vec(), + // A range, not a copy: the bytes are already in the buffer + // being parsed, and copying them is what lazy mode exists to + // avoid. + raw: Retained::of(payload, part.raw_bytes), }); continue; } @@ -954,6 +1050,9 @@ fn lazy_from_payload(payload: &[u8]) -> Result { } Ok(LazyMail { + // Filled by the caller, which is the only place that knows whether a + // repair happened. + repaired: None, subject, text_plain, text_html, @@ -1075,9 +1174,15 @@ pub enum NodeBody { /// a later `decode_part` reproduce full mode's `content` -- the same /// mechanism, and the same claim, as flat lazy mode (#97). Metadata mode /// leaves it `None` and keeps only the size. + /// + /// `Retained` rather than `Vec`: for a leaf of the message itself those + /// bytes are already in the buffer being parsed and are kept as offsets into + /// it (#239). A leaf below a `message/rfc822` node is the exception and is + /// copied, because the buffer it sits in was produced by decoding and is + /// dropped when the walk leaves that subtree. Undecoded { encoded_size: usize, - raw: Option>, + raw: Option, }, /// Decoded during the walk, because the structure below it needed it. /// @@ -1136,7 +1241,7 @@ pub struct TreeNode { /// reason: a `message/rfc822` body has to be decoded before the message inside /// it can be parsed, and a tree that dropped those children in one mode would /// not be the same tree. Nothing else is decoded. -pub fn parse_tree_deferred(payload: &[u8], defer: bool) -> Result { +pub fn parse_tree_deferred(payload: &[u8], defer: bool) -> Result { if payload.len() > MAX_INPUT_BYTES { return Err(MailParseError::Generic(ERR_INPUT_TOO_LARGE)); } @@ -1144,20 +1249,54 @@ pub fn parse_tree_deferred(payload: &[u8], defer: bool) -> Result { + /// Nothing at all: metadata mode, which keeps sizes and no bytes. + Nothing, + /// Offsets into this buffer, which the caller of the parse keeps alive. + In(&'a [u8]), + /// A copy, for a subtree parsed out of a buffer this walk owns and drops. + Copy, +} + +/// A tree whose leaves are offsets, and the buffer those offsets index when it +/// is not the caller's (#239). +/// +/// The pair travels together for the reason recorded on `LazyMail::repaired`: a +/// range is only meaningful next to the buffer the parse ran over, and for a +/// message whose header block had to be repaired that buffer is the rebuilt copy. +#[derive(Debug)] +pub struct DeferredTree { + pub root: TreeNode, + pub repaired: Option>, } -/// `#[inline(never)]`, like `MimePart::build`: it recurses, it returns a large -/// struct, and no hot path reaches it. +/// `#[inline(never)]`: it recurses, it returns a large struct, and no hot path +/// reaches it. +/// +/// `retain` says what a leaf of this walk keeps of itself -- see `Retain`. #[inline(never)] fn build_node( part: &ParsedMail<'_>, depth: usize, - defer: bool, + retain: Retain<'_>, ) -> Result { if depth >= MAX_MIME_DEPTH { return Err(MailParseError::Generic(ERR_MIME_DEPTH)); @@ -1170,7 +1309,7 @@ fn build_node( let children = part .subparts .iter() - .map(|child| build_node(child, depth + 1, defer)) + .map(|child| build_node(child, depth + 1, retain)) .collect::, _>>()?; (NodeBody::Container, children) } else if mime == "message/rfc822" { @@ -1180,7 +1319,18 @@ fn build_node( let inner = { let repaired = repair_missing_separator(&raw); let parsed = parse_mail(repaired.as_deref().unwrap_or(raw.as_slice()))?; - build_node(&parsed, depth + 1, defer)? + // Copies below here, not offsets: `raw` is a decode of this body and + // `repaired` a rebuild of that, and both are dropped at the end of + // this block while the nodes built from them outlive it. Leaves below + // an embedded message therefore keep what they kept before #239 -- + // the borrow this mode is about is the borrow of the caller's + // payload, and these bytes were never in it. Metadata mode still + // keeps nothing, here as everywhere. + let inside = match retain { + Retain::Nothing => Retain::Nothing, + Retain::In(_) | Retain::Copy => Retain::Copy, + }; + build_node(&parsed, depth + 1, inside)? }; let body = NodeBody::Decoded { encoded_size: encoded_size(&part.get_body_encoded()), @@ -1190,12 +1340,15 @@ fn build_node( } else { let body = NodeBody::Undecoded { encoded_size: encoded_size(&part.get_body_encoded()), - // The one copy lazy mode makes, and the encoded part rather than the - // decoded one. For the root of a single-part message that is the - // whole payload, since mailparse's `raw_bytes` for a root is the - // message -- see the note in the binding layer. Metadata mode keeps - // nothing. - raw: defer.then(|| part.raw_bytes.to_vec()), + // Offsets into the buffer being parsed, not a copy of it. For the + // root of a single-part message that range is the whole payload, + // since mailparse's `raw_bytes` for a root is the message -- see the + // note in the binding layer. Metadata mode keeps nothing. + raw: match retain { + Retain::Nothing => None, + Retain::In(base) => Some(Retained::of(base, part.raw_bytes)), + Retain::Copy => Some(Retained::Owned(part.raw_bytes.to_vec())), + }, }; (body, Vec::new()) }; @@ -1566,6 +1719,100 @@ mod tests { /// only ever sees through a dict. const SIMPLE: &[u8] = b"Subject: hi\r\nFrom: a@example.com\r\n\r\nbody\r\n"; + /// The fixtures for the retention tests below. `#[test]` builds run in debug, + /// where `Retained::of`'s assertion is live -- so a leaf that stopped being a + /// subslice of the parsed buffer and started being copied fails here rather + /// than becoming a quiet performance regression. + const WITH_ATTACHMENT: &[u8] = b"Subject: has one\r\n\ + Content-Type: multipart/mixed; boundary=\"b\"\r\n\r\n\ + --b\r\nContent-Type: text/plain\r\n\r\nhello\r\n\ + --b\r\nContent-Type: application/pdf\r\n\ + Content-Disposition: attachment; filename=doc.pdf\r\n\ + Content-Transfer-Encoding: base64\r\n\r\ncGRmIGJ5dGVz\r\n--b--\r\n"; + + /// The same message with the blank line after the header block removed, which + /// is what makes the parse run over a rebuilt copy (#150). + const UNTERMINATED: &[u8] = b"Subject: has one\r\n\ + Content-Type: multipart/mixed; boundary=\"b\"\r\n\ + this line has no colon\r\n\ + --b\r\nContent-Type: text/plain\r\n\r\nhello\r\n\ + --b\r\nContent-Type: application/pdf\r\n\ + Content-Disposition: attachment; filename=doc.pdf\r\n\ + Content-Transfer-Encoding: base64\r\n\r\ncGRmIGJ5dGVz\r\n--b--\r\n"; + + #[test] + fn a_deferred_attachment_is_a_range_and_not_a_copy() { + let mail = parse_email_lazy(WITH_ATTACHMENT).expect("the fixture must parse"); + let attachment = &mail.attachments[0]; + + assert!( + matches!(attachment.raw, Retained::Range(_)), + "a leaf of a message parsed in one piece must be retained as offsets, \ + not copied: {:?}", + attachment.raw + ); + assert!(mail.repaired.is_none(), "this fixture needs no repair"); + assert_eq!( + decode_part(attachment.raw.slice(WITH_ATTACHMENT)).expect("decodes"), + b"pdf bytes", + ); + } + + #[test] + fn a_repaired_message_retains_ranges_into_the_rebuilt_buffer() { + let mail = parse_email_lazy(UNTERMINATED).expect("the fixture must parse"); + let repaired = mail + .repaired + .as_deref() + .expect("the header block was unterminated"); + let attachment = &mail.attachments[0]; + + // The offsets index the rebuild, which is one byte longer than the + // payload. Resolving them against the payload instead is exactly the + // off-by-one this pairing exists to prevent, so assert the right buffer + // gives the right bytes rather than merely that some buffer does. + assert_eq!(repaired.len(), UNTERMINATED.len() + 1); + assert_eq!( + decode_part(attachment.raw.slice(repaired)).expect("decodes"), + b"pdf bytes", + ); + } + + #[test] + fn a_leaf_below_an_embedded_message_is_copied() { + // The one case that cannot borrow: the bytes were produced by decoding + // the enclosing body, and that buffer is dropped when the walk leaves the + // subtree. Asserted so the copy stays deliberate. + let mut payload = b"Content-Type: message/rfc822\r\n\r\n".to_vec(); + payload.extend_from_slice(WITH_ATTACHMENT); + let tree = parse_tree_deferred(&payload, true).expect("the fixture must parse"); + + let root = match &tree.root.body { + NodeBody::Decoded { .. } => &tree.root, + other => panic!("the root should have been decoded to reach its child: {other:?}"), + }; + let leaves = collect_undecoded(&root.children[0]); + + assert!(!leaves.is_empty(), "the embedded message must have leaves"); + for leaf in leaves { + assert!( + matches!(leaf, Retained::Owned(_)), + "a leaf below an embedded message has no buffer to borrow: {leaf:?}" + ); + } + } + + fn collect_undecoded(node: &TreeNode) -> Vec<&Retained> { + let mut out = Vec::new(); + if let NodeBody::Undecoded { raw: Some(raw), .. } = &node.body { + out.push(raw); + } + for child in &node.children { + out.extend(collect_undecoded(child)); + } + out + } + #[test] fn a_plain_message_parses_with_no_warnings() { let mail = parse_email(SIMPLE).expect("a well-formed message must parse"); diff --git a/fast_mail_parser/__init__.pyi b/fast_mail_parser/__init__.pyi index 1b8823e..fc12e50 100644 --- a/fast_mail_parser/__init__.pyi +++ b/fast_mail_parser/__init__.pyi @@ -134,9 +134,11 @@ class PyLazyMimePart: has nothing to decode, and the second was decoded during the walk because its body is the embedded message that gives it children. - Memory: a leaf retains a copy of itself as it sits in the message. For a - single-part message that is the whole payload, because the root is the leaf -- - so this is for walking a large multipart message, not for small mail in bulk. + Memory: a leaf remembers where it sits in the payload rather than copying + itself out of it, so the tree costs its structure and not its bytes -- and it + keeps that payload alive for as long as any node of it is reachable. The + leaves inside a ``message/rfc822`` node are the exception and hold copies, + because the bytes they were parsed from were produced by decoding that node. A separate type rather than a lazy ``PyMimePart.content``, for the reason ``PyLazyAttachment`` is one: re-timing an existing attribute, and moving where @@ -343,6 +345,12 @@ class PyLazyAttachment: ``is_decoded`` says whether ``content`` has been decoded yet -- so whether reading it is free or is about to cost a decode. + Memory: the part points into the payload it was parsed from rather than + copying itself out of it, and keeps that payload alive for as long as it is + reachable. Holding one attachment of a large message holds the message. + ``content`` is a decoded copy, so a caller who wants the bytes without the + message reads it and drops the attachment. + A separate type rather than a lazy ``PyAttachment.content``: changing what an existing attribute costs, and when it raises, is a change to a shipped contract, and those batch into one API-v2 window. Adding a type is not. @@ -532,10 +540,10 @@ def parse_email(payload: str | bytes, *, strict: bool = False) -> PyMail: ``mode="lazy"`` decodes the bodies as usual and defers each attachment: ``PyLazyAttachment.content`` decodes on first access and caches, so an - attachment nobody reads is never decoded. Returns a ``PyLazyMail``. It trades - memory for decoding -- the encoded bytes of every attachment are retained, - and base64 is about 1.33x the size of what it encodes -- so it is for - selective extraction, not for reading everything anyway. + attachment nobody reads is never decoded. Returns a ``PyLazyMail``. A + deferred attachment points into ``payload`` rather than copying itself out of + it, so the result keeps ``payload`` alive for as long as any attachment of it + is reachable -- selective extraction, not a way to hold a mailbox undecoded. ``mode="metadata"`` reads the headers and the attachment inventory without transfer-decoding anything, and returns a ``PyMailMetadata``. On an @@ -602,6 +610,7 @@ def parse_email_tree(payload: str | bytes) -> PyMimePart: * ``"full"`` (the default) returns a ``PyMimePart`` tree, every leaf decoded. * ``"lazy"`` returns a ``PyLazyMimePart`` tree, each leaf decoded on first access and cached -- walk a large message and decode the one part you want. + A leaf points into ``payload``, so the tree keeps it alive. * ``"metadata"`` returns a ``PyMimePartMetadata`` tree, which decodes nothing and retains nothing; a leaf reports ``encoded_size`` instead of ``content``. @@ -695,9 +704,10 @@ def parse_many( the mode was built for -- headers for a whole mailbox without decoding any of it. ``strict=True`` is rejected with ``ValueError``, as on ``parse_email``, because the mode never reads the bodies. - * ``"lazy"``: each slot is a ``PyLazyMail``. Note that this retains the - encoded bytes of every attachment in the batch for as long as the result - lives, so it is for pulling a few parts out of a batch rather than for + * ``"lazy"``: each slot is a ``PyLazyMail``. Each slot's attachments point + into that slot's own payload and keep it alive, so keeping one attachment + keeps one message rather than the batch -- but holding every slot holds + every payload. It is for pulling a few parts out of a batch rather than for holding a large one undecoded. ``ParseError`` instances still occupy failed slots in every mode, and diff --git a/fuzz/fuzz_targets/parse_agreement.rs b/fuzz/fuzz_targets/parse_agreement.rs index 7de4713..dd46865 100644 --- a/fuzz/fuzz_targets/parse_agreement.rs +++ b/fuzz/fuzz_targets/parse_agreement.rs @@ -28,11 +28,12 @@ //! flat parse's headers -- the root is the same message, so they cannot differ //! without one derivation having drifted from the other. //! 7. **A deferred decode equals the full parse's content.** This is the load- -//! bearing one for lazy mode: it retains a copy of each part exactly as it sits -//! in the message and decodes that copy on demand, which reproduces the full -//! parse's bytes only if mailparse's `raw_bytes` really is that part and -//! nothing else. Arbitrary input is the right place to test a claim about a -//! parser's slicing. The envelope, the bodies and the whole warning list must +//! bearing one for lazy mode: it retains each part's *offsets* in the buffer it +//! was parsed from (#239) and decodes those bytes on demand, which reproduces +//! the full parse's bytes only if mailparse's `raw_bytes` really is that part +//! and the offsets really are its offsets. Arbitrary input is the right place +//! to test a claim about a parser's slicing, and doubly so now that the claim +//! is arithmetic: a range that is off by one still decodes, to something else. The envelope, the bodies and the whole warning list must //! agree too -- the last of those is what lets `strict=True` mean the same //! thing in both modes. //! 8. **A tree is the same tree in all three modes** (#202). `mode=` on @@ -145,8 +146,14 @@ fn full_bodies(part: &mail_parser::MimePart, out: &mut Vec>>) { /// A leaf that cannot decode from its retained bytes returns an `Err`, which is a /// failure of the retention rather than of the message -- the full parse decoded /// the same part. +/// +/// `base` is the buffer a leaf's offsets index (#239): the input, or the repaired +/// rebuild the tree carries when the header block had to be resynced. Resolving +/// the range here is what makes this check cover the offsets themselves -- a +/// range that is off by a byte decodes to something else, or fails to decode. fn deferred_bodies( node: &mail_parser::TreeNode, + base: &[u8], out: &mut Vec>>, ) -> Result<(), ()> { match &node.body { @@ -154,11 +161,13 @@ fn deferred_bodies( mail_parser::NodeBody::Decoded { content, .. } => out.push(Some(content.clone())), mail_parser::NodeBody::Undecoded { raw, .. } => { let raw = raw.as_ref().ok_or(())?; - out.push(Some(mail_parser::decode_part(raw).map_err(|_| ())?)); + out.push(Some( + mail_parser::decode_part(raw.slice(base)).map_err(|_| ())?, + )); } } for child in &node.children { - deferred_bodies(child, out)?; + deferred_bodies(child, base, out)?; } Ok(()) } @@ -273,6 +282,11 @@ fuzz_target!(|data: &[u8]| { "attachment count disagrees" ); + // What the attachments' offsets index: the input, unless the header block + // had to be repaired, in which case the parse ran over the rebuild and + // the result carries it (#239). + let lazy_base = lazy.repaired.as_deref().unwrap_or(data); + for (decoded, deferred) in full.attachments.iter().zip(&lazy.attachments) { assert_eq!(decoded.mimetype, deferred.mimetype, "mimetype disagrees"); assert_eq!(decoded.filename, deferred.filename, "filename disagrees"); @@ -289,7 +303,7 @@ fuzz_target!(|data: &[u8]| { // what the full parse decoded from it. A part the full parse decoded // must also decode from its retained bytes -- if it cannot, the // retained slice is not the part. - let content = mail_parser::decode_part(&deferred.raw).expect( + let content = mail_parser::decode_part(deferred.raw.slice(lazy_base)).expect( "a part the full parse decoded failed to decode from its \ retained bytes", ); @@ -358,12 +372,12 @@ fuzz_target!(|data: &[u8]| { let shape = canonical_full(full); assert_eq!( shape, - canonical_node(&described), + canonical_node(&described.root), "the metadata tree's shape disagrees with full mode" ); assert_eq!( shape, - canonical_node(&deferred), + canonical_node(&deferred.root), "the lazy tree's shape disagrees with full mode" ); @@ -371,9 +385,9 @@ fuzz_target!(|data: &[u8]| { // must be one answer across the two deferred modes -- `encoded_size is // None` is the same question as `content is None`. let mut sizes = Vec::new(); - node_sizes(&described, &mut sizes); + node_sizes(&described.root, &mut sizes); let mut deferred_sizes = Vec::new(); - node_sizes(&deferred, &mut deferred_sizes); + node_sizes(&deferred.root, &mut deferred_sizes); assert_eq!( sizes, deferred_sizes, "the two deferred modes disagree about encoded sizes" @@ -406,7 +420,11 @@ fuzz_target!(|data: &[u8]| { // The claim the lazy tree rests on, and the one flat lazy mode cannot // reach: a retained *root* re-parses to the part it was taken from. let mut produced = Vec::new(); - deferred_bodies(&deferred, &mut produced) + deferred_bodies( + &deferred.root, + deferred.repaired.as_deref().unwrap_or(data), + &mut produced, + ) .expect("a node the full parse decoded failed to decode from its retained bytes"); assert_eq!( expected, produced, diff --git a/src/fast_mail_parser.rs b/src/fast_mail_parser.rs index fe0868d..035c3fb 100644 --- a/src/fast_mail_parser.rs +++ b/src/fast_mail_parser.rs @@ -25,7 +25,7 @@ use pyo3::pybacked::{PyBackedBytes, PyBackedStr}; use pyo3::types::{PyBytes, PyDateTime, PyDict, PyList, PyString, PyTzInfo}; use pyo3::{create_exception, exceptions, wrap_pyfunction}; use std::num::NonZeroUsize; -use std::sync::OnceLock; +use std::sync::{Arc, OnceLock}; /// Header pairs in wire order, plus the one Python `dict` they project to. /// @@ -535,10 +535,11 @@ impl PyMimePartMetadata { /// re-timing an existing attribute, and moving where it raises, is a change to a /// shipped contract that #104 batches into an API-v2 window. /// -/// Memory: a leaf retains a copy of itself as it sits in the message. For a -/// single-part message that is the whole payload, because the root *is* the leaf -/// -- so this mode is for walking a large multipart message and decoding one part -/// of it, which is what the tree is for, and not for small mail in bulk. +/// Memory: a leaf points at itself where it sits in the caller's payload, so the +/// tree costs its headers and not its bytes -- but it pins that payload for as +/// long as any node of it is alive (#239). A leaf below a `message/rfc822` node +/// is the exception and holds a copy, because the bytes it was parsed from were +/// produced by decoding that node and exist in no caller buffer. #[pyclass(frozen, skip_from_py_object)] pub struct PyLazyMimePart { #[pyo3(get)] @@ -560,7 +561,7 @@ pub struct PyLazyMimePart { pub encoded_size: Option, /// The part as it sits in the message, still encoded. `None` for a container /// and for a `message/rfc822` node, whose bytes are already published below. - raw: Option>, + raw: Option, /// The decoded bytes, published exactly once -- as on `PyLazyAttachment`, so /// that `part.content is part.content`. content: OnceLock>, @@ -600,7 +601,7 @@ impl PyLazyMimePart { let Some(raw) = self.raw.as_ref() else { return Ok(None); }; - self.decode(py, raw).map(Some) + self.decode(py, raw.bytes()).map(Some) } /// Whether reading `content` is free. @@ -668,11 +669,15 @@ fn metadata_node(py: Python<'_>, node: mail_parser::TreeNode) -> PyResult, node: mail_parser::TreeNode) -> PyResult { +fn lazy_node( + py: Python<'_>, + node: mail_parser::TreeNode, + base: &Arc, +) -> PyResult { let children = node .children .into_iter() - .map(|child| Py::new(py, lazy_node(py, child)?)) + .map(|child| Py::new(py, lazy_node(py, child, base)?)) .collect::>>()?; // A `message/rfc822` body was decoded to reach the children below it, so it @@ -680,9 +685,11 @@ fn lazy_node(py: Python<'_>, node: mail_parser::TreeNode) -> PyResult (None, None, OnceLock::new()), - mail_parser::NodeBody::Undecoded { encoded_size, raw } => { - (Some(encoded_size), raw, OnceLock::new()) - } + mail_parser::NodeBody::Undecoded { encoded_size, raw } => ( + Some(encoded_size), + raw.map(|at| RetainedBytes::new(base, at)), + OnceLock::new(), + ), mail_parser::NodeBody::Decoded { encoded_size, content, @@ -988,6 +995,11 @@ impl PyMail { /// of `PyAttachment` plus `encoded_size` and `is_decoded`, with `content` a /// property that does the work rather than a value the parse already paid for. /// +/// Memory: the part points into the payload it was parsed from and keeps it +/// alive, so holding one attachment of a large message holds the message (#239). +/// `content` is a decoded copy, so a caller who wants the bytes and not the +/// message reads it and drops the attachment. +/// /// A new type rather than making `PyAttachment.content` lazy. Changing what an /// existing attribute costs -- and when it raises -- is a change to a shipped /// contract, and #104 batches those into one API-v2 window; adding a type is not @@ -1015,8 +1027,11 @@ pub struct PyLazyAttachment { /// that required a decode to obtain would defeat it. #[pyo3(get)] pub encoded_size: usize, - /// The part as it sits in the message, still encoded. - raw: Vec, + /// Where the part sits in the message, still encoded, and the buffer that + /// keeps those bytes alive (#239). Offsets rather than a copy: the bytes are + /// already in the payload the caller passed in, and duplicating them is what + /// deferring the decode is meant to avoid. + raw: RetainedBytes, /// The decoded bytes, published exactly once. /// /// `OnceLock>` rather than `OnceLock>` so that repeated @@ -1083,14 +1098,14 @@ impl PyLazyAttachment { } impl PyLazyAttachment { - fn from_lazy(attachment: mail_parser::LazyAttachment) -> Self { + fn from_lazy(attachment: mail_parser::LazyAttachment, base: &Arc) -> Self { PyLazyAttachment { mimetype: attachment.mimetype, filename: attachment.filename, content_id: attachment.content_id, disposition: attachment.disposition, encoded_size: attachment.encoded_size, - raw: attachment.raw, + raw: RetainedBytes::new(base, attachment.raw), content: OnceLock::new(), } } @@ -1108,16 +1123,17 @@ impl PyLazyAttachment { /// object, holds without a lock. /// /// The GIL is released for the decode, so several threads pulling different - /// attachments overlap rather than serialise. `raw` is owned by this object - /// and this object is immutable, so nothing can move underneath the slice - /// while it is detached. + /// attachments overlap rather than serialise. The slice is a range of a buffer + /// this object holds an `Arc` to, and that buffer is either an immutable + /// Python object or a `Vec` nothing else can reach, so nothing can move + /// underneath it while it is detached. /// /// `#[cold]` and out of line: it runs at most once per attachment, and the /// hot path through the getter above is the cached one. #[cold] #[inline(never)] fn decode<'py>(&self, py: Python<'py>) -> PyResult> { - let raw = self.raw.as_slice(); + let raw = self.raw.bytes(); let decoded = py .detach(|| mail_parser::decode_part(raw)) .map_err(to_py_err)?; @@ -1202,11 +1218,16 @@ impl PyLazyMail { /// reaches this, and cold binding code in this module has already cost the /// hot path 24% through nothing but lost inlining (#99). #[inline(never)] - fn from_lazy(py: Python<'_>, mail: mail_parser::LazyMail) -> PyResult { + fn from_lazy(py: Python<'_>, mail: mail_parser::LazyMail, payload: Payload) -> PyResult { + // The pin is built here, from this message's own payload and its own + // repair, so a batch gets one per slot rather than one shared across the + // batch -- reading one attachment out of message 3 must not keep messages + // 1 and 2 alive. + let base = Pinned::new(payload, mail.repaired); let attachments = mail .attachments .into_iter() - .map(|part| Py::new(py, PyLazyAttachment::from_lazy(part))) + .map(|part| Py::new(py, PyLazyAttachment::from_lazy(part, &base))) .collect::>>()?; Ok(PyLazyMail { @@ -1289,6 +1310,84 @@ fn payload_to_bytes(payload: &Py, py: Python<'_>) -> PyResult { )) } +/// The buffer a deferred part's byte range indexes, kept alive for exactly as +/// long as some part still points into it (#239). +/// +/// This is the memory contract lazy mode now has, and it is a real change: a +/// result that borrows its bytes pins the message it was parsed from. Holding one +/// attachment of a 100 MB mail holds the 100 MB. That is the right default here +/// -- the alternative is the copy this mode exists to avoid, and the caller who +/// wants the bytes without the payload can ask for `content`, which is a copy by +/// construction, and drop the attachment. +/// +/// Both fields are needed because the buffer the parse ran over is not always the +/// one the caller passed: a message whose header block was never terminated is +/// rebuilt first (#150), and the ranges then index the rebuild. Whichever it is, +/// its address is stable across the move into this struct -- a `Vec` owns a heap +/// allocation, and PyO3's backed types point into a Python object -- which is what +/// makes it sound to take the ranges during the parse and build the pin after it. +pub(crate) struct Pinned { + payload: Payload, + repaired: Option>, +} + +impl Pinned { + fn new(payload: Payload, repaired: Option>) -> Arc { + let before = payload.as_ref().as_ptr(); + let pinned = Pinned { payload, repaired }; + debug_assert!( + std::ptr::eq(pinned.payload.as_ref().as_ptr(), before), + "moving the payload moved the bytes it points at, so ranges taken \ + during the parse no longer index it" + ); + Arc::new(pinned) + } + + /// The buffer the parse ran over, which is the one the ranges index. + fn base(&self) -> &[u8] { + match &self.repaired { + Some(repaired) => repaired, + None => self.payload.as_ref(), + } + } +} + +/// One deferred part's bytes: where they are, and what keeps them there. +/// +/// `Arc` rather than one handle on the message: a part can outlive the +/// `PyLazyMail` or the tree node that produced it, because Python hands out +/// references and a caller can keep the one attachment they wanted. Refcounting +/// the buffer is what makes "keep this attachment, drop everything else" mean the +/// payload is released when the last of them goes. +/// +/// An enum rather than a pin plus a `Retained`, so that the part that owns its +/// bytes holds no pin at all. A leaf inside a `message/rfc822` node is the only +/// one that does, and its bytes came from decoding that node rather than from the +/// payload -- pinning the payload for it would keep a whole message alive on +/// behalf of bytes that are not in it. +pub(crate) enum RetainedBytes { + /// A range of a buffer this part keeps alive. + In(Arc, std::ops::Range), + /// Bytes of its own, for a part that was in no caller buffer. + Owned(Vec), +} + +impl RetainedBytes { + fn new(base: &Arc, at: mail_parser::Retained) -> Self { + match at { + mail_parser::Retained::Range(range) => RetainedBytes::In(Arc::clone(base), range), + mail_parser::Retained::Owned(bytes) => RetainedBytes::Owned(bytes), + } + } + + fn bytes(&self) -> &[u8] { + match self { + RetainedBytes::In(base, range) => &base.base()[range.clone()], + RetainedBytes::Owned(bytes) => bytes, + } + } +} + /// Parse a raw email (`bytes` or `str`) into a [`PyMail`]. /// /// Raises `ParseError`, or more precisely one of its subtypes: @@ -1303,8 +1402,10 @@ fn payload_to_bytes(payload: &Py, py: Python<'_>) -> PyResult { /// that reads the bodies, so `"full"` or `"lazy"`. /// /// `mode="lazy"` returns a [`PyLazyMail`]: the bodies decoded as today, and each -/// attachment's content decoded on first access and cached. `mode="metadata"` -/// returns a [`PyMailMetadata`] and decodes nothing at all. +/// attachment's content decoded on first access and cached. A deferred +/// attachment points into `payload` rather than copying itself out of it, so the +/// result keeps `payload` alive -- see [`Pinned`]. `mode="metadata"` returns a +/// [`PyMailMetadata`] and decodes nothing at all, and pins nothing. #[pyfunction] #[pyo3(signature = (payload, *, mode = "full", strict = false))] pub fn parse_email( @@ -1462,14 +1563,13 @@ fn parse_lazy_inner(py: Python<'_>, payload: Py) -> PyResult let message = payload_to_bytes(&payload, py)?; // The GIL is released for the parse, as in every other mode. What the parse - // retains per attachment is a copy of that part's encoded bytes, so nothing - // borrows from the caller's payload once this returns -- which is what lets - // the attachments outlive it. + // retains per attachment is that part's offsets in the payload, so the + // payload is moved into the result rather than dropped here -- see `Pinned`. let mail = py .detach(|| mail_parser::parse_email_lazy(message.as_ref())) .map_err(to_py_err)?; - PyLazyMail::from_lazy(py, mail) + PyLazyMail::from_lazy(py, mail, message) } fn parse_email_inner(py: Python<'_>, payload: Py) -> PyResult { @@ -1725,11 +1825,12 @@ fn parse_many_metadata( /// per slot the way full mode honours it: lazy mode finds every repair the full /// parse finds, so the verdict is the same verdict. /// -/// Worth a caution the single-message mode does not need: this retains the -/// encoded bytes of every attachment in the *whole batch* until the batch is -/// dropped, which is the opposite of what the mode saves on one message. Use it -/// to sweep a batch and pull a few parts out of it, not to hold ten thousand -/// messages' attachments undecoded. +/// Worth a caution the single-message mode does not need: each slot's +/// attachments pin that slot's payload (#239), so holding the whole batch holds +/// every payload in it. One pin per slot rather than one for the batch, so +/// keeping one attachment keeps one message -- but the batch as a whole is still +/// the batch. Use it to sweep a batch and pull a few parts out of it, not to hold +/// ten thousand messages undecoded. #[inline(never)] fn parse_many_lazy( py: Python<'_>, @@ -1748,13 +1849,17 @@ fn parse_many_lazy( let parsed = py.detach(|| mail_parser::parse_many_as(&messages, workers, mail_parser::parse_email_lazy)); + // Zipped, not indexed: each result is paired with the payload it was parsed + // from, and that payload is what its attachments' offsets index. The borrow + // `parse_many_as` took ended when it returned, so the payloads can be moved + // into the results one by one -- one pin per slot, never one for the batch. let items = PyList::empty(py); - for result in parsed { + for (message, result) in messages.into_iter().zip(parsed) { // Folded into one `Err` arm as in full mode: one notion of "this slot // failed", two ways to reach it. let outcome = match result { Ok(mail) => { - let mail = PyLazyMail::from_lazy(py, mail)?; + let mail = PyLazyMail::from_lazy(py, mail, message)?; if strict && !mail.warnings.is_empty() { Err(strict_rejection(&mail.warnings)) } else { @@ -1833,9 +1938,12 @@ fn parse_email_tree_other_mode( .map_err(to_py_err)?; if defer { - return Ok(Py::new(py, lazy_node(py, tree)?)?.into_any()); + let base = Pinned::new(message, tree.repaired); + return Ok(Py::new(py, lazy_node(py, tree.root, &base)?)?.into_any()); } - Ok(Py::new(py, metadata_node(py, tree)?)?.into_any()) + // Metadata mode retains nothing, so the payload is dropped here as it + // always was. + Ok(Py::new(py, metadata_node(py, tree.root)?)?.into_any()) }) } diff --git a/tests/benchmark/test_read_message.py b/tests/benchmark/test_read_message.py index ba0d228..b9a811b 100644 --- a/tests/benchmark/test_read_message.py +++ b/tests/benchmark/test_read_message.py @@ -795,9 +795,10 @@ def test__fast_mail_parser___parse_lazy_untouched(large_message: str, benchmark: def test__fast_mail_parser___parse_lazy_all_attachments(large_message: str, benchmark: Callable): # The other end of the trade, measured rather than asserted: lazy mode plus # reading every attachment does the full parse's work in a worse order -- - # a copy of each part's encoded bytes, then a re-parse of its headers per - # attachment. Whoever is going to decode everything anyway should use the - # default mode, and this is the number that says so. + # a re-parse of each part's headers before its decode. Since #239 the copy is + # gone from that list, so the remaining gap is the re-parse alone. Whoever is + # going to decode everything anyway should use the default mode, and this is + # the number that says so. import pytest from fast_mail_parser import parse_email @@ -841,9 +842,11 @@ def test__fast_mail_parser___parse_tree_metadata(large_message: str, benchmark: def test__fast_mail_parser___parse_tree_lazy_untouched(large_message: str, benchmark: Callable): - # The other deferred tree mode with nothing read. It retains a copy of every - # leaf where metadata mode retains nothing, so the gap between this and the - # benchmark above is the price of being able to decode one part later. + # The other deferred tree mode with nothing read. It retains each leaf's + # offsets where metadata mode retains nothing at all, so since #239 the gap + # between this and the benchmark above is the price of *being able* to decode + # one part later rather than the price of copying it: the two modes now + # allocate the same amount on the fixtures `bench/tests/allocs.rs` counts. import pytest from fast_mail_parser import parse_email_tree diff --git a/tests/test_lazy_mode.py b/tests/test_lazy_mode.py index 06e3e61..07c0778 100644 --- a/tests/test_lazy_mode.py +++ b/tests/test_lazy_mode.py @@ -8,11 +8,17 @@ bytes of every attachment in the whole corpus must be equal, and the envelope and the warning list must be equal too, because a pipeline that switches modes to save work must not switch what it reads. That equality is also what makes the -implementation trustworthy: lazy mode keeps a copy of each part exactly as it sits -in the message and re-parses that copy on access, which reproduces the full -parse's `content` only if `raw_bytes` really is the part and nothing else. The +implementation trustworthy: lazy mode remembers where each part sits in the buffer +it was parsed from and re-parses those bytes on access, which reproduces the full +parse's `content` only if `raw_bytes` really is the part and the offsets really +are its offsets -- an arithmetic slip reads the right number of bytes from the +wrong place, and equality against full mode is what catches it. The `parse_agreement` fuzz target asserts the same thing on arbitrary input. +**The payload is pinned.** Borrowing rather than copying (#239) means the result +keeps the message alive. That is a change to the memory contract, so it is +asserted rather than described -- see the section on `sys.getrefcount` below. + **The cache must be one object.** `a.content is a.content`, and the same across threads: a cache that hands back an equal copy is not a cache, it is a decode with extra steps. @@ -22,6 +28,7 @@ """ import glob import os +import sys import threading import pytest @@ -202,9 +209,11 @@ def test__content_is_bytes(attachment_message: str): def test__attachments_outlive_the_payload(attachment_message: str): # `bytes` payloads are borrowed rather than copied (#96), so a mode that - # retained a view into the caller's buffer would be a use-after-free waiting - # for a garbage collection. Lazy mode copies each part's encoded bytes; this - # is what says so. + # retained a view into the caller's buffer without keeping that buffer alive + # would be a use-after-free waiting for a garbage collection. Since #239 lazy + # mode does retain a view -- and keeps the payload alive for exactly as long + # as some part still points into it. This is what says so: the caller's last + # reference goes and the bytes are still there and still right. payload = attachment_message.encode("utf-8") expected = [a.content for a in parse_email(payload).attachments] @@ -214,6 +223,134 @@ def test__attachments_outlive_the_payload(attachment_message: str): assert [a.content for a in attachments] == expected +# --- what the borrow costs: the payload is pinned (#239) ----------------------- +# +# Lazy mode stopped copying every deferred part out of the message, which is the +# opposite of what deferring is for. What it does instead is hold the payload and +# remember where each part sits in it, and that is a change to the memory contract +# worth asserting rather than describing: the result keeps the message alive. +# +# `sys.getrefcount` is the instrument. It is an implementation detail of CPython +# and these tests would need rewriting on another runtime, but it is the only way +# to observe the pin directly -- the alternative is measuring RSS, which says +# "something is large" and not "this object is held by that one". + + +def _refs(obj) -> int: + # One less than `getrefcount` reports, because the argument itself is a + # reference. The absolute number is never asserted; only what it does. + return sys.getrefcount(obj) - 1 + + +def test__an_attachment_pins_the_payload_it_was_parsed_from(attachment_message: str): + payload = attachment_message.encode("utf-8") + before = _refs(payload) + + mail = parse_email(payload, mode="lazy") + assert _refs(payload) > before, ( + "a lazy parse must hold the payload its attachments point into" + ) + + # The attachment alone is enough: a caller who keeps the one part they wanted + # and drops the message still has bytes to decode. + attachment = mail.attachments[0] + del mail + assert _refs(payload) > before + + assert attachment.content + del attachment + assert _refs(payload) == before, ( + "dropping the last part that points into the payload must release it" + ) + + +def test__a_message_with_no_attachments_pins_nothing(valid_message: str): + # The pin is per part, not per parse. A message with nothing deferred has + # nothing pointing into the payload, so the parse must not be what keeps a + # large message alive when it kept nothing from it. + payload = valid_message.encode("utf-8") + before = _refs(payload) + + mail = parse_email(payload, mode="lazy") + + assert not mail.attachments, "fixture must have no attachments" + assert _refs(payload) == before + + +def test__reading_content_does_not_release_the_payload(attachment_message: str): + # `content` caches a decoded copy, but the part still knows where it came + # from -- there is no point at which the pin is dropped early, because a + # second attachment of the same message may not have been read yet. + payload = attachment_message.encode("utf-8") + before = _refs(payload) + + attachment = parse_email(payload, mode="lazy").attachments[0] + assert attachment.content + + assert _refs(payload) > before + + +def test__each_slot_of_a_batch_pins_only_its_own_payload(attachment_message: str): + # A batch must not be all-or-nothing: keeping one attachment out of message + # three must not keep messages one and two alive as well. + from fast_mail_parser import parse_many + + payloads = [attachment_message.encode("utf-8") for _ in range(3)] + before = [_refs(p) for p in payloads] + + parsed = parse_many(payloads, mode="lazy") + kept = parsed[2].attachments[0] + del parsed + + assert [_refs(p) for p in payloads] == [before[0], before[1], before[2] + 1] + assert kept.content + + +REPAIRED_WITH_ATTACHMENT = ( + b"Subject: no separator\r\n" + b'Content-Type: multipart/mixed; boundary="b"\r\n' + b"this line has no colon, so the header block never ended\r\n" + b"--b\r\n" + b"Content-Type: text/plain\r\n" + b"\r\n" + b"hello\r\n" + b"--b\r\n" + b"Content-Type: application/pdf\r\n" + b"Content-Disposition: attachment; filename=doc.pdf\r\n" + b"Content-Transfer-Encoding: base64\r\n" + b"\r\n" + b"cGRmIGJ5dGVz\r\n" + b"--b--\r\n" +) + + +def test__a_repaired_message_decodes_its_attachments(): + # The case a range-based retention has to get right and a copy never could + # get wrong: a message whose header block was never terminated is parsed from + # a rebuilt copy (#150), so the offsets index the rebuild and not the payload + # the caller passed. Getting that wrong reads the right number of bytes from + # the wrong place -- one byte off, and silently. + full = parse_email(REPAIRED_WITH_ATTACHMENT) + lazy = parse_email(REPAIRED_WITH_ATTACHMENT, mode="lazy") + + assert full.warnings, "fixture must trigger the separator repair" + assert [a.filename for a in lazy.attachments] == [a.filename for a in full.attachments] + assert [a.content for a in lazy.attachments] == [a.content for a in full.attachments] + + +def test__a_repaired_message_pins_its_payload_and_still_decodes(): + # A distinct object, so that the local really is the caller's last reference + # and the refcount below is this test's and not the module constant's. + payload = bytes(bytearray(REPAIRED_WITH_ATTACHMENT)) + before = _refs(payload) + + attachments = list(parse_email(payload, mode="lazy").attachments) + assert _refs(payload) > before + + del payload + assert [a.content for a in attachments] == [b"pdf bytes"] + + # --- laziness ----------------------------------------------------------------- diff --git a/tests/test_payload_memory.py b/tests/test_payload_memory.py index 4736550..7b2f13e 100644 --- a/tests/test_payload_memory.py +++ b/tests/test_payload_memory.py @@ -17,6 +17,14 @@ above what the call reaches — and the assertion would pass without measuring anything. It runs outside the repository root, so that the source package there does not shadow the installed extension. + +The lazy probe at the end of this file measures a payload that parses, and so +needs a third thing: a payload built on **disk** rather than in memory. Building +a 96 MiB payload in the probe means holding a temporary and the result at once, +which raises the water mark to twice the payload before the parser is reached — +and a parser that copied the whole thing would then fit underneath it and measure +as free. Reading it from a file is one allocation, so the mark starts at one +payload and a copy is visible as a second. """ import subprocess import sys @@ -137,3 +145,88 @@ def test__bytes_and_str_payloads_agree(valid_message: str): assert from_str.subject == from_bytes.subject assert from_str.text_plain == from_bytes.text_plain assert list(from_str.headers) == list(from_bytes.headers) + + +# --- what a lazy parse retains (#239) ----------------------------------------- + +# Lazy mode used to copy each deferred part out of the message, so a message that +# is one large attachment cost its own size again the moment it was parsed — +# while deferring the decode, which is the cost the mode exists to avoid. It now +# keeps offsets and pins the payload. +LAZY_PAYLOAD_MIB = 96 + +PROBE_LAZY = textwrap.dedent( + """ + import resource + import sys + + from fast_mail_parser import parse_email + + # Before the payload exists: what is measured is the payload plus whatever + # the parse adds, so a retained copy of the attachment shows up as a second + # payload rather than disappearing under the first one's water mark. + before = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss + with open(sys.argv[1], "rb") as handle: + payload = handle.read() + + mail = parse_email(payload, mode="lazy") + after = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss + + if len(mail.attachments) != 1: + sys.exit(f"expected one attachment, got {len(mail.attachments)}") + if mail.attachments[0].is_decoded: + sys.exit("the attachment was decoded by the parse") + + print((after - before) / 1024) + """ +) + + +def _write_one_big_attachment(path, mib: int) -> None: + # Written in chunks so this process never holds the payload either. It is not + # measured here, but a 96 MiB temporary in the test runner is worth avoiding + # on its own. + with open(path, "wb") as handle: + handle.write( + b"Subject: one big attachment\r\n" + b'Content-Type: multipart/mixed; boundary="b"\r\n' + b"\r\n" + b"--b\r\n" + b"Content-Type: text/plain\r\n" + b"\r\n" + b"hello\r\n" + b"--b\r\n" + b"Content-Type: application/octet-stream\r\n" + b"Content-Disposition: attachment; filename=big.bin\r\n" + b"Content-Transfer-Encoding: base64\r\n" + b"\r\n" + ) + chunk = b"A" * 1024 * 1024 + for _ in range(mib): + handle.write(chunk) + handle.write(b"\r\n--b--\r\n") + + +def test__a_lazy_parse_does_not_copy_the_attachment_it_defers(tmp_path): + message = tmp_path / "big.eml" + _write_one_big_attachment(message, LAZY_PAYLOAD_MIB) + + probe = subprocess.run( + [sys.executable, "-c", PROBE_LAZY, str(message)], + capture_output=True, + text=True, + cwd=tmp_path, + ) + + assert probe.returncode == 0, probe.stderr + growth = float(probe.stdout.strip()) + + # One payload, plus the allowance. Two payloads is what a retained copy costs + # and is the failure this is looking for, so the allowance has room to spare + # without letting that through. + allowed = LAZY_PAYLOAD_MIB + ALLOWED_GROWTH_MIB + assert growth < allowed, ( + f"peak memory grew {growth:.1f} MiB while parsing a {LAZY_PAYLOAD_MIB} MiB " + f"message whose attachment was never read; more than {allowed:.0f} MiB " + f"means the deferred part was copied rather than pointed at" + ) diff --git a/tests/test_tree_modes.py b/tests/test_tree_modes.py index f5f77c4..e05e8a3 100644 --- a/tests/test_tree_modes.py +++ b/tests/test_tree_modes.py @@ -28,6 +28,7 @@ """ import glob import os +import sys import pytest @@ -302,10 +303,11 @@ def test__the_children_attribute_hands_back_the_same_objects(): def test__the_tree_outlives_the_payload(): - # The parse copies each part rather than borrowing the caller's bytes, so - # dropping the payload cannot invalidate a deferred decode. - payload = bytearray(MIXED) - root = parse_email_tree(bytes(payload), mode="lazy") + # The parse borrows the caller's bytes and keeps the payload alive for as long + # as the tree is (#239), so dropping the caller's reference cannot invalidate + # a deferred decode. + payload = bytes(bytearray(MIXED)) + root = parse_email_tree(payload, mode="lazy") del payload assert list(walk(root))[1].content == b"plain version" @@ -313,7 +315,7 @@ def test__the_tree_outlives_the_payload(): def test__a_single_part_message_defers_its_root(): # The root is the leaf here, so there is no subpart to defer -- the whole - # payload is what gets retained and re-parsed. + # payload is the range that gets retained and re-parsed. root = parse_email_tree(SINGLE_PART, mode="lazy") assert root.children == [] @@ -350,6 +352,80 @@ def test__an_embedded_message_arrives_already_decoded(): assert embedded.content == parse_email_tree(_bounce(ORIGINAL)).children[1].content +def test__a_leaf_inside_an_embedded_message_still_decodes_in_lazy_mode(): + # The subtree that cannot borrow. Its bytes were produced by decoding the + # `message/rfc822` body, so the buffer they sit in is the parser's own and is + # dropped when the walk leaves the subtree -- these leaves keep copies (#239). + # Asserted from Python because the distinction is invisible from here and must + # stay that way: a leaf inside an embedded message decodes like any other. + root = parse_email_tree(_bounce(ORIGINAL), mode="lazy") + + embedded = next(part for part in walk(root) if part.is_message) + leaves = [part for part in walk(embedded.children[0]) if not part.children] + + assert leaves, "the embedded message must have leaves" + assert all(leaf.content is not None for leaf in leaves), ( + "a leaf inside an embedded message reported no body at all, which is what " + "a container reports -- its bytes were not retained" + ) + assert [leaf.content for leaf in leaves] == [ + leaf.content + for leaf in walk( + next( + part for part in walk(parse_email_tree(_bounce(ORIGINAL))) if part.is_message + ).children[0] + ) + if not leaf.children + ] + + +def test__a_lazy_tree_outlives_its_payload(): + # Every leaf of the tree at once: the ones that point into the payload, which + # the tree now pins, and the ones inside the embedded message, which copied. + payload = bytes(bytearray(_bounce(ORIGINAL))) + expected = [ + part.content for part in walk(parse_email_tree(payload)) if not part.children + ] + + root = parse_email_tree(payload, mode="lazy") + del payload + + assert [part.content for part in walk(root) if not part.children] == expected + + +def test__a_leaf_inside_an_embedded_message_pins_nothing(): + # It owns its bytes -- they were produced by decoding the `message/rfc822` + # body and are in no caller buffer -- so it must not keep the payload alive on + # their behalf. Keeping one leaf out of a bounce should not keep the bounce. + payload = bytes(bytearray(_bounce(ORIGINAL))) + before = sys.getrefcount(payload) + + root = parse_email_tree(payload, mode="lazy") + embedded = next(part for part in walk(root) if part.is_message) + leaf = next(part for part in walk(embedded.children[0]) if not part.children) + del root, embedded + + assert sys.getrefcount(payload) == before + assert leaf.content is not None + + +def test__a_lazy_tree_pins_the_payload_and_a_metadata_tree_does_not(): + # The two halves of the trade, in one assertion. A lazy tree keeps offsets, so + # it holds the message; a metadata tree keeps sizes, so it holds nothing. + payload = bytes(bytearray(_bounce(ORIGINAL))) + before = sys.getrefcount(payload) + + lazy = parse_email_tree(payload, mode="lazy") + assert sys.getrefcount(payload) > before + + del lazy + assert sys.getrefcount(payload) == before + + described = parse_email_tree(payload, mode="metadata") + assert sys.getrefcount(payload) == before + assert described.encoded_size is None, "the root of this fixture is a container" + + def test__metadata_mode_can_raise_on_a_broken_embedded_message(): # The documented exception to "metadata mode never decodes": it must decode a # `message/rfc822` body to parse the message inside it. `parse_email(...,