Skip to content

Conversation

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Aug 5, 2023

(cherry picked from commit 41178e4)

Co-authored-by: Kumar Aditya kumaraditya@python.org

…is not closed (pythonGH-107650)

(cherry picked from commit 41178e4)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@kumaraditya303 kumaraditya303 requested a review from Yhg1s August 5, 2023 12:21
@Yhg1s Yhg1s enabled auto-merge (squash) August 5, 2023 12:41
@Yhg1s Yhg1s disabled auto-merge August 5, 2023 12:42
@Yhg1s
Copy link
Member

Yhg1s commented Aug 5, 2023

I think this should go into rc2, but without the warning. I think the warning is too potentially disruptive this late in the release cycle.

@kumaraditya303 kumaraditya303 changed the title [3.12] GH-106684: raise ResourceWarning when asyncio.StreamWriter is not closed (GH-107650) [3.12] GH-106684: Close asyncio.StreamWriter when asyncio.StreamWriter is not closed by application (GH-107650) Aug 10, 2023
@kumaraditya303 kumaraditya303 added the needs backport to 3.11 only security fixes label Aug 10, 2023
@Yhg1s Yhg1s merged commit 7853c76 into python:3.12 Aug 10, 2023
@miss-islington
Copy link
Contributor Author

Thanks @miss-islington for the PR, and @Yhg1s for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington miss-islington deleted the backport-41178e4-3.12 branch August 10, 2023 09:24
@bedevere-bot
Copy link

GH-107836 is a backport of this pull request to the 3.11 branch.

miss-islington added a commit to miss-islington/cpython that referenced this pull request Aug 10, 2023
…reamWriter` is not closed by application (pythonGH-107650) (pythonGH-107656)

pythonGH-106684: raise `ResourceWarning` when `asyncio.StreamWriter` is not closed (pythonGH-107650)
(cherry picked from commit 41178e4)

(cherry picked from commit 7853c76)

Co-authored-by: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Aug 10, 2023
kumaraditya303 added a commit that referenced this pull request Aug 10, 2023
…treamWriter` is not closed by application (GH-107650) (GH-107656) (#107836)

[3.12] GH-106684:  Close `asyncio.StreamWriter` when `asyncio.StreamWriter` is not closed by application (GH-107650) (GH-107656)

GH-106684: raise `ResourceWarning` when `asyncio.StreamWriter` is not closed (GH-107650)
(cherry picked from commit 41178e4)

(cherry picked from commit 7853c76)

Co-authored-by: Kumar Aditya <kumaraditya@python.org>
@rigens
Copy link

rigens commented Sep 12, 2023

It seems that closing the connection in the StreamWriter's __del__ method is not a good idea. Consider the following use case

import asyncio

HOST = 'ifconfig.me'
PORT = 80


async def connect() -> asyncio.Transport:
    reader, writer = await asyncio.open_connection(
        host=HOST,
        port=PORT,
    )
    return writer.transport


async def fetch():
    loop = asyncio.get_running_loop()

    transport = await connect()
    # !!! transport is already closed here after this PR merged

    reader = asyncio.StreamReader(limit=2**16, loop=loop)
    protocol = asyncio.StreamReaderProtocol(reader, loop=loop)

    transport.set_protocol(protocol)
    loop.call_soon(protocol.connection_made, transport)
    loop.call_soon(transport.resume_reading)

    writer = asyncio.StreamWriter(
        transport=transport,
        protocol=protocol,
        reader=reader,
        loop=loop,
    )

    request = f'GET /ip HTTP/1.1\r\nHost: {HOST}\r\nConnection: close\r\n\r\n'.encode()
    writer.write(request)
    await writer.drain()

    response = await reader.read(-1)
    print(response)


if __name__ == '__main__':
    asyncio.run(fetch())

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

Labels

type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants