BrighterCommand / BrighterCommand/Brighter

Reconsider BrighterAsyncContext as the default for the async Proactor pump

Open
#4,121 4 comments 1 reaction 0 assignees View on GitHub
under_discussion V11
Dominant language
C#
Stars
2.5k
Forks
296
Avg merge
1d 11h
Merged PRs (30d)
21

Description

## Why I'm opening this

I've been going through Brighter's use of `BrighterAsyncContext` and started wondering whether it earns its keep as the default for the async Proactor pump. Posting to start a real discussion before any code change.

TL;DR: there's a reasonable case for **eventually** flipping the default, staged, not silently.

## What `BrighterAsyncContext` is doing today

Two distinct jobs:

1. **Sync-over-async bridge** for sync APIs wrapping async-only implementations. `Receive() => BrighterAsyncContext.Run(() => ReceiveAsync(...))` and the same pattern across ~60 transport / outbox / luggage-store call sites.
2. **Ambient execution context for the async Proactor pump.** `Proactor.Run()` line 99 wraps the whole pump lifetime; every continuation in receive / translate / dispatch / ack runs through Brighter's paired `SynchronizationContext` + `TaskScheduler`.

This issue is **only** about (2). Job (1) earns its place — known-good idiom, removing it would require auditing every transport for context-capture deadlock risk.

## ADR 0023 is from 2019

[ADR 0023](docs/adr/0023-reactor-and-nonblocking-io.md) explains the original choice. The §Consequences section names this exact alternative and rejects it:

> "We could decide that as we cannot control ConfigureAwait, we should just choose to queue on the threadpool and use the default synchronizationcontext and task scheduler."

The reasoning was sound for 2019, but the ecosystem has shifted:

- ASP.NET Core has run without a sync context for years.
- MassTransit, Wolverine, NServiceBus all run on default scheduler. Per-partition / per-consumer ordering comes from *consumer design*, not custom schedulers.
- Modern threadpool work-stealing is significantly faster than when ADR 0023 was written.

"ADR 0023 said no in 2019" isn't a strong defense in 2026.

## `continueOnCapturedContext: true` is doing less than it looks

`Proactor.cs:123, 130` dispatch with `continueOnCapturedContext: true`. Looks load-bearing. But the loop is:

```
await Channel.ReceiveAsync(TimeOut) // sequential
await TranslateMessage(message, context) // sequential
await CommandProcessor.SendAsync(...) // sequential
await Channel.AcknowledgeAsync(message) // sequential
// next iteration
```

Every step is awaited. Iteration N+1 cannot start until N completes regardless of which thread runs continuations. **Per-performer ordering comes from the sequential await chain, not the scheduler.** True under any scheduler.

The custom scheduler matters only in narrow cases:

- Handlers using `ThreadLocal` and assuming same-thread continuations (anti-pattern in async code).
- Handlers doing `Task.Run` / fire-and-forget — but those break ordering under any scheduler.
- `ConfigureAwait(false)` defensive cases — the paired `TaskScheduler` exists specifically to defeat the leak (`BrighterAsyncContext.cs` lines 5-7), real but only for code that doesn't fight it explicitly.

## The case for change

1. **Debuggability.** Stack traces, profilers, IDE async-stepping all assume default scheduler. Custom schedulers complicate diagnostics.
2. **Ecosystem alignment.** Peers (MassTransit, Wolverine, NServiceBus) demonstrate default scheduler is workable for ordered async messaging.
3. **`ConfigureAwait(false)` semantics.** Stephen Toub's [ConfigureAwait FAQ](https://devblogs.microsoft.com/dotnet/configureawait-faq/) — the canonical .NET guidance — recommends every await in library code use `ConfigureAwait(false)` so callers don't have to think about context capture. Brighter's own internal code follows this (e.g. `Proactor.cs:524` in `TranslateMessage`). But the paired `TaskScheduler` in `BrighterAsyncContext` is specifically designed to *defeat* `ConfigureAwait(false)` so continuations land back on the pump thread (see header comment of `BrighterAsyncContext.cs` lines 5-7). The project is pulling against its own use of the standard library convention. Defensible if there's a strong reason, but with sequential `await` already enforcing per-performer ordering, the strong reason is hard to find.
4. **Hard-coded `continueOnCapturedContext: true`** at `Proactor.cs:123, 130` looks load-bearing but isn't. Sequential `await` already enforces per-performer ordering.

## Concrete proposal

Not asking for a default flip today. Staged path:

**Step 1 (next minor):** Opt-in `AsyncPumpSchedulerMode` on subscription / dispatcher:

```csharp
public enum AsyncPumpSchedulerMode
{
BrighterSingleThreaded, // current behaviour, default
RuntimeDefault,
}
```

`BrighterAsyncContext` stays for sync-over-async bridges. Only the wrap around `EventLoop` becomes conditional.

**Step 2 (after a minor of telemetry):** Survey users, look for issues. If `RuntimeDefault` shakes out as expected, plan default flip for next major with clear release notes:

- Handlers using `ThreadLocal` (rare, anti-pattern, exists in legacy code).
- Custom transports relying on thread affinity (in-tree ones don't; needs audit).
- Tests asserting single-threaded execution (e.g. `ProactorShutdownInsideAsyncContextTests`).

**Step 3 (same major):** Drop hard-coded `continueOnCapturedContext: true` in `Proactor.cs:123, 130`. Driven by mode setting, or default `false` for `RuntimeDefault`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.