classification
Title: Reading received data from a closed TCP stream using `StreamReader.read` might hang forever
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: asvetlov, vxgmichel, yselivanov
Priority: normal Keywords: patch

Created on 2018-10-25 12:32 by vxgmichel, last changed 2018-11-08 12:22 by asvetlov. This issue is now closed.

Files
File name Uploaded Description Edit
stuck_on_py38.py vxgmichel, 2018-10-25 12:32 This test fails on 3.8 but passes on 3.7.
patch-bpo-35065.diff vxgmichel, 2018-10-26 13:29 A possible fix
Pull Requests
URL Status Linked Edit
PR 10212 merged vxgmichel, 2018-10-29 09:21
Messages (5)
msg328430 - (view) Author: Vincent Michel (vxgmichel) * Date: 2018-10-25 12:32
I'm not sure whether it is intended or not, but I noticed a change in the  behavior of `StreamReader` between version 3.7 and 3.8.

Basically, reading some received data from a closed TCP stream using `StreamReader.read` might hang forever, under certain conditions.

I'm not sure what those conditions are but I managed to reproduce the issue consistently with the following workflow:
 - server writes some data
 - client reads a part of the data
 - client closes the writer
 - server closes the writer
 - client tries to read the remaining data

The test attached implements the behavior. It fails on 3.8 but passes on 3.7
msg328442 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-10-25 14:19
Hi Vincent!

No, the hang is not intended behavior. Thanks for the report.
I'll assign the issue to myself to don't miss the bug; but if you have a patch with a fix -- please don't hesitate to make a Pull Request for it.
msg328528 - (view) Author: Vincent Michel (vxgmichel) * Date: 2018-10-26 09:47
Hi Andrew!

I reverted the commit associated with the following PR, and the hanging issue disappeared:
https://github.com/python/cpython/pull/9201

I'll look into it.
msg328547 - (view) Author: Vincent Michel (vxgmichel) * Date: 2018-10-26 13:29
I found the culprit:
https://github.com/python/cpython/blob/a05bef4f5be1bcd0df63ec0eb88b64fdde593a86/Lib/asyncio/streams.py#L350

The call to `_untrack_reader` is performed too soon. Closing the transport causes `protocol.connection_lost()` to be "called soon" but by the time it is actually executed, the stream reader has been "untracked".  Since the protocol doesn't know the stream reader anymore, it has not way to feed it the EOF.

The fix attached removes the `_untrack_reader` call and definition altogether. I don't really see the point of this method since one has to wait for `connection_lost` to be executed before untracking the reader, but `connection_lost` already gets rid of the reader reference.

With this fix, calling `writer.close` then awaiting `writer.wait_closed` (or awaiting `writer.aclose`) should:
- close the transport
- schedule `protocol.connection_lost`
- wait for the protocol to be closed
- run `protocol.connection_lost`
- feed the EOF to the reader
- set the protocol as closed
- get rid of the reader reference in the protocol
- return (making aclose causal and safe)
- the reader can then be safely garbage collected

But maybe I'm missing something about `_untrack_reader`?
msg329467 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-11-08 12:21
New changeset fd512d76456b65c529a5bc58d8cfe73e4a10de7a by Andrew Svetlov (Vincent Michel) in branch 'master':
bpo-35065: Remove `StreamReaderProtocol._untrack_reader` (#10212)
https://github.com/python/cpython/commit/fd512d76456b65c529a5bc58d8cfe73e4a10de7a
History
Date User Action Args
2018-11-08 12:22:13asvetlovsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-11-08 12:21:51asvetlovsetmessages: + msg329467
2018-10-29 09:21:08vxgmichelsetstage: patch review
pull_requests: + pull_request9528
2018-10-26 13:29:50vxgmichelsetfiles: + patch-bpo-35065.diff
keywords: + patch
messages: + msg328547
2018-10-26 09:47:56vxgmichelsettype: behavior
messages: + msg328528
2018-10-25 14:19:55asvetlovsetmessages: + msg328442
2018-10-25 12:32:26vxgmichelcreate