classification
Title: BaseSelectorEventLoop.sock_{recv,sendall}() don't remove their callbacks when canceled
Type: behavior Stage:
Components: asyncio Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: asvetlov Nosy List: abacabadabacaba, asvetlov, lukasz.langa, ned.deily, yselivanov
Priority: high Keywords: patch

Created on 2017-04-13 08:55 by abacabadabacaba, last changed 2020-02-26 12:39 by asvetlov.

Pull Requests
URL Status Linked Edit
PR 10419 merged asvetlov, 2018-11-08 22:41
Messages (12)
msg291593 - (view) Author: Evgeny Kapun (abacabadabacaba) Date: 2017-04-13 08:55
Code:

    import asyncio as a
    import socket as s

    @a.coroutine
    def coro():
        s1, s2 = s.socketpair()
        s1.setblocking(False)
        s2.setblocking(False)
        try:
            yield from a.wait_for(loop.sock_recv(s2, 1), 1)
        except a.TimeoutError:
            pass
        yield from loop.sock_sendall(s1, b'\x00')
        yield
        s1.close()
        s2.close()

    loop = a.get_event_loop()
    loop.run_until_complete(coro())

Result:

    Exception in callback BaseSelectorEventLoop._sock_recv(<Future cancelled>, True, <socket.socke...2049, proto=0>, 1)
    handle: <Handle BaseSelectorEventLoop._sock_recv(<Future cancelled>, True, <socket.socke...2049, proto=0>, 1)>
    Traceback (most recent call last):
      File "/usr/lib/python3.6/asyncio/events.py", line 127, in _run
        self._callback(*self._args)
      File "/usr/lib/python3.6/asyncio/selector_events.py", line 378, in _sock_recv
        self.remove_reader(fd)
      File "/usr/lib/python3.6/asyncio/selector_events.py", line 342, in remove_reader
        return self._remove_reader(fd)
      File "/usr/lib/python3.6/asyncio/selector_events.py", line 279, in _remove_reader
        key = self._selector.get_key(fd)
      File "/usr/lib/python3.6/selectors.py", line 189, in get_key
        return mapping[fileobj]
      File "/usr/lib/python3.6/selectors.py", line 70, in __getitem__
        fd = self._selector._fileobj_lookup(fileobj)
      File "/usr/lib/python3.6/selectors.py", line 224, in _fileobj_lookup
        return _fileobj_to_fd(fileobj)
      File "/usr/lib/python3.6/selectors.py", line 41, in _fileobj_to_fd
        raise ValueError("Invalid file descriptor: {}".format(fd))
    ValueError: Invalid file descriptor: -1
msg326294 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-09-24 21:57
See also https://bugs.python.org/issue34795
msg329746 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2018-11-12 17:00
New changeset 74387926072abf338a4c1cec1bf0501fc65bbee5 by Andrew Svetlov in branch 'master':
bpo-30064: Refactor sock_* asyncio API (#10419)
https://github.com/python/cpython/commit/74387926072abf338a4c1cec1bf0501fc65bbee5
msg355272 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2019-10-23 22:42
I think this broke asyncio a bit.  These two uvloop sock_* API tests no longer pass on asyncio 3.8:

* https://github.com/MagicStack/uvloop/blob/master/tests/test_sockets.py#L192

* https://github.com/MagicStack/uvloop/blob/master/tests/test_sockets.py#L238

Andrew, since this was your patch, please take a look.
msg355273 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2019-10-23 22:58
I'll elevate the status so that we don't forget before 3.8.1 is too close
msg357989 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-12-07 20:48
> I'll elevate the status so that we don't forget before 3.8.1 is too close

Andrew, Yury: ping. 3.8.1 cutoff is approaching.
msg358011 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-12-08 16:37
Not sure if failed uvloop tests are correct.
The scenario is like the following:
1. Suppose we have an unblocking socket connected to peer.
2. Create a task for reading data: 
  task = asyncio.create_task(loop.sock_read(sock, 1))
 Note, the task is not started yet.
3. Just after it, cancel the task without waiting for the actual cancellation
  task.cancel()
4. Finally, read from the socket again:
  data = await loop.sock_read(sock, 1)

If I put context switch (await asyncio.sleep(0)) between 3) and 4) *or* replace direct sock_read() call in 4) with creation a task (data = await asyncio.create_task(loop.sock_read(sock, 1))) the cancellation of former read is performed and test passes.

I very doubt if any sane code is organizing like this test: start delayed reading, cancel it and read again.

The worse, neither previous not current sock_read() implementation doesn't prevent the concurrent reading which basically delivers data in an unpredictable order. Honestly, I'm not sure if we need to provide the concurrency guarantee for such low-level functions.  The code built on top of these low-level primitives should handle the cuncurrent access problem IMHO.
msg358098 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2019-12-09 14:19
Note: this is going to miss Python 3.8.1 as I'm releasing 3.8.1rc1 right now.  Please address this before 3.8.2 (due in February).
msg358108 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2019-12-09 14:59
As I wrote, I don't think that we should fix the code but the failed uvloop test is slightly weird instead.

Maybe I'm wrong, I need to communicate with Yuri anyway.
msg362637 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2020-02-25 12:10
Downgrading priority on this one.
msg362671 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2020-02-25 23:38
> I very doubt if any sane code is organizing like this test: start delayed reading, cancel it and read again.

Hm, cancellation should work correctly no matter how "sane" or "insane" the user code is.

> The worse, neither previous not current sock_read() implementation doesn't prevent the concurrent reading which basically delivers data in an unpredictable order.

But we're not discussing using a socket concurrently -- asyncio explicitly does not support that for the sock_ api. 

AFAICT this issue is about consequent cancel operation not working as expected in asyncio, no?
msg362690 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2020-02-26 12:39
This is a very interesting question.
In asyncio, task cancellation is not executed on `task.cancel()` call immediately but the task is *marked* for cancellation. 
The actual cancellation requires a context switch (await asyncio.sleep(0) or similar) to propagate `asyncio.CancelledError` into the task code and unwind the exception.

As I said, the test needs a context switch between `cancel()` call and analyzing the state after the task cancellation.

Futures are canceled immediately, that's why the previous implementation passed uvloop's test suite untouched. Unfortunately, it had own flaws.
Also please note, sock_connect()/sock_accept() were implemented this way about two years before my changes, they also suffer from cancel-without-actual-cancellation problems as well.
History
Date User Action Args
2020-02-26 12:39:03asvetlovsetmessages: + msg362690
2020-02-25 23:38:54yselivanovsetmessages: + msg362671
2020-02-25 12:10:38lukasz.langasetpriority: deferred blocker -> high

messages: + msg362637
2019-12-09 14:59:33asvetlovsetmessages: + msg358108
2019-12-09 14:19:33lukasz.langasetnosy: + lukasz.langa
messages: + msg358098
2019-12-08 16:37:21asvetlovsetmessages: + msg358011
2019-12-07 20:48:28ned.deilysetnosy: + ned.deily
messages: + msg357989
2019-10-23 22:58:45yselivanovsetpriority: normal -> deferred blocker

messages: + msg355273
2019-10-23 22:42:58yselivanovsetstatus: closed -> open
resolution: fixed ->
messages: + msg355272

stage: resolved ->
2018-11-12 17:02:04asvetlovsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-11-12 17:00:25asvetlovsetmessages: + msg329746
2018-11-08 22:41:33asvetlovsetkeywords: + patch
stage: patch review
pull_requests: + pull_request9699
2018-10-09 08:57:06asvetlovsetassignee: asvetlov
2018-09-24 21:57:11yselivanovsetnosy: + asvetlov

messages: + msg326294
versions: + Python 3.8, - Python 3.5, Python 3.6
2017-04-13 08:55:09abacabadabacabacreate