classification
Title: sock_recv not detected a coroutine
Type: behavior Stage: resolved
Components: asyncio, Documentation Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: Jeremy Bustamante, asvetlov, berker.peksag, docs@python, gvanrossum, yselivanov
Priority: normal Keywords: patch

Created on 2017-01-22 14:09 by Jeremy Bustamante, last changed 2018-01-22 18:17 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
issue29344.diff berker.peksag, 2017-02-01 19:49 review
Messages (15)
msg286011 - (view) Author: Jeremy Bustamante (Jeremy Bustamante) Date: 2017-01-22 14:09
Documemtation says sock_recv is a coroutine
https://docs.python.org/3.6/library/asyncio-eventloop.html#low-level-socket-operations

But following code says it isnt:

import asyncio
loop = asyncio.get_event_loop()

asyncio.iscoroutinefunction(loop.sock_recv)
False

asyncio.iscoroutine(loop.sock_recv)
False
msg286575 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-02-01 00:54
I'm not really familiar with asyncio terminology, but it looks like it returns a future:

    >>> asyncio.isfuture(loop.sock_recv(sock, 1024))
    True

There is a also a test in test_selector_events:

    f = self.loop.sock_recv(sock, 1024)
    self.assertIsInstance(f, asyncio.Future)
msg286677 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2017-02-01 19:10
OP is correct. sock_recv(), sock_sendall(), sock_connect(), sock_accept() all are functions that return Futures. The docs are wrong, as are the docstrings in the code.
msg286684 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-02-01 19:49
Here's a patch. sock_connect() wrapped by @coroutine in Lib/asyncio/selector_events.py so I left it as-is. Let me know if it still needs to be updated:

    @coroutine
    def sock_connect(self, sock, address):
        """Connect to a remote socket at address.

        This method is a coroutine.
        """
        ...

        return (yield from fut)
msg286689 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-02-01 20:06
I think the original idea was to document that all methods of the loop are coroutines, so that:

1. if a user needs a Future object they call ensure_future: 

   fut = asyncio.ensure_future(loop.method())

2. it gives us ability to refactor things. For instance, sock_connect was a method that returned Futures, but at one point of 3.5 we changed it to be coroutine.  Because the method was documented as a coroutine, it wasn't strictly a backwards incompatible way.

In general, I think it would be safer for us to simple make all loop methods coroutines. Or, less radical, just keeping the status quo: document everything as a coroutine.
msg286704 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2017-02-01 21:32
The word coroutine has a more specific meaning though (and we have iscoroutine*() inspection functions to check for it).

Maybe we should switch all these to "awaitable"?

Also note that in proactor_events.py, sock_connect() is *not* a coroutine. In fact I'm not sure what it is -- it calls self._proactor.connect() which appears to return None from the code in windows_events.py. That's presumably a separate bug.
msg286707 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-02-01 21:46
> Maybe we should switch all these to "awaitable"?

I like this!  Would it be OK to add a new sphinx declaration?  So that:

   .. coroutinemethod:: AbstractEventLoop.shutdown_asyncgens()

would become:

   .. awaitable:: AbstractEventLoop.shutdown_asyncgens()
   
?

> Also note that in proactor_events.py, sock_connect() is *not* a coroutine. 

Yeah, those small incompatibilities are inevitable for asyncio programs/frameworks -- something that returns a Future may occasionally become a coroutine.  Your idea to document API methods as "awaitables" seems to be the right way to go.

> In fact I'm not sure what it is -- it calls self._proactor.connect() which appears to return None from the code in windows_events.py. That's presumably a separate bug.

It looks like it returns the result of "IocpProactor.connect()" call, which returns an _OverlappedFuture instance (I don't really know Windows part of asyncio code, so I might be missing something).
msg286710 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2017-02-01 21:58
A new Sphinx declaration sounds great to me, but you might want to check
with some Sphinx or Python-docs expert.

I somehow misread the code of IocpProactor.connect(), so ignore that part
-- the point is that it's not always a coroutine.
msg308808 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2017-12-20 20:38
sock_recv was converted into genuine coroutine in Python 3.7

Closing the issue.
msg310430 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-01-22 16:41
Andrew, shouldn't we fix 3.6 documentation? sock_* methods still documented as coroutines.
msg310431 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-01-22 16:49
> Andrew, shouldn't we fix 3.6 documentation? sock_* methods still documented as coroutines.

We only fixed it in 3.7 and the doc was updated.  3.6 stays as is.
msg310434 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-01-22 17:10
> We only fixed it in 3.7 and the doc was updated.  3.6 stays as is.

I'm not talking about fixing code in 3.6. In 3.6, sock_* methods still documented as coroutines but they are normal functions:

* https://docs.python.org/3.6/library/asyncio-eventloop.html#low-level-socket-operations
* https://github.com/python/cpython/blob/4002d5dbf4c058bbf2462f9f5dea057956d1caff/Doc/library/asyncio-eventloop.rst#low-level-socket-operations
* https://github.com/python/cpython/blob/4002d5dbf4c058bbf2462f9f5dea057956d1caff/Lib/asyncio/selector_events.py#L354

3.6 documentation still needs to be fixed. See my patch.
msg310435 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-01-22 17:16
Since we made them coroutines in 3.7, I'd prefer to keep the 3.6 documentation as is (so that people don't write code that expects them to be not coroutines in 3.6).

Please let's keep the current status quo.
msg310442 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-01-22 18:00
Well, the current status quo has already confused a user (see msg286011)

> (so that people don't write code that expects them to be not coroutines in 3.6)

Even if people expect these methods to be coroutines, two functions of the asyncio public API (iscoroutinefunction() and iscoroutine()) disagree with them as described in msg286011.

In similar scenarios, we've always keep the code as is and clarified the docs in stable releases, then changed the code behave as documented in master.
msg310445 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-01-22 18:17
They have been documented as coroutines and were returning futures since Python 3.4. I believe we can keep the status quo especially if only one user complained about it during the last 5 years.
History
Date User Action Args
2018-01-22 18:17:06yselivanovsetmessages: + msg310445
2018-01-22 18:00:30berker.peksagsetmessages: + msg310442
2018-01-22 17:16:32yselivanovsetmessages: + msg310435
2018-01-22 17:10:13berker.peksagsetmessages: + msg310434
2018-01-22 16:49:02yselivanovsetmessages: + msg310431
2018-01-22 16:41:02berker.peksagsetmessages: + msg310430
2017-12-20 20:38:50asvetlovsetstatus: open -> closed

versions: - Python 3.5, Python 3.6
nosy: + asvetlov

messages: + msg308808
resolution: fixed
stage: patch review -> resolved
2017-02-01 21:58:49gvanrossumsetmessages: + msg286710
2017-02-01 21:46:57yselivanovsetmessages: + msg286707
2017-02-01 21:32:34gvanrossumsetmessages: + msg286704
2017-02-01 20:06:00yselivanovsetmessages: + msg286689
2017-02-01 19:49:24berker.peksagsetfiles: + issue29344.diff

assignee: docs@python
components: + Documentation

keywords: + patch
nosy: + docs@python
messages: + msg286684
stage: patch review
2017-02-01 19:10:08gvanrossumsetmessages: + msg286677
versions: + Python 3.5, Python 3.7
2017-02-01 00:54:43berker.peksagsetnosy: + berker.peksag
messages: + msg286575
2017-01-22 14:09:45Jeremy Bustamantecreate