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

Jump to bottom

Reading received data from a closed TCP stream using StreamReader.read might hang forever #79246

Closed
vxgmichel mannequin opened this issue Oct 25, 2018 · 5 comments
Closed

Reading received data from a closed TCP stream using StreamReader.read might hang forever #79246

vxgmichel mannequin opened this issue Oct 25, 2018 · 5 comments
Labels
3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@vxgmichel
Copy link
Mannequin

vxgmichel mannequin commented Oct 25, 2018

BPO 35065
Nosy @asvetlov, @1st1, @vxgmichel
PRs
  • bpo-35065: Remove StreamReaderProtocol._untrack_reader #10212
    		•
    Files
  • stuck_on_py38.py: This test fails on 3.8 but passes on 3.7.
  • patch-bpo-35065.diff: A possible fix
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2018-11-08.12:22:13.649>
created_at = <Date 2018-10-25.12:32:25.991>
labels = ['type-bug', '3.8', 'expert-asyncio']
title = 'Reading received data from a closed TCP stream using `StreamReader.read` might hang forever'
updated_at = <Date 2018-11-08.12:22:13.648>
user = 'https://github.com/vxgmichel'

    bugs.python.org fields:

    activity = <Date 2018-11-08.12:22:13.648>
actor = 'asvetlov'
assignee = 'none'
closed = True
closed_date = <Date 2018-11-08.12:22:13.649>
closer = 'asvetlov'
components = ['asyncio']
creation = <Date 2018-10-25.12:32:25.991>
creator = 'vxgmichel'
dependencies = []
files = ['47891', '47893']
hgrepos = []
issue_num = 35065
keywords = ['patch']
message_count = 5.0
messages = ['328430', '328442', '328528', '328547', '329467']
nosy_count = 3.0
nosy_names = ['asvetlov', 'yselivanov', 'vxgmichel']
pr_nums = ['10212']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue35065'
versions = ['Python 3.8']
    @vxgmichel
    Copy link
    Mannequin Author

    vxgmichel mannequin commented Oct 25, 2018

    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

    @vxgmichel vxgmichel mannequin added 3.8 only security fixes topic-asyncio labels Oct 25, 2018
    @asvetlov
    Copy link
    Contributor

    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.

    @vxgmichel
    Copy link
    Mannequin Author

    vxgmichel mannequin commented Oct 26, 2018

    Hi Andrew!

    I reverted the commit associated with the following PR, and the hanging issue disappeared:
    #9201

    I'll look into it.

    @vxgmichel vxgmichel mannequin added the type-bug An unexpected behavior, bug, or error label Oct 26, 2018
    @vxgmichel
    Copy link
    Mannequin Author

    vxgmichel mannequin commented Oct 26, 2018

    I found the culprit:

    cpython/Lib/asyncio/streams.py

    Line 350 in a05bef4

    self._protocol._untrack_reader()

    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?

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Nov 8, 2018

    New changeset fd512d7 by Andrew Svetlov (Vincent Michel) in branch 'master':
    bpo-35065: Remove StreamReaderProtocol._untrack_reader (bpo-10212)
    fd512d7

    @asvetlov asvetlov closed this as completed Nov 8, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.8 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @asvetlov