Skip to content

bpo-36801: Fix waiting in StreamWriter.drain for closing SSL transport - #13098

Merged
miss-islington merged 2 commits into
python:masterfrom
asvetlov:streams-wait-ssl-shutdown
May 7, 2019
Merged

bpo-36801: Fix waiting in StreamWriter.drain for closing SSL transport#13098
miss-islington merged 2 commits into
python:masterfrom
asvetlov:streams-wait-ssl-shutdown

Conversation

@asvetlov

@asvetlovasvetlov commented May 5, 2019

Copy link
Copy Markdown
Contributor

@asvetlov

Copy link
Copy Markdown
ContributorAuthor

The backport to 3.7 is desired.
The problem not in SSL connections only.
Working on the patch I've found that await subproc.stdin.wait_closed() is broken on 3.7
Probably nobody executes this call though, nobody blames.

# ConnectionResetError otherwise
fut = self._protocol._get_close_waiter(self)
await fut
raise ConnectionResetError('Connection lost')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we want to raise an error from drain()? Maybe it should just return? What's the point of this error?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That's how protocol._drain_helper() works now.
The future is set to exception only if the connection is closed with a failure.
But await writer.drain() raises ConnectionResetError.
I believe this is good: await writer.write(b'data') should fail loudly if the socket is closed.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe the code could be simplified by setting ConnectionResetError by connection_lost() callback handler but I'd like to keep the PR small.
Future improvement worth another pull request.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, got it.

1st1
1st1 approved these changes May 7, 2019
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @asvetlov for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 7, 2019
pythonGH-13098)
https://bugs.python.org/issue36801
(cherry picked from commit 1cc0ee7)
Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
@bedevere-bot

Copy link
Copy Markdown

GH-13176 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request May 7, 2019
GH-13098)
https://bugs.python.org/issue36801
(cherry picked from commit 1cc0ee7)
Co-authored-by: Andrew Svetlov <andrew.svetlov@gmail.com>
vstinner added a commit that referenced this pull request May 14, 2019
@asvetlov
asvetlov deleted the streams-wait-ssl-shutdown branch September 12, 2019 12:43
@nullstd

Copy link
Copy Markdown

Hi @asvetlov , I ran into a BrokenPipeError error when testing the following code:

#!/usr/bin/env python
import sys
import os
import asyncio
async def main_task():
print(sys.version)
xxx = "x" * 196428
incoming_original_data = xxx.encode()
print(f"stdin len: {len(incoming_original_data)}")
proc = await asyncio.create_subprocess_shell(
f"sleep 2 && xxd -l 1",
stdin=asyncio.subprocess.PIPE,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE)
await proc.communicate(input = incoming_original_data)
print("subprocess ended")
def main():
opts = asyncio.run(main_task())
print(f"main END")
return opts
if __name__ == '__main__':
main()

Note the subprocess ends very soon, and it seems _stdin_closed isn't properly handled because I see the following output:

3.11.3 (main, Apr 7 2023, 19:25:52) [Clang 14.0.0 (clang-1400.0.29.202)]
stdin len: 196428
subprocess ended
main END
Future exception was never retrieved
future: <Future finished exception=BrokenPipeError(32, 'Broken pipe')>
Traceback (most recent call last):
File "/usr/local/Cellar/python@3.11/3.11.3/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/subprocess.py", line 152, in _feed_stdin
await self.stdin.drain()
File "/usr/local/Cellar/python@3.11/3.11.3/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/streams.py", line 378, in drain
await self._protocol._drain_helper()
File "/usr/local/Cellar/python@3.11/3.11.3/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/streams.py", line 173, in _drain_helper
await waiter
File "/usr/local/Cellar/python@3.11/3.11.3/Frameworks/Python.framework/Versions/3.11/lib/python3.11/asyncio/unix_events.py", line 709, in _write_ready
n = os.write(self._fileno, self._buffer)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
BrokenPipeError: [Errno 32] Broken pipe

@nullstd

Copy link
Copy Markdown

@nullstd

Copy link
Copy Markdown

cc @1st1

@gpshead

gpshead commented Jun 9, 2023

Copy link
Copy Markdown
Member

please do not post questions and cc people on long closed/merged PRs. Please use the discuss.python.org Help category - and only when it seems an actual CPython issue has been found, file a new issue in the issue tracker.

@pythonpython locked as resolved and limited conversation to collaborators Jun 9, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@asvetlov@miss-islington@bedevere-bot@nullstd@gpshead@1st1@the-knights-who-say-ni