Future.set_result is not safe for loop.call_soon()

[Nikolay Kim]

I see "InvalidStateError: CANCELLED: Future<CANCELLED>” exception in my production system.
It doesn’t really affect application logic, just annoying. also it is very hard to understand where exception from.
so i propose to add safe_set_result() method to Future that will check status before setting result.
code review is here https://codereview.appspot.com/69870048/

here is test case:

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

[Victor Stinner]

Hi,

2014-03-03 23:59 GMT+01:00 Nikolay Kim <fafh...@gmail.com>:
> I see "InvalidStateError: CANCELLED: Future<CANCELLED>” exception in my production system.
> It doesn’t really affect application logic, just annoying. also it is very hard to understand where exception from.
> so i propose to add safe_set_result() method to Future that will check status before setting result.
> code review is here https://codereview.appspot.com/69870048/

I didn't understood with your example what happens. In fact, the
future is cancelled before future.set_result() is called. Something
like:
---
fut = Future()
<do something else>
fut.cancel()
<do something else>
fut.set_result()
---
where "<do something else>" is a side-effect of asynchronous
programming and "yield from".

Checking the future status before scheduling the call (with
loop.call_soon) to set_result() would not fix the issue.

If I understood correctly, _SelectorSocketTransport constructor
doesn't call directly set_result() (but use loop.call_soon instead) to
wait until protocol.connection_made() has been called. So
loop.create_connection() only returns the socket when
protocol.connection_made() has been called. Is this correct?

_SelectorSocketTransport constructor cannot call directly
protocol.connection_made()?

Victor

[Nikolay Kim]

On Mar 3, 2014, at 5:07 PM, Victor Stinner <victor....@gmail.com> wrote:

> Hi,
>
> 2014-03-03 23:59 GMT+01:00 Nikolay Kim <fafh...@gmail.com>:
>> I see "InvalidStateError: CANCELLED: Future<CANCELLED>” exception in my production system.
>> It doesn’t really affect application logic, just annoying. also it is very hard to understand where exception from.
>> so i propose to add safe_set_result() method to Future that will check status before setting result.
>> code review is here https://codereview.appspot.com/69870048/
>
> I didn't understood with your example what happens. In fact, the
> future is cancelled before future.set_result() is called. Something
> like:
> ---
> fut = Future()
> <do something else>
> fut.cancel()
> <do something else>
> fut.set_result()
> ---
> where "<do something else>" is a side-effect of asynchronous
> programming and "yield from”.

yes, thats right. but fut.set_result is called by event loop. i see this kind of exceptions:
Feb 28 00:28:03 ip-10-31-197-58 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>

>
> Checking the future status before scheduling the call (with
> loop.call_soon) to set_result() would not fix the issue.

it doesn’t check status of fut before scheduling call with call_soon, it checks status of fut during set_result call.

>
> If I understood correctly, _SelectorSocketTransport constructor
> doesn't call directly set_result() (but use loop.call_soon instead) to
> wait until protocol.connection_made() has been called. So
> loop.create_connection() only returns the socket when
> protocol.connection_made() has been called. Is this correct?

> _SelectorSocketTransport constructor cannot call directly
> protocol.connection_made()?

waiter future is used by clients and that’s different topic.
this problem is not directly related to _SelectorSocketTransport, but how it uses Future.set_result.
i can write similar test for asyncio.sleep as well.

[Guido van Rossum]

It sounds like Nikolay's general problem is that various parts of asyncio are a bit cavalier about returning Futures and scheduling set_result() on them without first checking if the Future is cancelled.

But I don't like the solution of adding a new public method to Future -- I worry that it will be used to mask bugs. We could have an internal helper API used for this purpose and passed to call_soon, e.g.

def _set_result_unless_cancelled(fut, result=None):

    if not fut.cancelled():

        fut.set_result(result)

and then use loop.call_soon(_set_result_unless_cancelled, fut, result) in places that currently call loop.call_soon(fut.set_result, result).

Another possibility might be to have an internal helper that suppresses a specific exception, e.g.

def _call_ignoring_exception(exc, func, *args):

    try:

        func(*args)

    except exc:

        pass

and then use loop.call_soon(_call_ignoring_exception, fut.set_result, None).

--
--Guido van Rossum (python.org/~guido)

[Nikolay Kim]

sure, safe_set_result should not be public method. i like idea with helper function

i just updated code review with helper function.