chunked data reading #543
Open
chunked data reading #543
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smtp_DATA() could buffer an unbounded amount of data in line_fragments until it got crlf and we're going to throw it away anyway. drop TestSMTPWithController.test_long_line_leak which is now moot
…g loop. DATA_CHUNK takes 3 parameters: data : bytes, decoded_data : Optional[str], last : bool and returns Optional[bytes] response If the hook returns a response prior to the last=True chunk, smtp_DATA will read/discard the remaining data from the client without invoking the hook again. This allows streaming the data to a file or other storage api without having to buffer the whole message in memory first. Move dotstuff and utf8 decode into data reading loop to support this. This may also improve/fix aio-libs#293 which I suspect is due to the tight loop decoding dotstuff for the whole message at once hogging the GIL in the old implementation.
otherwise do it at the end as before so we don't ~double the memory while we're reading from the client
1: buffer through BytesIO, not List[bytes], based on a simple microbenchmark, this is about 40% faster 2: when using the DATA_CHUNK hook, buffer SMTP(chunk_size=2**16) before calling the hook, the way this was before was calling the hook for every line
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
webknjaz
reviewed
Jun 18, 2025
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #543 +/- ##
==========================================
+ Coverage 97.82% 97.87% +0.04%
==========================================
Files 23 23
Lines 5701 5869 +168
Branches 766 796 +30
==========================================
+ Hits 5577 5744 +167
- Misses 78 79 +1
Partials 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This brings up a bit of a discussion point: the way I have it now, the new DATA_CHUNK hook returns Optional[bytes]. The idea is that it will typically return None if last=False unless there was an early-return error but the final call with last=True is always expected to return a value. This can't exactly be expressed with annotations so I needed some asserts to pass mypy.
webknjaz
reviewed
Jun 19, 2025
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
this is probably not common but gives the handler implementation another opportunity to preempt the data transfer in case of an error
…t response these changes increased the scope of the status variable in SMTP.smtp_DATA() that entailed an extra test of status is None in the backward-compatible path and it was easier to add the coverage than untangle that
I have encountered situations in the wild where someone was trying to send a large message over a slow connection that would require a very large static timeout (>hour) to accomodate. This way you can keep the timeout reasonably short (minutes) and it can take as long as it takes as long as the client keeps making forward progress. We could make this available to the current api under control of a flag but I don't want to unexpectedly change the behavior for exiseting users.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What do these changes do?
Add new handler hook DATA_CHUNK which is invoked from the data reading loop. This allows streaming the data to a file or other storage api without having to buffer the whole message in memory first.
Are there changes in behavior for the user?
The new hook is opt-in, there should be no behavior changes for existing users.
Related issue number
This may also improve/fix #293 which I suspect is due to the tight loop decoding dotstuff for the whole message at once hogging the GIL in the old implementation.
Checklist
{py36,py37,py38,py39}-{nocov,cov,diffcov}, qa, docs{py36,py37,py38,py39}-{nocov,cov,diffcov}{py36,py37,py38,py39}-{nocov,cov,diffcov}, pypy3-{nocov,cov}, qa, docs{py36,pypy3}-{nocov,cov,diffcov}, qapy36-{nocov,cov,diffcov}, qa, docsNEWS.rstfile