Skip to content

bpo-25292: asyncio ssl fail hard when writing to closed transport #22541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

carlbordum
Copy link
Contributor

@carlbordum carlbordum commented Oct 4, 2020

Here is my attempt at bpo-25292 🤗 I hope this is the desired behaviour. Should I add a note to asyncio.StreamWriter.write, mentioning that it can fail with BrokenPipeError?

https://bugs.python.org/issue25292

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Hi, this looks pretty good to me, I think you might need a Misc/News entry though since this change affects behavior of the server itself :).

if self._ssl_protocol._sslpipe is None:
# The SSL pipe has been destroyed. Writing should fail.
# See bpo-25292.
raise BrokenPipeError()
Copy link
Member

@Fidget-Spinner Fidget-Spinner Oct 5, 2020

Choose a reason for hiding this comment

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

I think an error message describing the error might be helpful to the user. Maybe something like
raise BrokenPipeError("SSL pipe is broken") ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats good. I added your suggested error message

Before this patch, writing to a closed pipe will silently fail. This
issue is slightly related to bpo-33037/pythonGH-6044.
@tiran tiran removed their request for review April 17, 2021 21:24
@kumaraditya303
Copy link
Contributor

As per bpo, the ssl implementation was rewritten so this is outdated now, try to create a reproducer and create a new issue if the issue still exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants