classification
Title: Verbose TypeError for asyncio.ensure_future
Type: enhancement Stage: resolved
Components: asyncio Versions: Python 3.7, Python 3.6, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mariatta, crenwick, r.david.murray, yselivanov
Priority: normal Keywords:

Created on 2017-04-18 22:24 by crenwick, last changed 2017-04-22 03:00 by Mariatta. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1170 merged crenwick, 2017-04-18 22:24
PR 1246 merged Mariatta, 2017-04-22 02:15
PR 1247 merged Mariatta, 2017-04-22 02:15
Messages (14)
msg291845 - (view) Author: crenwick (crenwick) * Date: 2017-04-18 22:24
Despite the shy mention in the docs, it was not clear to me that the future returned from asyncio.run_coroutine_threadsafe is not compatible with asyncio.ensure_future (and other asyncio functions), and it took me a fair amount of frustration and source-code-digging to figure out what was going on.

To avoid this confusion for other users, I think that a verbose TypeError warning when a concurrent.futures.Future object is passed into asyncio.ensure_future would be very helpful.
msg291846 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-04-18 22:51
`asyncio.run_coroutine_threadsafe` was't designed to be compatible with `asyncio.ensure_future`. It should be used to run a coroutine in an event loop from another thread. You shouldn't need to await on the future it returns.
msg291847 - (view) Author: crenwick (crenwick) * Date: 2017-04-18 22:59
Totally. But this wasn't immediately clear to me when using the API. A verbose error like this one would have made this behavior more clear to me.
msg291848 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-04-18 23:06
> A verbose error like this one would have made this behavior more clear to me.

Well, it already raises a TypeError with a clear message -- this is how we tell the user that the argument type is not supported in Python.  I don't think we need to specialize this code any further.

Instead I'd suggest to try to improve asyncio documentation for `ensure_future` and `run_coroutine_threadsafe`.
msg291849 - (view) Author: crenwick (crenwick) * Date: 2017-04-18 23:20
> it already raises a TypeError with a clear message

This is more to my point: I found the TypeError message not clear at all. 

From my prospective, I was using a future object returned from an asyncio function to call another asyncio function, yet that function was telling me it could only use a future with it. 

Though I eventually figured out what was going on, I think that a more clear message at that moment would have been very helpful to me. And I think other users who benefit from this was well.

A better warning/wording for the API would be helpful as well, but I don't think it fully suffices the problem.
msg291850 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-04-18 23:35
> This is more to my point: I found the TypeError message not clear at all. 

> From my prospective, I was using a future object returned from an asyncio function to call another asyncio function, yet that function was telling me it could only use a future with it. 

I understand, but this is the first time I see somebody tries to use `ensure_future` on `run_coroutine_threadsafe` result, which suggests that it's not a common problem.

Specializing error messages makes code more complex and adds to maintenance overhead, so we can't afford to make changes on first request.  In cases like this we usually put the issue on hold to see if more people complain.
msg291851 - (view) Author: crenwick (crenwick) * Date: 2017-04-18 23:45
Totally understandable. Some final thoughts:

> this is the first time I see somebody tries to use `ensure_future` on `run_coroutine_threadsafe` result, which suggests that it's not a common problem.

 - I wonder how much self-reporting will be done here.

 - I also considered making the existing error message more verbose (I think that still could be effective). However, I decided to open this PR as a separate TypeError to not muddle the language of the probably more common TypeError.

Thanks for your time and insight, Yury!
msg291852 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2017-04-18 23:50
> - I wonder how much self-reporting will be done here.

Yeah, I don't think we see enough reporting on bugs.python.org.  I hope people will become more vocal :)  I try to monitor what people say about asyncio on SO, Twitter and /r/python to get a better picture.

> - I also considered making the existing error message more verbose (I think that still could be effective).

Feel free to suggest a better wording.
msg291856 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2017-04-19 02:22
I don't think your specialized error message adds anything.  The the most common mistake, IMO, is going to be not realizing that run_coroutine_threadsafe don't return one of the acceptable types.  So being told that concurrent.future.Future is not acceptable will add zero additional information.  I think the only change that needs to be made is to make the existing message say 'asyncio.Future' instead of just 'Future'.
msg291885 - (view) Author: crenwick (crenwick) * Date: 2017-04-19 13:28
> Feel free to suggest a better wording.

> I think the only change that needs to be made is to make the existing message say 'asyncio.Future' instead of just 'Future'.

I hear ya. New commit in the PR reverts the TypeError I added, and changes the original TypeError language to simply read "An asyncio.Future ..." instead of just "A Future ...".
msg292077 - (view) Author: Mariatta Wijaya (Mariatta) * (Python committer) Date: 2017-04-21 20:49
New changeset ae5b3260dd459845aad8a30491b76d471577785d by Mariatta (Charles Renwick) in branch 'master':
bpo-30098: Clarify that run_coroutine_threadsafe expects asyncio.Future   (GH-1170)
https://github.com/python/cpython/commit/ae5b3260dd459845aad8a30491b76d471577785d
msg292096 - (view) Author: Mariatta Wijaya (Mariatta) * (Python committer) Date: 2017-04-22 02:58
New changeset 503d74a60aa9290cf8d174eab95fdfdea1f2b284 by Mariatta in branch '3.5':
bpo-30098: Clarify that run_coroutine_threadsafe expects asyncio.Future   (GH-1170) (#1246)
https://github.com/python/cpython/commit/503d74a60aa9290cf8d174eab95fdfdea1f2b284
msg292097 - (view) Author: Mariatta Wijaya (Mariatta) * (Python committer) Date: 2017-04-22 02:58
New changeset a3d8dda7d899bf41ab7eb2c6148459ad276fe295 by Mariatta in branch '3.6':
bpo-30098: Clarify that run_coroutine_threadsafe expects asyncio.Future   (GH-1170) (#1247)
https://github.com/python/cpython/commit/a3d8dda7d899bf41ab7eb2c6148459ad276fe295
msg292098 - (view) Author: Mariatta Wijaya (Mariatta) * (Python committer) Date: 2017-04-22 03:00
PR has been merged and backported to 3.5 and 3.6. 
Thanks everyone :)
History
Date User Action Args
2017-04-22 03:00:36Mariattasetstatus: open -> closed
resolution: fixed
messages: + msg292098

stage: backport needed -> resolved
2017-04-22 02:58:30Mariattasetmessages: + msg292097
2017-04-22 02:58:03Mariattasetmessages: + msg292096
2017-04-22 02:15:53Mariattasetpull_requests: + pull_request1364
2017-04-22 02:15:45Mariattasetpull_requests: + pull_request1363
2017-04-21 20:51:02Mariattasetstage: backport needed
versions: + Python 3.5, Python 3.6
2017-04-21 20:49:51Mariattasetnosy: + Mariatta
messages: + msg292077
2017-04-19 13:28:37crenwicksetmessages: + msg291885
2017-04-19 02:22:03r.david.murraysetnosy: + r.david.murray
messages: + msg291856
2017-04-18 23:50:22yselivanovsetmessages: + msg291852
2017-04-18 23:45:45crenwicksetmessages: + msg291851
2017-04-18 23:35:59yselivanovsetmessages: + msg291850
2017-04-18 23:20:53crenwicksetmessages: + msg291849
2017-04-18 23:07:00yselivanovsetmessages: + msg291848
2017-04-18 22:59:19crenwicksetmessages: + msg291847
2017-04-18 22:51:58yselivanovsetmessages: + msg291846
2017-04-18 22:24:34crenwickcreate