Future.set_result is not safe

Future.set_result is not safe

[Nikolay Kim, 2014-03-03 22:03:25]

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)

[Nikolay Kim, 2014-03-03 22:04:52]

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

[GvR, 2014-03-03 22:33:01]

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).

[GvR, 2014-03-03 22:34:33]

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?

[Nikolay Kim, 2014-03-03 22:46:21]

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>

[Nikolay Kim, 2014-03-03 22:49:48]

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

[Nikolay Kim, 2014-03-04 17:40:26]

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

[GvR, 2014-03-05 19:59:13]

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.

[haypo_gmail, 2014-06-30 14:10:24]

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

[haypo_gmail, 2014-06-30 14:15:34]

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