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 handling of TCP EOF #87

Closed
wants to merge 2 commits into from
Closed

Fix handling of TCP EOF #87

wants to merge 2 commits into from

Conversation

tohin
Copy link

@tohin tohin commented Oct 23, 2022

This patch fixes handling of StreamReader.read for the case when remote client terminates the connection. As documented here: https://docs.python.org/3.4/library/asyncio-stream.html#asyncio.StreamReader.read read method retuns b"" in this case.

This patch fixes handling of StreamReader.read for the case
when remote client terminates the connection. As documented
here: https://docs.python.org/3.4/library/asyncio-stream.html#asyncio.StreamReader.read
read method retuns b"" in this case.
@pgjones
Copy link
Owner

pgjones commented Nov 5, 2022

Could you help me understand how this change relates to https://pgjones.dev/blog/half-closed-sockets-2021/ ?

@tohin
Copy link
Author

tohin commented Nov 6, 2022

Hello Phil,

I've tested the code for TCP FIN case and it is clear that current code
skips an event of EOF that is documented here https://docs.python.org/3/library/asyncio-stream.html#asyncio.StreamReader.read

For a half closed case, I think it would not be detected by StreamReader.read()
and EOF would not be returned, so nothing is changed after this PR.

@richardsheridan
Copy link
Contributor

richardsheridan commented Feb 28, 2023

Hi @tohin,

This bug also affects the trio backend. For me, it lead to a leak of sockets in CLOSE_WAIT state and, after enough users close their browsers, a crash from too many open file handles.
on trio the following lines work:

    async def _read_data(self) -> None:
        while True:
            try:
                with trio.fail_after(self.config.read_timeout or inf):
                    data = await self.stream.receive_some(MAX_RECV)
            except (trio.ClosedResourceError, trio.BrokenResourceError):
                break
            else:
                await self.protocol.handle(RawData(data))
                if data == b"":
                    break
        await self.protocol.handle(Closed())

But I am concerned that the final line should possibly be in a finally: block.
I will make a pull request against your fork branch to add this additional code.

FWIW I found #103 trying to debug this problem.

@tohin
Copy link
Author

tohin commented Mar 1, 2023

Hi @richardsheridan,

I've rebased the PR.

@tohin
Copy link
Author

tohin commented Mar 2, 2023

@pgjones By the way, it is the same issue that is already partially fixed for Trio 8b4ecd4

@tohin tohin closed this Jul 2, 2023
@pgjones
Copy link
Owner

pgjones commented Jul 8, 2023

Thanks, fixed in 8a3e512

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.

None yet

3 participants