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

asyncio: ConnectionResetError after StreamWriter is garbage collected #109321

Closed
rigens opened this issue Sep 12, 2023 · 8 comments
Closed

asyncio: ConnectionResetError after StreamWriter is garbage collected #109321

rigens opened this issue Sep 12, 2023 · 8 comments
Labels
pending The issue will be closed if no feedback is provided topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@rigens
Copy link

rigens commented Sep 12, 2023

Bug report

Bug description:

The code below works perfectly on Python 3.11.4 but fails with ConnectionResetError on Python 3.11.5

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  # type: ignore


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

    transport = await connect()
    # transport is already closed here on 3.11.5

    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)

    writer.close()


if __name__ == '__main__':
    asyncio.run(fetch())
Traceback (most recent call last):
  File "/home/rigens/aio_test1.py", line 50, in <module>
    asyncio.run(fetch())
  File "/usr/local/miniconda/envs/py311_5/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/usr/local/miniconda/envs/py311_5/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/miniconda/envs/py311_5/lib/python3.11/asyncio/base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/rigens/aio_test1.py", line 41, in fetch
    await writer.drain()
  File "/usr/local/miniconda/envs/py311_5/lib/python3.11/asyncio/streams.py", line 378, in drain
    await self._protocol._drain_helper()
  File "/usr/local/miniconda/envs/py311_5/lib/python3.11/asyncio/streams.py", line 167, in _drain_helper
    raise ConnectionResetError('Connection lost')
ConnectionResetError: Connection lost

CPython versions tested on:

3.11

Operating systems tested on:

Linux

@rigens rigens added the type-bug An unexpected behavior, bug, or error label Sep 12, 2023
@kumaraditya303
Copy link
Contributor

It is not recommended to manually instantiate StreamWriter 1, StreamReader2 objects and transports, so this is not a bug of asyncio.

Footnotes

  1. https://docs.python.org/3/library/asyncio-stream.html#asyncio.StreamWriter

  2. https://docs.python.org/3/library/asyncio-stream.html#asyncio.StreamReader

@github-project-automation github-project-automation bot moved this to Todo in asyncio Sep 12, 2023
@kumaraditya303 kumaraditya303 added the pending The issue will be closed if no feedback is provided label Sep 12, 2023
@kumaraditya303
Copy link
Contributor

If you want to just use the transport then you should loop.create_connection API and not streaming APIs.

@gvanrossum gvanrossum closed this as not planned Won't fix, can't repro, duplicate, stale Sep 12, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in asyncio Sep 12, 2023
@rigens
Copy link
Author

rigens commented Sep 13, 2023

It is not recommended to manually instantiate StreamWriter 1, StreamReader...

It's not about manually instantiating the StreamWriter....
The code above is given only as an example

If you want to just use the transport then you should loop.create_connection API and not streaming APIs.

I want to reuse the transport received from some third-party component that uses the streams api (StreamReader, StreamWriter) to communicate with the remote host.

This approach worked fine until version 3.11.5 and this PR #107656

What was the purpose of closing the transport in the StreamWriter __del__ method, why not do it in the __del__ method of the transport itself?

This PR has already broken some libraries: romis2012/aiohttp-socks#27 (comment)

The mentioned PR at least breaks backward compatibility

@rigens
Copy link
Author

rigens commented Sep 13, 2023

@kumaraditya303, if you don't have any arguments, please reopen the issue.

SomberNight added a commit to spesmilo/electrum that referenced this issue Nov 30, 2023
This should fix an issue when running with python 3.11 (possibly only 3.11.5<= ).

```
 47.45 | I | exchange_rate.CoinGecko | getting fx quotes for EUR
 48.18 | E | exchange_rate.CoinGecko | failed fx quotes: ClientOSError('Cannot write to closing transport')
Traceback (most recent call last):
  File "...\electrum\env11\Lib\site-packages\aiohttp\client.py", line 599, in _request
    resp = await req.send(conn)
           ^^^^^^^^^^^^^^^^^^^^
  File "...\electrum\env11\Lib\site-packages\aiohttp\client_reqrep.py", line 712, in send
    await writer.write_headers(status_line, self.headers)
  File "...\electrum\env11\Lib\site-packages\aiohttp\http_writer.py", line 130, in write_headers
    self._write(buf)
  File "...\electrum\env11\Lib\site-packages\aiohttp\http_writer.py", line 75, in _write
    raise ConnectionResetError("Cannot write to closing transport")
ConnectionResetError: Cannot write to closing transport

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "...\electrum\electrum\exchange_rate.py", line 85, in update_safe
    self._quotes = await self.get_rates(ccy)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\electrum\electrum\exchange_rate.py", line 345, in get_rates
    json = await self.get_json('api.coingecko.com', '/api/v3/exchange_rates')
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\electrum\electrum\exchange_rate.py", line 69, in get_json
    async with session.get(url) as response:
  File "...\electrum\env11\Lib\site-packages\aiohttp\client.py", line 1187, in __aenter__
    self._resp = await self._coro
                 ^^^^^^^^^^^^^^^^
  File "...\electrum\env11\Lib\site-packages\aiohttp\client.py", line 613, in _request
    raise ClientOSError(*exc.args) from exc
aiohttp.client_exceptions.ClientOSError: Cannot write to closing transport
```

related:
romis2012/aiohttp-socks#27
python/cpython#109321
@puddly
Copy link

puddly commented Jan 3, 2024

The current asyncio API provides no way to attach StreamReader and StreamWriter to a transport other than UNIX sockets and TCP. I'm trying use a line-based API with a SerialTransport (wrapping a serial port).

#107656 unexpectedly closing the transport on __del__ breaks instances where set_protocol was used:

# Transport is already open
transport = ...

loop = asyncio.get_running_loop()
reader = asyncio.StreamReader(limit=65536, loop=loop)
protocol = asyncio.StreamReaderProtocol(reader, loop=loop)
writer = asyncio.StreamWriter(transport, protocol, reader, loop)

transport.set_protocol(protocol)

# Do line-based operations using `reader` and `writer`
...

# Change protocols
transport.set_protocol(new_protocol)

There is no way to detach the transport without digging into _transport and providing it a mock transport to "close":

class MockTransport:
    def close(self):
        pass

writer._transport = MockTransport()
del writer

It would be nice to have an API to clear writer._transport without closing.

@gvanrossum
Copy link
Member

gvanrossum commented Jan 3, 2024

@puddly

It would be nice to have an API to clear writer._transport without closing.

If that's your main point, could you open a new issue please? While I'm not sure that this issue should remain closed, I don't think it's the place to discuss new feature requests.

@gvanrossum
Copy link
Member

@rigens

This approach worked fine until version 3.11.5 and this PR #107656

The __del__ method got some improvements in #109538.

Is it still preventing you to do what you want to?

@gvanrossum
Copy link
Member

@rigens

What was the purpose of closing the transport in the StreamWriter __del__ method, why not do it in the __del__ method of the transport itself?

AFAICT the reason is because the transport is not under our control -- there are many possible transports and we don't want to add a __del__ method to all of them.

FWIW: The asyncio streams implementation is not very flexible. There are a variety of design aspects that we wish we had done differently. But it's about a decade too late. We've contemplated deprecating it, but didn't have the energy to design and implement something better to replace it. But it looks like you are trying to do something unusual with it, and it's just hard to support that. The changes that broke your code, after all, were put in place to fix someone else's code. Alas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending The issue will be closed if no feedback is provided topic-asyncio type-bug An unexpected behavior, bug, or error
Projects
Status: Done
Development

No branches or pull requests

4 participants