Skip to content

events: reconsider per-event task spawn for handler panic isolation #3

Description

@senamakel

What it does now

events::subscriber::spawn isolates a panicking handler by running each dispatch as its own task and inspecting the JoinHandle:

let dispatch = {
    let handler = handler.clone();
    let event = event.clone();
    tokio::spawn(async move { handler.handle(&event).await })
};
if let Err(join) = dispatch.await {
    tracing::error!(handler = task_name, panicked = join.is_panic(), "handler failed, continuing");
}

The isolation itself is not in question — a handler panicking must not kill the subscriber loop, because that silently unsubscribes it and nothing anywhere says so. The question is the mechanism.

Why it was written this way

tokio already turns a panicking task into an Err on its JoinHandle, so this gets isolation for free without a dependency — in a crate whose entire purpose is removing dependencies from a graph. The port it replaced (OpenHuman's core::event_bus) used futures::FutureExt::catch_unwind inline instead, which would have meant adding futures-util here.

What it costs

  1. A task allocation per event, per subscriber. Every subscriber pays it on every matching event, whether or not any handler ever panics.
  2. An extra hop on the delivery path. An event now reaches a handler via subscriber-loop → spawned task, where it used to run inline. This is observable: it broke a downstream test that asserted on delivery after a single yield_now() (refactor(core): replace the event bus with tinybus openhuman#5459 — that test now waits on a deadline). Anything else assuming near-synchronous delivery will be affected the same way.
  3. Event: Clone is partly forced by it. The event is cloned into the task so the future can be 'static.

It does not cost concurrency: the loop awaits each dispatch, so per-subscriber ordering is preserved exactly as before.

Options

a. Keep it. The bus is human-scale by design — "transcribe a file", "sign a transaction" — and a task spawn is cheap next to what these events trigger. Cost is real but probably irrelevant at the actual event rate.

b. catch_unwind inline. Removes the hop and the allocation, restores near-synchronous delivery. Costs futures-util as a dependency, which is the thing this crate is least willing to spend.

c. Isolate per subscriber rather than per event. Let the panic take the dispatch task, and have the loop notice and restart it. One allocation per panic instead of one per event, and no dependency — but it needs care around what happens to the handler's state after a panic, and it changes Arc<dyn EventHandler> lifetime assumptions.

d. Make it configurable. Almost certainly the wrong answer — two delivery paths, one of which is barely exercised.

What would settle it

  • A benchmark at a realistic event rate. If the spawn is lost in the noise next to the work an event triggers, (a) wins and this closes as documented-and-intentional.
  • A decision on whether near-synchronous local delivery is part of the contract. If callers may reasonably assume a publish is visible to an in-process subscriber after one yield, that argues for (b) or (c) regardless of throughput — the current behaviour is a silent trap for exactly the kind of test that caught it.

Raised because this was my call while porting and it is a design trade a reviewer should get to overrule, not something to leave buried in a comment. Context: #1 (the port), #2 (the runtime fix).

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestquestionFurther information is requested

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions