dabeaz / curio

Good Curio!
Other
4.01k stars 240 forks source link

UniversalEvent.set() is not safe to call more than once. #350

Closed stharding closed 1 year ago

stharding commented 2 years ago

Calling .set() on a UniversalEvent raises an InvalidStateError due to the underlying Future checking if its state is one of: {CANCELLED, CANCELLED_AND_NOTIFIED, FINISHED}, but calling .set() transitions the UniversalEvent._fut to FINISHED.

See the following traceback:

In [1]: from curio import run, UniversalEvent

In [2]: e = UniversalEvent()

In [3]: async def f():
   ...:     print('setting')
   ...:     await e.set()
   ...:     print('done')
   ...:     

In [4]: run(f)
setting
done

In [5]: run(f)
setting
---------------------------------------------------------------------------
InvalidStateError                         Traceback (most recent call last)
Input In [5], in <cell line: 1>()
----> 1 run(f)

File ~/.virtualenvs/py3.10/lib/python3.10/site-packages/curio/kernel.py:822, in run(corofunc, with_monitor, selector, debug, activations, *args, **kernel_extra)
    819     kernel.run(m.start)
    821 with kernel:
--> 822     return kernel.run(corofunc, *args)

File ~/.virtualenvs/py3.10/lib/python3.10/site-packages/curio/kernel.py:172, in Kernel.run(self, corofunc, shutdown, *args)
    169     self._shutdown_funcs = None
    171 if ret_exc:
--> 172     raise ret_exc
    173 else:
    174     return ret_val

File ~/.virtualenvs/py3.10/lib/python3.10/site-packages/curio/kernel.py:736, in Kernel._make_kernel_runtime.<locals>.kernel_run(***failed resolving arguments***)
    734 while current:
    735     try:
--> 736         trap = current.send(current._trap_result)
    737     except BaseException as e:
    738         # If any exception has occurred, the task is done.
    739         current = None

File ~/.virtualenvs/py3.10/lib/python3.10/site-packages/curio/task.py:167, in Task.send(self, value)
    161 def send(self, value):
    162     '''
    163     Send the next value into the task coroutine.  This method is
    164     a wrapper around the actual method and may be subclassed to
    165     implement different kinds of low-level functionality.
    166     '''
--> 167     return self._send(value)

File ~/.virtualenvs/py3.10/lib/python3.10/site-packages/curio/task.py:171, in Task._task_runner(self, coro)
    169 async def _task_runner(self, coro):
    170     try:
--> 171         return await coro
    172     finally:
    173         if self.taskgroup:

Input In [3], in f()
      1 async def f():
      2     print('setting')
----> 3     await e.set()
      4     print('done')

File ~/.virtualenvs/py3.10/lib/python3.10/site-packages/curio/sync.py:96, in UniversalEvent.set(self)
     93 @awaitable(set)
     94 async def set(self):
     95     self._set = True
---> 96     self._fut.set_result(True)

File /usr/lib/python3.10/concurrent/futures/_base.py:534, in Future.set_result(self, result)
    532 with self._condition:
    533     if self._state in {CANCELLED, CANCELLED_AND_NOTIFIED, FINISHED}:
--> 534         raise InvalidStateError('{}: {!r}'.format(self._state, self))
    535     self._result = result
    536     self._state = FINISHED

InvalidStateError: FINISHED: <Future at 0x7fba3870bee0 state=finished returned bool>

Note that the regular Event class does not behave this way:

In [6]: from curio import run, Event

In [7]: e = Event()

In [8]: async def f():
   ...:     print('setting')
   ...:     await e.set()
   ...:     print('done')
   ...:     

In [9]: run(f)
setting
done

In [10]: run(f)
setting
done

Recommend updating the implementation of .set() from this:

    def set(self):
        self._set = True
        self._fut.set_result(True)

    @awaitable(set)
    async def set(self):
        self._set = True
        self._fut.set_result(True)

    @asyncioable(set)
    async def set(self):
        self._set = True
        self._fut.set_result(True)

to this:

    def set(self):
        if self._set:
            return
        self._set = True
        self._fut.set_result(True)

    @awaitable(set)
    async def set(self):
        if self._set:
            return
        self._set = True
        self._fut.set_result(True)

    @asyncioable(set)
    async def set(self):
        if self._set:
            return
        self._set = True
        self._fut.set_result(True)

I'm happy to put up a PR to that effect if you agree.

dabeaz commented 2 years ago

This sounds like a good change. Let's do it!

stharding commented 2 years ago

Thanks Dave! I'll put up a PR in just a sec.

stharding commented 2 years ago

Ok, PR is up. Not sure how to link it to this issue, so I just put this issue number in the title.