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

_read_ready and _write_ready should respect _conn_lost #71410

Closed
ambv opened this issue Jun 5, 2016 · 3 comments
Closed

_read_ready and _write_ready should respect _conn_lost #71410

ambv opened this issue Jun 5, 2016 · 3 comments
Assignees
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@ambv
Copy link
Contributor

ambv commented Jun 5, 2016

BPO 27223
Nosy @gvanrossum, @vstinner, @asvetlov, @ambv, @1st1
Files
  • issue27223.diff
  • cli.py
  • srv.py
  • 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 = 'https://github.com/ambv'
    closed_at = <Date 2016-06-11.15:43:02.752>
    created_at = <Date 2016-06-05.01:20:32.955>
    labels = ['type-bug', 'expert-asyncio']
    title = '_read_ready and _write_ready should respect _conn_lost'
    updated_at = <Date 2016-06-11.15:43:02.751>
    user = 'https://github.com/ambv'

    bugs.python.org fields:

    activity = <Date 2016-06-11.15:43:02.751>
    actor = 'yselivanov'
    assignee = 'lukasz.langa'
    closed = True
    closed_date = <Date 2016-06-11.15:43:02.752>
    closer = 'yselivanov'
    components = ['asyncio']
    creation = <Date 2016-06-05.01:20:32.955>
    creator = 'lukasz.langa'
    dependencies = []
    files = ['43220', '43221', '43222']
    hgrepos = []
    issue_num = 27223
    keywords = ['patch']
    message_count = 3.0
    messages = ['267360', '268217', '268218']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'vstinner', 'asvetlov', 'lukasz.langa', 'python-dev', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27223'
    versions = ['Python 3.5', 'Python 3.6']

    @ambv
    Copy link
    Contributor Author

    ambv commented Jun 5, 2016

    Currently _read_ready and _write_ready callbacks do not respect _conn_lost, which makes them vulnerable to the following race condition:

    • the client protocol connects to a server
    • the client protocol sends some data
    • the server sends some data back
    • in the mean time, something causes the connection to be lost and that information gets handled by the client protocol first
    • the client transport receives _read_ready but now the connection is already marked as lost and cleared on the transport

    This causes an ugly exception to be raised:

    	Exception in callback _SelectorSocketTransport._read_ready()
    	handle: <Handle _SelectorSocketTransport._read_ready()>
    	Traceback (most recent call last):
    	File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/selector_events.py", line 664, in _read_ready
    		data = self._sock.recv(self.max_size)
    	AttributeError: 'NoneType' object has no attribute 'recv'
    
    	During handling of the above exception, another exception occurred:
    
    	Traceback (most recent call last):
    	File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/events.py", line 125, in _run
    		self._callback(*self._args)
    	File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/selector_events.py", line 668, in _read_ready
    		self._fatal_error(exc, 'Fatal read error on socket transport')
    	File "/Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/asyncio/selector_events.py", line 587, in _fatal_error
    		self._loop.call_exception_handler({
    	AttributeError: 'NoneType' object has no attribute 'call_exception_handler'

    This can be reproduced by running srv.py and cli.py attached here. Since this is an event loop timing issue, it might take a few tries of cli.py to the issue to surface. I get it to repro more consistently with PYTHONASYNCIODEBUG=1.

    In production I get this issue most often when there's a torrent of simultaneous connections being created while existing protocol connections are already being handled.

    To fix this, I propose to add guards for _conn_lost on both _ready_ready and _write_ready. See attached patch. This doesn't break any existing assumptions because we can't really respond to I/O events on a connection that's reset.

    The patch also changes 3 existing unit tests, which I believe are currently invalid. test_write_ready_exception_close simulates filling the buffer and calling _write_ready after the transport has been closed. This means _call_connection_lost has been called and now _sock is None and _loop is None. We'd have AttributeError raised (like in the traceback above) and not OSError like the test believes. That would cause the same except clause to be invoked but then (like in the traceback above) _fatal_error fails due to _loop being None. I renamed the test test_transport_close_remove_writer and removed the _write_ready assumptions in it.

    test_write_ready_send_closing required a change so that the buffer is non-empty BEFORE transport.close() is called. In this case close() doesn't remove the writer and schedule connection_lost. Instead, this is done by _write_ready after the write has been sent out. I also removed the following assert: self.assertFalse(self.loop.writers) because it's true during the entire execution of the test (in other words, checks nothing).

    test_write_ready_send_closing_empty_buffer checks for _call_connection_lost to be called but in fact _call_connection_lost is scheduled to be called_soon in transport.close(). I changed the test to reflect this.

    @ambv ambv self-assigned this Jun 5, 2016
    @ambv ambv changed the title _ready_ready and _write_ready should respect _conn_lost _read_ready and _write_ready should respect _conn_lost Jun 10, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 11, 2016

    New changeset 9512cfd53903 by Yury Selivanov in branch '3.5':
    Issue bpo-27223: aio: Fix _read_ready and _write_ready to respect _conn_lost.
    https://hg.python.org/cpython/rev/9512cfd53903

    New changeset d56b3e5ebfe2 by Yury Selivanov in branch 'default':
    Merge 3.5 (issue bpo-27223)
    https://hg.python.org/cpython/rev/d56b3e5ebfe2

    @1st1
    Copy link
    Member

    1st1 commented Jun 11, 2016

    Thanks, Łukasz!

    @1st1 1st1 closed this as completed Jun 11, 2016
    @1st1 1st1 added the type-bug An unexpected behavior, bug, or error label Jun 11, 2016
    @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
    Labels
    topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants