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