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) * |
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) * |
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) * |
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) * |
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) * |
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 (Mariatta) * |
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 (Mariatta) * |
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 (Mariatta) * |
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 (Mariatta) * |
Date: 2017-04-22 03:00 |
PR has been merged and backported to 3.5 and 3.6.
Thanks everyone :)
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:45 | admin | set | github: 74284 |
2017-04-22 03:00:36 | Mariatta | set | status: open -> closed resolution: fixed messages:
+ msg292098
stage: backport needed -> resolved |
2017-04-22 02:58:30 | Mariatta | set | messages:
+ msg292097 |
2017-04-22 02:58:03 | Mariatta | set | messages:
+ msg292096 |
2017-04-22 02:15:53 | Mariatta | set | pull_requests:
+ pull_request1364 |
2017-04-22 02:15:45 | Mariatta | set | pull_requests:
+ pull_request1363 |
2017-04-21 20:51:02 | Mariatta | set | stage: backport needed versions:
+ Python 3.5, Python 3.6 |
2017-04-21 20:49:51 | Mariatta | set | nosy:
+ Mariatta messages:
+ msg292077
|
2017-04-19 13:28:37 | crenwick | set | messages:
+ msg291885 |
2017-04-19 02:22:03 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg291856
|
2017-04-18 23:50:22 | yselivanov | set | messages:
+ msg291852 |
2017-04-18 23:45:45 | crenwick | set | messages:
+ msg291851 |
2017-04-18 23:35:59 | yselivanov | set | messages:
+ msg291850 |
2017-04-18 23:20:53 | crenwick | set | messages:
+ msg291849 |
2017-04-18 23:07:00 | yselivanov | set | messages:
+ msg291848 |
2017-04-18 22:59:19 | crenwick | set | messages:
+ msg291847 |
2017-04-18 22:51:58 | yselivanov | set | messages:
+ msg291846 |
2017-04-18 22:24:34 | crenwick | create | |