Should we deprecate trio.Event.clear?

[njsmith]

It occurs to me that I've seen 4 different pieces of code try to use Event.clear lately, and they were all buggy:

• My first suggestion for solving Add a blocked task watchdog #591 used an Event that we called clear on, and this created a race condition. (This was using threading.Event, but the principle is the same.) Details: Add task blocked watchdog. #596 (comment)

• New signal API: trio.open_signal_receiver #619 uses it correctly (I think), but it's only safe because we enforce that only one task can call SignalReceiver.__anext__, which is subtle enough that I originally forgot to enforce it

• @belm0 tried to use Event to track a value that toggles between true and false, but then found it wasn't appropriate for what he needed after all. (Not exactly Event's fault, but if we didn't have the clear method then I'm sure he'd have realized more quickly that it wasn't what he was looking for.)

• @HyperionGray's websocket library tries to use Event objects to pass control back and forth between calls to send_message (in the main task) and a background writer task. Here's the writer task:

  https://github.com/HyperionGray/trio-websocket/blob/b787bf1a8a026ef1d9ca995d044bc97d42e7f727/trio_websocket/__init__.py#L300-L305

  If another task calls send_message while the writer task is blocked in send_all, then the send_message call will set() the event again, and then when send_all completes, it gets unconditionally cleared, so we end up in the invalid state where there is data pending, but self._data_pending is not set.

Now, maybe this isn't Event.clear's fault, but it makes me wonder :-). (And this is partly coming out of my general reassessment of the APIs we inherited from the stdlib threading module, see also #322 and #573.)

The core use case for Event is tracking whether an event has happened, and broadcasting that to an arbitrary number of listeners. For this purpose, clear isn't meaningful: once an event has happened, it can't unhappen. And if you stick to this core use case, Event seems very robust and difficult to mis-use.

All of the trouble above came when someone tried to use it for something outside of this core use case. Some of these patterns do make sense:

• If you have a periodic event, you might want to have the semantics of "wait for the next event". That can be done with an Event, where waiters call await ev.wait() and wakers call ev.set(); ev.clear(). But it can also be done with a Condition or a ParkingLot, or we could have a PeriodicEvent type if it comes up enough... for a dedicated PeriodicEvent it might also make sense to have a close method of some kind to avoid race conditions at shutdown, where tasks call wait after the last event has happened and deadlock.

  • Another option in many cases is to model a periodic event by creating one Event object per period. This is nice because it allows you to have overlapping periods. For example, consider a batching API, where tasks submit requests, and then every once in a while they get gathered up and submitted together. The submitting tasks want to wait until their request has been submitted. One way to do it would be to have an Event for each submission period. When a batch is gathered up for submission, the Event gets replaced, but the old Event doesn't get set until after the submission finishes. Maybe this is a pattern we should be nudging people towards, because it's more general/powerful.

• The websocket example above could be made correct by moving the clear so that it's right after the wait, and before the call that consumes the data (data = self._wsproto.bytes_to_send()). (It might be more complicated if the consuming call wasn't itself synchronous.) So ev.wait(); ev.clear() can make sense... IF we know there is exactly one task listening. Which is way outside Event's core use case. In this case, it's basically a way of "passing control" from one task to another, which is often a mistake anyway – Python already has a very easy way to express sequential control like this, just execute the two things in the same task :-). Here I think a Lock would be better in any case: WebSocketConnection should implement the trio.abc.AsyncResource interface trio-websocket#3

Are there any other use cases where Event.clear is really the right choice? Or where maybe it's not the right choice, but it's currently the best available choice?

[smurfix]

Well … I'd start with adding that close-able PeriodicEvent, though we might want to select a better name (not all multi-events are "periodic"). Personally I'd use "signal" but that's taken. "Blinker"?

Then, deprecate Event.clear.

As to the websocket example: If you have a background task that sends data, shouldn't you use a queue?

[njsmith]

@smurfix I'm not sure whether the PeriodicEvent use case actually comes up often enough to make it a new standard thing... I was thinking more that we should start keeping an eye out for it.

[njsmith]

(Note that it wouldn't have been useful in any of the cases above, except maybe @belm0's and my impression is that he needs something more complex anyway.)

[miracle2k]

I have been using Event.clear() (in the "right" way being discussed above) multiple times, but whenever I do, I get scared - does it really behave the way I expect? Can I trust that all waiters have been awoken before clear() goes into effect? I have to think it through and look at the source every time.

So I agree that a separate API for this use case might make things easier and clearer.

[njsmith]

@miracle2k oh cool, tell us more :-). When you say "the right way", do you mean doing ev.set(); ev.clear() for a use case like my "periodic event" suggestion? (Though @smurfix is right we should have a name that doesn't imply the periods are equal length, maybe "repeated event".) Can you say more about your use case?

[belm0]

I'm not sure having another special-purpose event is the right path in terms of ease of use and clear API.

A counter example: it was trivial to make an event supporting level and edge trigger with any polarity and a simple property interface for the value (sorry, impl omitted):

class BoolEvent:
    """Boolean offering the ability to wait for a value or transition."""

    def __init__(self, value=False): ...

    @property
    def value(self): ...

    @value.setter
    def value(self, x): ...

    async def wait_value(self, x=True):
        """Wait until given value."""
        ...

    async def wait_transition(self, x=True):
        """Wait until transition to given value."""
        ...

Using a property and not having the user bother about set/clear/get is very nice, as is making level and edge trigger very clear in the api and supporting all use cases an once.

Implementation uses 4 ParkingLot instances, but these can be allocated lazily, and obviously halved if you remove the polarity option.

And this same API can be used with arbitrary value types and predicates:

foo = ValueEvent(12.5)
...
await foo.wait_transition(lambda x: 50 < x <= 100)

[miracle2k]

@njsmith Yes its the 'wait for the next event' usecase you already mentioned.

[njsmith]

@miracle2k Do you have 1 waiter, or many? Is there data associated with the events? Do you have any trouble around shutting down?

[njsmith]

Further observation: there's a simple theoretical reason that Event.clear specifically is error-prone. If Event.clear doesn't exist, then Event.wait guarantees a simple invariant: "the value is now True". But if you have Event.clear, then the invariant that Event.wait guarantees is: "the value was True at some point in the recent past, but at this point who knows"

[edited: was "if you have Event.set"]

[miracle2k]

@njsmith Never had a any issue with shutting down. I think that is probably because my shutdowns, even if I want to controlled shutdown of a part of the task tree, always use trio's cancellation, so that should prevent wait() from deadlocking (I assume).

I just ran across a case where I used this approach: It is essentially one task communicating updates to a second task, but I am not using a queue, because if the second task is too busy to respond to all updates, I just want it to work with the latest update once it has time.

So the publisher does:

current_state = 42
state_available.set()

and the consumer does:

```python
while True:
    await state_available.wait()
    await state_available.clear()
    await process(current_state)

[smurfix]

I just ran across a case where I used this approach: It is essentially one task communicating updates to a second task, but I am not using a queue, because if the second task is too busy to respond to all updates, I just want it to work with the latest update once it has time.

Hmm. I'd use a queue for that, adding a (synchronous) Queue.put_force method that drops the oldest element if the queue is full.

[njsmith]

@miracle2k Ah interesting! So for this use case a pure wait-for-next-repeating-event isn't actually quite the same as what you're doing now. Specifically, consider the case where await process(...) takes so long that a new update is already published before it finishes. With you current design, the consumer will detect this and immediately start processing it without waiting. With a wait-for-next-repeating-event API, in this case it would ignore the new data and sit idle into new, new data became available.

This way of using an Event as a single consumer, is-there-work-to-do flag is actually pretty similar to how SignalReceiver uses it (#619). It's a bit different because SignalReceiver is like a queue with idiosyncratic rules (basically duplicate items get dropped), so we only clear after observing that the queue is empty. "Queue with special rule for coalescing in-flight objects" sounds like it covers both of these cases, and is kind of where @smurfix's mind is going too. Actually, a kind of queue where each item has a key and a value and we only saved the latest value for each key would be sufficient for both SignalReceiver (which would always use None for the value), and for @miracle2k's case (which would always use None for the key). I wonder if there are any cases that would use non-None keys and values simultaneously.

(@smurfix though on another note, I've also recently been pondering the possibility of have a put_force that ignores limits but doesn't drop any items – the motivating example is a work queue where you have items coming in from some external process, plus some work items generate new work items when processed, and you have some kind of rate limiting on processing items off of the queue. Here you want to apply backpressure to the external source, but not to the workers themselves, because that risks deadlocks. The relevance here is mostly to keep in mind that there might be multiple kinds of "forcing".)

Another direction to generalize @miracle2k's case would be to make it multi-consumer, like a value that broadcasts updates. The interesting thing here is that you want to track, for each consumer, which was the latest update that it saw. Maybe something like, calling handle.wait_new_value() returns a pair (new_value, new_handle), and then you process new_data and call new_handle.wait_new_value()? That would be simple to implement using the "one Event per period" model mentioned in my original post.