|
|
|
Created:
11 years, 10 months ago by Nikolay Kim Modified:
11 years, 6 months ago Visibility:
Public. |
Future.set_result is not safe
Patch Set 1 #Patch Set 2 : Use helper function #
Total comments: 5
Total messages: 10
|
Nikolay Kim
Future.set_result is not safe to use with loop.call_soon() here is test for that issue: @mock.patch('asyncio.base_events.logger') ...
|
11 years, 10 months ago (2014年03月03日 22:03:25 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Future.set_result is not safe to use with loop.call_soon()
here is test for that issue:
@mock.patch('asyncio.base_events.logger')
def test_ctor_with_cancelled_waiter(self,m_log):
fut = asyncio.Future(loop=self.loop)
@asyncio.coroutine
def foo():
_SelectorSocketTransport(
self.loop, self.sock, self.protocol, fut)
yield from fut
task = asyncio.async(foo(), loop=self.loop)
test_utils.run_once(self.loop)
task.cancel()
test_utils.run_briefly(self.loop)
self.assertTrue(fut.cancelled())
# exception should not be raised
self.assertFalse(m_log.error.called)
On 2014年03月03日 22:03:25, Nikolay Kim wrote:
> Future.set_result is not safe to use with loop.call_soon()
>
> here is test for that issue:
>
> @mock.patch('asyncio.base_events.logger')
> def test_ctor_with_cancelled_waiter(self,m_log):
> fut = asyncio.Future(loop=self.loop)
>
> @asyncio.coroutine
> def foo():
> _SelectorSocketTransport(
> self.loop, self.sock, self.protocol, fut)
> yield from fut
>
> task = asyncio.async(foo(), loop=self.loop)
> test_utils.run_once(self.loop)
> task.cancel()
> test_utils.run_briefly(self.loop)
> self.assertTrue(fut.cancelled())
>
> # exception should not be raised
> self.assertFalse(m_log.error.called)
i see this problem in production
Hmm... Are you just concerned about the log messages or is the logic of your app compromised? Adding a new method to Future is a pretty drastic step (especially since we are now past CPython 3.4 rc2).
I should add the behavior of set_result is intentional. Are you seeing these spurious log message for all the places that you are patching? Maybe we should have a discussion on the tracker first, or on python-tulip?
On 2014年03月03日 22:33:01, GvR wrote:
> Hmm... Are you just concerned about the log messages or is the logic of your
app
> compromised? Adding a new method to Future is a pretty drastic step
(especially
> since we are now past CPython 3.4 rc2).
it doesnt compromised app logic, just annoying. and problem with Cancelled
exception, it is very hard to understand where it is from.
i do not insists on immediate fix, but it should be fixed eventually.
Feb 28 00:28:03 ip-10-31-197-58 sqs-files ERROR [asyncio] Exception in callback
<bound method Future.set_result of Future<CANCELLED>>(None,)
handle: Handle(<bound method Future.set_result of Future<CANCELLED>>, (None,))
Traceback (most recent call last):
File
"/usr/local/lib/python3.3/dist-packages/asyncio-0.4.1_p1-py3.3.egg/asyncio/events.py",
line 39, in _run
self._callback(*self._args)
File
"/usr/local/lib/python3.3/dist-packages/asyncio-0.4.1_p1-py3.3.egg/asyncio/futures.py",
line 298, in set_result
raise InvalidStateError('{}: {!r}'.format(self._state, self))
asyncio.futures.InvalidStateError: CANCELLED: Future<CANCELLED>
On 2014年03月03日 22:34:33, GvR wrote: > I should add the behavior of set_result is intentional. Are you seeing these > spurious log message for all the places that you are patching? Maybe we should > have a discussion on the tracker first, or on python-tulip? sure, let's discuss on python-tulip
On 2014年03月03日 22:49:48, Nikolay Kim wrote: > On 2014年03月03日 22:34:33, GvR wrote: > > I should add the behavior of set_result is intentional. Are you seeing these > > spurious log message for all the places that you are patching? Maybe we should > > have a discussion on the tracker first, or on python-tulip? > > sure, let's discuss on python-tulip this patch uses helper function
A caveat on my suggestions: various tests fail if I do that. I haven't looked into why yet. Separately, I don't think you've added tests that reproduce the need for every single call site you're modifying. https://codereview.appspot.com/69870048/diff/20001/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/69870048/diff/20001/asyncio/selector_events.py... asyncio/selector_events.py:434: futures.InvalidStateError, waiter.set_result, None) I'm curious if this one can't just drop the call_soon() step -- just go with if not waiter.cancelled(): waiter.set_result(None) Same below. https://codereview.appspot.com/69870048/diff/20001/asyncio/unix_events.py File asyncio/unix_events.py (right): https://codereview.appspot.com/69870048/diff/20001/asyncio/unix_events.py#new... asyncio/unix_events.py:14: from . import events Why not in alphabetical order? https://codereview.appspot.com/69870048/diff/20001/asyncio/unix_events.py#new... asyncio/unix_events.py:266: self._loop.call_soon( Same comments here and below as for selector_events.py -- I'm not sure why these use call_soon() at all.
The issue fixed by this patch can still reproduced in Tulip and Python 3.4/3.5. I opened in issue to discuss it: http://bugs.python.org/issue21886
https://codereview.appspot.com/69870048/diff/20001/asyncio/events.py File asyncio/events.py (right): https://codereview.appspot.com/69870048/diff/20001/asyncio/events.py#newcode20 asyncio/events.py:20: pass Instead of catching InvalidStateError, I would prefer to check for cancelled state: def _maybe_set_result(fut, result): if fut.cancelled(): return fut.set_result(result) https://codereview.appspot.com/69870048/diff/20001/tests/test_selector_events.py File tests/test_selector_events.py (right): https://codereview.appspot.com/69870048/diff/20001/tests/test_selector_events... tests/test_selector_events.py:704: fut = asyncio.Future(loop=self.loop) I don't think that test_selector_events.py is the right place to test the patch. The test is not reliable, it depends on the exact execution order of each function. IMO a test for the private helper function in test_futures.py should be enough. Or no test at all? :-p