Skip to content

Commit 2eb48f2

Browse files
authored
fix: resume interaction task handlers on the captured context (#4409)
## What kind of change does this PR introduce? Bug fix. ## What is the new behavior? `Interaction.RegisterHandler(Func<..., Task>)` now resumes the async handler on the captured `SynchronizationContext`. A handler registered from a UI thread runs its body back on that UI context, so it can touch UI-only APIs without re-dispatching. The handler still yields off the current scheduler trampoline before running (see #4351). ## What is the current behavior? After the interaction performance refactor, the overload awaited an internal `YieldToCurrentContext()` with `.ConfigureAwait(false)`. `Task.Yield()` posts the continuation back to the captured context, but the outer `.ConfigureAwait(false)` discarded it, so the handler resumed on a thread-pool thread. UI handlers then failed unless they manually marshalled back to the UI thread. Closes #4393. ## What might this PR break? None. The handler still yields before running, so the #4351 trampoline-avoidance behaviour is preserved; only the resumption thread is corrected back to the captured context, matching the documented and pre-refactor behaviour. ## Checklist - [x] I have read the Contribute guide - [x] Tests have been added or updated (for bug fixes / features) - [ ] Docs have been added or updated (for bug fixes / features) - [x] Changes target the `main` branch - [x] PR title follows Conventional Commits ## Additional information - Fix: `await Task.Yield()` directly instead of awaiting a `Task`-returning helper. `Task.Yield()` yields off the trampoline while resuming on the captured context; a bare `Task` await cannot use `ConfigureAwait(false)` without re-introducing the bug (and awaiting a `Task` without it trips the repo's ConfigureAwait analyzer). The now-dead `YieldToCurrentContext` helper is removed. - Verified by test, not reasoning: reintroducing `.ConfigureAwait(false)` on the yield makes the new context test fail (the handler observes a null / thread-pool context); the fix makes it pass. The #4351 ordering test stays green in both, confirming the yield is preserved. - Coverage: `Interaction.cs` is at 100% line (58/58) and 100% branch (8/8) coverage, measured across `ReactiveUI.Tests` (net9.0). - The new tests install a single-threaded `SynchronizationContext` to assert resumption on the captured context on Linux/CI; a real UI dispatcher (WPF/WinUI) exhibits the same contract but is not exercised directly here.
1 parent e6088d3 commit 2eb48f2

2 files changed

Lines changed: 118 additions & 7 deletions

File tree

‎src/ReactiveUI.Shared/Interactions/Interaction.cs‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -118,16 +118,17 @@ public IDisposable RegisterHandler(Func, Ta
118118

119119
return RegisterHandlerCore(ContentHandler);
120120

121-
// Yield before invoking the async handler so it is not run inside the current scheduler
122-
// trampoline (see #4351).
123121
IObservable<RxVoid> ContentHandler(IInteractionContext<TInput, TOutput> interaction) =>
124122
new TaskUnitObservable(InvokeAsync(interaction, handler));
125123

126124
static async Task InvokeAsync(
127125
IInteractionContext<TInput, TOutput> interaction,
128126
Func<IInteractionContext<TInput, TOutput>, Task> asyncHandler)
129127
{
130-
await YieldToCurrentContext().ConfigureAwait(false);
128+
// Yield so the handler is not invoked inside the current scheduler trampoline (see #4351). Task.Yield
129+
// resumes on the captured SynchronizationContext, so a handler registered from a UI thread runs back on
130+
// that context instead of a thread-pool thread (see #4393); ConfigureAwait(false) here would discard it.
131+
await Task.Yield();
131132
await asyncHandler(interaction).ConfigureAwait(false);
132133
}
133134
}
@@ -169,10 +170,6 @@ protected Func, IObservable>[] GetH
169170
protected virtual IOutputContext<TInput, TOutput> GenerateContext(TInput input) =>
170171
new InteractionContext<TInput, TOutput>(input);
171172

172-
/// Yields once so asynchronous handlers are not invoked inside the current scheduler trampoline.
173-
/// A task that completes after the current context has yielded.
174-
private static async Task YieldToCurrentContext() => await Task.Yield();
175-
176173
/// Registers a normalized interaction handler that produces a stream.
177174
/// The normalized handler.
178175
/// A disposable which unregisters the handler.

‎src/tests/ReactiveUI.Tests/InteractionsTest.cs‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// The .NET Foundation licenses this file to you under the MIT license.
44
// See the LICENSE file in the project root for full license information.
55

6+
using System.Collections.Concurrent;
67
using ReactiveUI.Tests.Utilities.Schedulers;
78
using TUnit.Core.Executors;
89

@@ -270,4 +271,117 @@ public async Task UnhandledInteractionsShouldCauseException()
270271
await Assert.That(ex.Input).IsEqualTo("bar");
271272
}
272273
}
274+
275+
///
276+
/// A task-based handler registered while a is installed resumes on that
277+
/// captured context rather than on a thread-pool thread, so UI handlers run on the UI thread (see issue #4393).
278+
///
279+
/// A representing the asynchronous operation.
280+
[Test]
281+
public async Task TaskHandlerResumesOnCapturedSynchronizationContext()
282+
{
283+
using var uiContext = new SingleThreadedSynchronizationContext();
284+
var observedContext = new TaskCompletionSource<SynchronizationContext?>(TaskCreationOptions.RunContinuationsAsynchronously);
285+
var completed = new TaskCompletionSource<RxVoid>(TaskCreationOptions.RunContinuationsAsynchronously);
286+
287+
uiContext.Post(
288+
static state =>
289+
{
290+
var (observed, done) = ((TaskCompletionSource<SynchronizationContext?>, TaskCompletionSource<RxVoid>))state!;
291+
var interaction = new Interaction<RxVoid, RxVoid>();
292+
_ = interaction.RegisterHandler(async context =>
293+
{
294+
_ = observed.TrySetResult(SynchronizationContext.Current);
295+
context.SetOutput(RxVoid.Default);
296+
await Task.CompletedTask;
297+
});
298+
299+
_ = interaction.Handle(RxVoid.Default).Subscribe(
300+
_ => done.TrySetResult(RxVoid.Default),
301+
done.SetException);
302+
},
303+
(observedContext, completed));
304+
305+
var handlerContext = await observedContext.Task;
306+
_ = await completed.Task;
307+
308+
await Assert.That(handlerContext).IsSameReferenceAs(uiContext);
309+
}
310+
311+
///
312+
/// A task-based handler yields before running, so it is not invoked inside the subscription that triggers the
313+
/// interaction (see issue #4351); the handler only runs once the triggering call has unwound.
314+
///
315+
/// A representing the asynchronous operation.
316+
[Test]
317+
public async Task TaskHandlerDoesNotRunInsideTriggeringSubscription()
318+
{
319+
const string afterSubscribeMarker = "after-subscribe";
320+
const string handlerMarker = "handler";
321+
const int expectedStepCount = 2;
322+
323+
using var uiContext = new SingleThreadedSynchronizationContext();
324+
var order = new List<string>();
325+
var completed = new TaskCompletionSource<IReadOnlyList<string>>(TaskCreationOptions.RunContinuationsAsynchronously);
326+
327+
uiContext.Post(
328+
static state =>
329+
{
330+
var (steps, done) = ((List<string>, TaskCompletionSource<IReadOnlyList<string>>))state!;
331+
var interaction = new Interaction<RxVoid, string>();
332+
_ = interaction.RegisterHandler(async context =>
333+
{
334+
steps.Add(handlerMarker);
335+
context.SetOutput(ResultOutput);
336+
await Task.CompletedTask;
337+
});
338+
339+
_ = interaction.Handle(RxVoid.Default).Subscribe(
340+
_ => done.TrySetResult(steps),
341+
done.SetException);
342+
343+
// Recorded before the yielded handler continuation is pumped, so it must precede the handler marker.
344+
steps.Add(afterSubscribeMarker);
345+
},
346+
(order, completed));
347+
348+
var sequence = await completed.Task;
349+
350+
using (Assert.Multiple())
351+
{
352+
await Assert.That(sequence).Count().IsEqualTo(expectedStepCount);
353+
await Assert.That(sequence[0]).IsEqualTo(afterSubscribeMarker);
354+
await Assert.That(sequence[1]).IsEqualTo(handlerMarker);
355+
}
356+
}
357+
358+
/// A minimal single-threaded backed by one pumped worker thread.
359+
private sealed class SingleThreadedSynchronizationContext : SynchronizationContext, IDisposable
360+
{
361+
/// The queue of work items pumped on the worker thread in order.
362+
private readonly BlockingCollection<Action> _queue = new();
363+
364+
/// Initializes a new instance of the class.
365+
public SingleThreadedSynchronizationContext()
366+
{
367+
var thread = new Thread(Run) { IsBackground = true, Name = "interaction-ui" };
368+
thread.Start();
369+
}
370+
371+
///
372+
public override void Post(SendOrPostCallback d, object? state) => _queue.Add(() => d(state));
373+
374+
///
375+
public void Dispose() => _queue.CompleteAdding();
376+
377+
/// Pumps queued work on the worker thread with this context installed as current.
378+
private void Run()
379+
{
380+
SynchronizationContext.SetSynchronizationContext(this);
381+
foreach (var work in _queue.GetConsumingEnumerable())
382+
{
383+
work();
384+
}
385+
}
386+
}
273387
}

0 commit comments

Comments
 (0)