gh-127655: Ensure _SelectorSocketTransport.writelines pauses the protocol if needed

[bdraco]

gh-127655: Ensure writelines pauses the protocol if needed

Note for reviewers: 3.12+ only since writelines only exists since 3.12.

[bdraco]

Looks like the CI failure was GitHub being flakey and not an actual problem AFAICT

[gvanrossum]

I will review later today.

…

--Guido (mobile)

On Thu, Dec 5, 2024 at 14:25 Seth Michael Larson ***@***.***> wrote: @sethmlarson <https://github.com/sethmlarson> requested your review on: #127656 <#127656> gh-127655 <#127655>: Ensure _SelectorSocketTransport.writelines pauses the protocol if needed. — Reply to this email directly, view it on GitHub <#127656 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAWCWMR5R5HAQRB5UCXG5A32EDHGDAVCNFSM6AAAAABTDMMDRGVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJVGU2TCMZUGQ3DIMI> . You are receiving this because your review was requested.Message ID: ***@***.***>

[gvanrossum]

The one-line fix looks good.

The test doesn't really test anything except that the new logic is in place -- it doesn't prove at all that it does what it claims to do (prevent memory filling up if the other end never reads). But that's hard to test without bringing the test machine to its knees, so as long as you have tested this on one of your actual reproducing scenarios, I'm okay with this.

I suppose some people will be surprised that they are now paused by voluminous writelines() calls where previously they weren't. (What happens if you are paused but you keep calling writelines() anyway?) But consistency with write() is more important, and that may already pause. So I'm okay with this, since writelines() is supposed to be equivalent to multiple write() calls.

[bdraco]

New preview
https://cpython-previews--127656.org.readthedocs.build/en/127656/whatsnew/changelog.html#changelog

[kumaraditya303]

If the protocol is paused, the example in the PoC won't call writelines anymore until the buffer is drained.

Yeah but if you still continue to call writelines then I guess the memory will go unbounded right?

[kumaraditya303]

@python/organization-owners please add me so I can view the advisory

[bdraco]

If the protocol is paused, the example in the PoC won't call writelines anymore until the buffer is drained.

Yeah but if you still continue to call writelines then I guess the memory will go unbounded right?

Yes if the implementation doesn't drain or otherwise wait to send more when the protocol is paused, the same is true about write, but thats a downstream implementation problem.

[miss-islington-app]

Thanks @bdraco for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

[bedevere-app]

GH-127663 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-127664 is a backport of this pull request to the 3.12 branch.

[sethmlarson]

@kumaraditya303 Do you still need access to the advisory?

[gvanrossum]

FWIW, it was arguably a design mistake to make write() and writelines() regular functions that must not block, but it's well over a decade too late to fix that. So the convention is that code calling write() or writelines() must also occasionally await drain(). The docstring says it's supposed to be used like this:

          w.write(data)
          await w.drain()

and I'm sure that's the same for writelines(). The problem is that for streams, drain() doesn't do anything if the protocol isn't paused (check _drain_helper()). So an app that uses writelines() together with drain() could still be accumulating memory forever.

Thanks @bdraco for finding, reporting and fixing this!

[kumaraditya303]

Do you still need access to the advisory?

@sethmlarson Not for this but do add me for any future asyncio issues.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot aarch64 Android 3.x has failed when building commit e991ac8.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/1594/builds/745) and take a look at the build logs.
4. Check if the failure is related to this commit (e991ac8) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/1594/builds/745

Failed tests:

• test_android

Failed subtests:

• test_str - test.test_android.TestAndroidOutput.test_str

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_android.py", line 63, in assert_log
    line = self.logcat_queue.get(timeout=(deadline - time()))
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/queue.py", line 212, in get
    raise Empty
_queue.Empty


Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/base_events.py", line 687, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/Volumes/InternalDisk/Users/android/buildarea/3.x.mhsmith-android-aarch64/build/Android/android.py", line 531, in run_testbed
    async with asyncio.TaskGroup() as tg:
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/taskgroups.py", line 134, in __aexit__
    raise propagate_cancellation_error
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/asyncio/taskgroups.py", line 110, in __aexit__
    await self._on_completed_fut
asyncio.exceptions.CancelledError


Traceback (most recent call last):
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_android.py", line 53, in setUp
    self.assert_log("I", tag, message, skip=True, timeout=5)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/data/user/0/org.python.testbed/files/python/lib/python3.14/test/test_android.py", line 65, in assert_log
    self.fail(f"line not found: {expected!r}")
    ~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: line not found: 'test.test_android.TestAndroidOutput.test_str 1733459890.9581628'

[bdraco]

Thanks for the quick turnaround on getting this merged. Looking forward to being able to use zero copy writes in production soon.

[JacobCoffee]

@kumaraditya303 I don't think this is possible, in a scoped way specific to asyncio, for potential future advisories. You will need to be added ad hoc

[freakboy3742]

The Android buildbot failure appears to be unrelated - it's looks like a timing-related failure, which passed on the next CI pass.