Skip to content
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

Fix SSLProtocol.connection_lost not being called when underlying socket is closed #639

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cjavad
Copy link

@cjavad cjavad commented Oct 23, 2024

As as a follow up to python/cpython#118950, the fix to the issue in CPython's asyncio module seemed to translate quite well onto uvloop, and my failing tests effectively work both using the underlying socket streams and using higher level libraries such as aiohttp where the issue originally appeared.

As a quick description the issue occurs when an underlying socket is broken (typically accompanied by a BrokenPipe error), in uvloop this translates to the system.write call returning some error code which is handled in the exact same way by calling fatal_error as the sister implementation in asyncio. This implementation has a similar pitfall when the writer loop actively is ran in some kind of loop where the call_soon connection_lost function that is supposed to mark the transport as closed (leading to an error being thrown) never is called, leading to some pretty annoying invalid state. But these details are mostly tested using asyncio but the same symptoms and underlying reasons seems to effect this implemention after some light testing, but there might be more to the story here.

These changes are effectively a port of python/cpython#118960 with absolutely no difference except the more thorough tests reflecting the existing tests provided.

I have initially just skipped all asyncio tests as the upstream PR has yet to be merged but most likely we will see it in a future 3.12 release and newer.

Feel free to address any issues or things i've left out from here.

…ts to ensure asyncio.streams code effectively can schedule connection_lost and raise ConnectionResetError
@cjavad
Copy link
Author

cjavad commented Oct 23, 2024

(It would appear the flake8 version CI/CD is using is old enough to have issues haha, locally everything is good and the error is nonsensical, do advise it and on Test_UV_FS_EVENT_RENAME failing though)

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

Successfully merging this pull request may close these issues.

1 participant