Skip to content

Commit 303d184

Browse files
ChrisPulmanChrisPulman
authored
Fix for 4280 interaction async handler scheduling (#4351)
<!-- Please be sure to read our [Contribute guide](https://www.reactiveui.net/contribute/index.html) before opening a PR. --> ## What kind of change does this PR introduce? <!-- Bug fix, feature, docs update, refactor, ci, ... --> Fix ## What is the current behavior? <!-- You can also link to an open issue here. Use "Closes #123" to auto-close on merge. --> Closes #4280 ## What is the new behavior? <!-- If this is a feature change --> This pull request refactors the `Interaction` handler registration and execution logic in `ReactiveUI` to improve consistency, reliability, and testability, especially around asynchronous handler scenarios. It introduces a unified handler registration core, ensures asynchronous handlers yield to the current context, and updates tests to use modern async/await patterns. Several new tests are also added to verify scheduler release behavior for both task-based and observable handlers. **Core handler registration and async context improvements:** * Introduced a private `RegisterHandlerCore` method to centralize handler registration and disposal logic, replacing duplicated code in `RegisterHandler` overloads. [[1]](diffhunk://#diff-4c32538b51e31225de509d21d08ea6cfd573a7df738530957077df781d5d3ba9L90-R129) [[2]](diffhunk://#diff-4c32538b51e31225de509d21d08ea6cfd573a7df738530957077df781d5d3ba9R172-R192) * Added a private `YieldToCurrentContext` method to ensure asynchronous handlers (both `Task` and `IObservable`-based) yield execution before invoking user code, preventing scheduler trampoline issues and improving nested interaction support. [[1]](diffhunk://#diff-4c32538b51e31225de509d21d08ea6cfd573a7df738530957077df781d5d3ba9L90-R129) [[2]](diffhunk://#diff-4c32538b51e31225de509d21d08ea6cfd573a7df738530957077df781d5d3ba9R172-R192) **Test modernization and reliability:** * Updated all relevant tests in `InteractionsTest.cs` to use `async`/`await` and `ToTask()` instead of synchronous blocking (`Wait()`/`FirstAsync().Wait()`), improving test reliability and better reflecting real-world async usage. [[1]](diffhunk://#diff-f2ed870457c71b058a31b5f78fde682cb52406c51bd54a6626d8f71de439d2ceL26-R28) [[2]](diffhunk://#diff-f2ed870457c71b058a31b5f78fde682cb52406c51bd54a6626d8f71de439d2ceL45-R60) [[3]](diffhunk://#diff-f2ed870457c71b058a31b5f78fde682cb52406c51bd54a6626d8f71de439d2ceL177-R258) [[4]](diffhunk://#diff-f2ed870457c71b058a31b5f78fde682cb52406c51bd54a6626d8f71de439d2ceL210-R277) [[5]](diffhunk://#diff-f2ed870457c71b058a31b5f78fde682cb52406c51bd54a6626d8f71de439d2ceL225-R304) [[6]](diffhunk://#diff-f2ed870457c71b058a31b5f78fde682cb52406c51bd54a6626d8f71de439d2ceL264-R345) * Marked the `InteractionsTest` class with `[NotInParallel]` to ensure tests are not run in parallel, avoiding flakiness due to shared state. **Expanded test coverage for async handler behavior:** * Added tests to verify that both `Task`-based and `IObservable`-based handlers release the current scheduler before executing user code, ensuring nested interactions are not blocked and behave as expected. **Test improvements for handler precedence and subscription timing:** * Enhanced tests to more reliably detect when handlers are subscribed and to ensure correct handler precedence, especially in asynchronous scenarios, using `TaskCompletionSource` for precise subscription tracking. [[1]](diffhunk://#diff-f2ed870457c71b058a31b5f78fde682cb52406c51bd54a6626d8f71de439d2ceR100-R120) [[2]](diffhunk://#diff-f2ed870457c71b058a31b5f78fde682cb52406c51bd54a6626d8f71de439d2ceR129) These changes collectively make handler registration more robust, improve async interaction patterns, and ensure that both the implementation and test suite are future-proof and reliable. ## What might this PR break? ## Checklist - [x] I have read the [Contribute guide](https://www.reactiveui.net/contribute/index.html) - [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](https://www.conventionalcommits.org/) ## Additional information
1 parent b54d29b commit 303d184

2 files changed

Lines changed: 162 additions & 47 deletions

File tree

‎src/ReactiveUI/Interactions/Interaction.cs‎

Lines changed: 43 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -87,30 +87,46 @@ public IDisposable RegisterHandler(Action<IInteractionContext<TInput, TOutput>>
8787
{
8888
ArgumentExceptionHelper.ThrowIfNull(handler);
8989

90-
return RegisterHandler(interaction =>
90+
IObservable<Unit> ContentHandler(IInteractionContext<TInput, TOutput> interaction)
9191
{
9292
handler(interaction);
9393
return Observables.Unit;
94-
});
94+
}
95+
96+
return RegisterHandlerCore(ContentHandler);
9597
}
9698

9799
/// <inheritdoc />
98100
public IDisposable RegisterHandler(Func<IInteractionContext<TInput, TOutput>, Task> handler)
99101
{
100102
ArgumentExceptionHelper.ThrowIfNull(handler);
101103

102-
return RegisterHandler(interaction => handler(interaction).ToObservable());
104+
IObservable<Unit> ContentHandler(IInteractionContext<TInput, TOutput> interaction) =>
105+
Observable.FromAsync(
106+
async () =>
107+
{
108+
await YieldToCurrentContext();
109+
await handler(interaction).ConfigureAwait(false);
110+
});
111+
112+
return RegisterHandlerCore(ContentHandler);
103113
}
104114

105115
/// <inheritdoc />
106116
public IDisposable RegisterHandler<TDontCare>(Func<IInteractionContext<TInput, TOutput>, IObservable<TDontCare>> handler)
107117
{
108118
ArgumentExceptionHelper.ThrowIfNull(handler);
109119

110-
IObservable<Unit> ContentHandler(IInteractionContext<TInput, TOutput> context) => handler(context).Select(_ => Unit.Default);
120+
IObservable<Unit> ContentHandler(IInteractionContext<TInput, TOutput> context) =>
121+
Observable.FromAsync(
122+
async () =>
123+
{
124+
await YieldToCurrentContext();
125+
return handler(context);
126+
})
127+
.SelectMany(result => result.Select(_ => Unit.Default));
111128

112-
AddHandler(ContentHandler);
113-
return Disposable.Create(() => RemoveHandler(ContentHandler));
129+
return RegisterHandlerCore(ContentHandler);
114130
}
115131

116132
/// <inheritdoc />
@@ -153,6 +169,27 @@ protected Func<IInteractionContext<TInput, TOutput>, IObservable<Unit>>[] GetHan
153169
/// <returns>The interaction context.</returns>
154170
protected virtual IOutputContext<TInput, TOutput> GenerateContext(TInput input) => new InteractionContext<TInput, TOutput>(input);
155171

172+
/// <summary>
173+
/// Yields once so asynchronous handlers are not invoked inside the current scheduler trampoline.
174+
/// </summary>
175+
/// <returns>A task that completes after the current context has yielded.</returns>
176+
private static async Task YieldToCurrentContext()
177+
{
178+
await Task.Yield();
179+
}
180+
181+
/// <summary>
182+
/// Registers a normalized interaction handler.
183+
/// </summary>
184+
/// <param name="contentHandler">The normalized handler.</param>
185+
/// <returns>A disposable which unregisters the handler.</returns>
186+
private IDisposable RegisterHandlerCore(Func<IInteractionContext<TInput, TOutput>, IObservable<Unit>> contentHandler)
187+
{
188+
ArgumentExceptionHelper.ThrowIfNull(contentHandler);
189+
AddHandler(contentHandler);
190+
return Disposable.Create(() => RemoveHandler(contentHandler));
191+
}
192+
156193
/// <summary>
157194
/// Adds a handler delegate to be invoked for interaction contexts.
158195
/// </summary>

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

Lines changed: 119 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ namespace ReactiveUI.Tests;
1010
/// <summary>
1111
/// Tests interactions.
1212
/// </summary>
13+
[NotInParallel]
1314
public class InteractionsTest
1415
{
1516
/// <summary>
@@ -23,8 +24,8 @@ public async Task AttemptingToGetInteractionOutputBeforeItHasBeenSetShouldCauseE
2324

2425
interaction.RegisterHandler(context => { _ = ((InteractionContext<Unit, Unit>)context).GetOutput(); });
2526

26-
var ex = Assert.Throws<InvalidOperationException>(() => interaction.Handle(Unit.Default).Subscribe());
27-
await Assert.That(ex.Message).IsEqualTo("Output has not been set.");
27+
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => interaction.Handle(Unit.Default).ToTask());
28+
await Assert.That(ex!.Message).IsEqualTo("Output has not been set.");
2829
}
2930

3031
/// <summary>
@@ -42,42 +43,54 @@ public async Task AttemptingToSetInteractionOutputMoreThanOnceShouldCauseExcepti
4243
context.SetOutput(Unit.Default);
4344
});
4445

45-
var ex = Assert.Throws<InvalidOperationException>(() => interaction.Handle(Unit.Default).Subscribe());
46-
await Assert.That(ex.Message).IsEqualTo("Output has already been set.");
46+
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => interaction.Handle(Unit.Default).ToTask());
47+
await Assert.That(ex!.Message).IsEqualTo("Output has already been set.");
4748
}
4849

4950
/// <summary>
5051
/// Tests that Handled interactions should not cause exception.
5152
/// </summary>
53+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
5254
[Test]
53-
public void HandledInteractionsShouldNotCauseException()
55+
public async Task HandledInteractionsShouldNotCauseException()
5456
{
5557
var interaction = new Interaction<Unit, bool>();
5658
interaction.RegisterHandler(static c => c.SetOutput(true));
5759

58-
interaction.Handle(Unit.Default).FirstAsync().Wait();
60+
await interaction.Handle(Unit.Default);
5961
}
6062

6163
/// <summary>
6264
/// Tests that Handlers are executed on handler scheduler.
6365
/// </summary>
6466
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
6567
[Test]
66-
[TestExecutor<WithSchedulerExecutor>]
6768
public async Task HandlersAreExecutedOnHandlerScheduler()
6869
{
69-
var scheduler = TestContext.Current!.GetScheduler();
70+
var schedulerThreadId = -1;
71+
using var scheduler = new EventLoopScheduler(
72+
threadStart =>
73+
{
74+
var thread = new Thread(threadStart) { IsBackground = true };
75+
schedulerThreadId = thread.ManagedThreadId;
76+
return thread;
77+
});
7078
var interaction = new Interaction<Unit, string>(scheduler);
79+
var handlerThreadId = -1;
7180

72-
using (interaction.RegisterHandler(x => x.SetOutput("done")))
81+
using (interaction.RegisterHandler(x =>
7382
{
74-
var handled = false;
75-
interaction
76-
.Handle(Unit.Default)
77-
.Subscribe(_ => handled = true);
83+
handlerThreadId = Environment.CurrentManagedThreadId;
84+
x.SetOutput("done");
85+
}))
86+
{
87+
var result = await interaction.Handle(Unit.Default).ToTask().WaitAsync(TimeSpan.FromSeconds(5));
7888

79-
// With ImmediateScheduler, handlers execute immediately
80-
await Assert.That(handled).IsTrue();
89+
using (Assert.Multiple())
90+
{
91+
await Assert.That(result).IsEqualTo("done");
92+
await Assert.That(handlerThreadId).IsEqualTo(schedulerThreadId);
93+
}
8194
}
8295
}
8396

@@ -95,16 +108,27 @@ public async Task HandlersCanContainAsynchronousCode()
95108

96109
// even though handler B is "slow" (i.e. mimicks waiting for the user), it takes precedence over A, so we expect A to never even be called
97110
var handler1AWasCalled = false;
111+
var handler1BWasSubscribed = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
98112
var handler1A = interaction.RegisterHandler(x =>
99113
{
100114
x.SetOutput("A");
101115
handler1AWasCalled = true;
102116
});
103117
var handler1B = interaction.RegisterHandler(x =>
104-
Observables
105-
.Unit
106-
.Delay(TimeSpan.FromSeconds(1), scheduler)
107-
.Do(_ => x.SetOutput("B")));
118+
{
119+
return Observable.Create<Unit>(
120+
observer =>
121+
{
122+
var subscription = Observables
123+
.Unit
124+
.Delay(TimeSpan.FromSeconds(1), scheduler)
125+
.Do(_ => x.SetOutput("B"))
126+
.Subscribe(observer);
127+
128+
handler1BWasSubscribed.TrySetResult();
129+
return subscription;
130+
});
131+
});
108132

109133
using (handler1A)
110134
using (handler1B)
@@ -113,6 +137,7 @@ public async Task HandlersCanContainAsynchronousCode()
113137
.Handle(Unit.Default)
114138
.ToObservableChangeSet(ImmediateScheduler.Instance).Bind(out var result).Subscribe();
115139

140+
await handler1BWasSubscribed.Task.WaitAsync(TimeSpan.FromSeconds(5));
116141
await Assert.That(result).IsEmpty();
117142
scheduler.AdvanceBy(TimeSpan.FromSeconds(0.5));
118143
await Assert.That(result).IsEmpty();
@@ -139,14 +164,67 @@ public async Task HandlersCanContainAsynchronousCodeViaTasks()
139164
return Task.FromResult(true);
140165
});
141166

142-
string? result = null;
143-
interaction
144-
.Handle(Unit.Default)
145-
.Subscribe(r => result = r);
167+
var result = await interaction.Handle(Unit.Default);
146168

147169
await Assert.That(result).IsEqualTo("result");
148170
}
149171

172+
/// <summary>
173+
/// Tests that task handlers release the current scheduler before invoking user code.
174+
/// </summary>
175+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
176+
[Test]
177+
public async Task TaskHandlersShouldNotBlockNestedInteractionsBeforeReturningTask()
178+
{
179+
var parent = new Interaction<Unit, Unit>();
180+
var nested = new Interaction<Unit, string>();
181+
var nestedHandledBeforeParentReturned = false;
182+
string? nestedOutput = null;
183+
184+
nested.RegisterHandler(context => context.SetOutput("nested"));
185+
186+
parent.RegisterHandler(context =>
187+
{
188+
using var nestedSubscription = nested.Handle(Unit.Default).Subscribe(output => nestedOutput = output);
189+
nestedHandledBeforeParentReturned = nestedOutput == "nested";
190+
191+
context.SetOutput(Unit.Default);
192+
return Task.CompletedTask;
193+
});
194+
195+
await parent.Handle(Unit.Default);
196+
197+
await Assert.That(nestedHandledBeforeParentReturned).IsTrue();
198+
}
199+
200+
/// <summary>
201+
/// Tests that observable handlers release the current scheduler before invoking user code.
202+
/// </summary>
203+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
204+
[Test]
205+
public async Task ObservableHandlersShouldNotBlockNestedInteractionsBeforeReturningObservable()
206+
{
207+
var parent = new Interaction<Unit, Unit>();
208+
var nested = new Interaction<Unit, string>();
209+
var nestedHandledBeforeParentReturned = false;
210+
string? nestedOutput = null;
211+
212+
nested.RegisterHandler(context => context.SetOutput("nested"));
213+
214+
parent.RegisterHandler(context =>
215+
{
216+
using var nestedSubscription = nested.Handle(Unit.Default).Subscribe(output => nestedOutput = output);
217+
nestedHandledBeforeParentReturned = nestedOutput == "nested";
218+
219+
context.SetOutput(Unit.Default);
220+
return Observables.Unit;
221+
});
222+
223+
await parent.Handle(Unit.Default);
224+
225+
await Assert.That(nestedHandledBeforeParentReturned).IsTrue();
226+
}
227+
150228
/// <summary>
151229
/// Tests that handlers can opt not to handle the interaction.
152230
/// </summary>
@@ -174,21 +252,21 @@ public async Task HandlersCanOptNotToHandleTheInteraction()
174252
using (handler1C)
175253
using (Assert.Multiple())
176254
{
177-
await Assert.That(interaction.Handle(false).FirstAsync().Wait()).IsEqualTo("C");
178-
await Assert.That(interaction.Handle(true).FirstAsync().Wait()).IsEqualTo("C");
255+
await Assert.That(await interaction.Handle(false)).IsEqualTo("C");
256+
await Assert.That(await interaction.Handle(true)).IsEqualTo("C");
179257
}
180258

181259
using (Assert.Multiple())
182260
{
183-
await Assert.That(interaction.Handle(false).FirstAsync().Wait()).IsEqualTo("A");
184-
await Assert.That(interaction.Handle(true).FirstAsync().Wait()).IsEqualTo("B");
261+
await Assert.That(await interaction.Handle(false)).IsEqualTo("A");
262+
await Assert.That(await interaction.Handle(true)).IsEqualTo("B");
185263
}
186264
}
187265

188266
using (Assert.Multiple())
189267
{
190-
await Assert.That(interaction.Handle(false).FirstAsync().Wait()).IsEqualTo("A");
191-
await Assert.That(interaction.Handle(true).FirstAsync().Wait()).IsEqualTo("A");
268+
await Assert.That(await interaction.Handle(false)).IsEqualTo("A");
269+
await Assert.That(await interaction.Handle(true)).IsEqualTo("A");
192270
}
193271
}
194272
}
@@ -207,7 +285,7 @@ public async Task HandlersReturningObservablesCanReturnAnyKindOfObservable()
207285
.Return(42)
208286
.Do(_ => x.SetOutput("result")));
209287

210-
var result = interaction.Handle(Unit.Default).FirstAsync().Wait();
288+
var result = await interaction.Handle(Unit.Default);
211289
await Assert.That(result).IsEqualTo("result");
212290
}
213291

@@ -222,19 +300,19 @@ public async Task NestedHandlersAreExecutedInReverseOrderOfSubscription()
222300

223301
using (interaction.RegisterHandler(static x => x.SetOutput("A")))
224302
{
225-
await Assert.That(interaction.Handle(Unit.Default).FirstAsync().Wait()).IsEqualTo("A");
303+
await Assert.That(await interaction.Handle(Unit.Default)).IsEqualTo("A");
226304
using (interaction.RegisterHandler(static x => x.SetOutput("B")))
227305
{
228-
await Assert.That(interaction.Handle(Unit.Default).FirstAsync().Wait()).IsEqualTo("B");
306+
await Assert.That(await interaction.Handle(Unit.Default)).IsEqualTo("B");
229307
using (interaction.RegisterHandler(static x => x.SetOutput("C")))
230308
{
231-
await Assert.That(interaction.Handle(Unit.Default).FirstAsync().Wait()).IsEqualTo("C");
309+
await Assert.That(await interaction.Handle(Unit.Default)).IsEqualTo("C");
232310
}
233311

234-
await Assert.That(interaction.Handle(Unit.Default).FirstAsync().Wait()).IsEqualTo("B");
312+
await Assert.That(await interaction.Handle(Unit.Default)).IsEqualTo("B");
235313
}
236314

237-
await Assert.That(interaction.Handle(Unit.Default).FirstAsync().Wait()).IsEqualTo("A");
315+
await Assert.That(await interaction.Handle(Unit.Default)).IsEqualTo("A");
238316
}
239317
}
240318

@@ -261,21 +339,21 @@ public void RegisterNullHandlerShouldCauseException()
261339
public async Task UnhandledInteractionsShouldCauseException()
262340
{
263341
var interaction = new Interaction<string, Unit>();
264-
var ex = Assert.Throws<UnhandledInteractionException<string, Unit>>(() =>
265-
interaction.Handle("foo").FirstAsync().Wait());
342+
var ex = await Assert.ThrowsAsync<UnhandledInteractionException<string, Unit>>(() =>
343+
interaction.Handle("foo").ToTask());
266344
using (Assert.Multiple())
267345
{
268-
await Assert.That(ex.Interaction).IsSameReferenceAs(interaction);
346+
await Assert.That(ex!.Interaction).IsSameReferenceAs(interaction);
269347
await Assert.That(ex.Input).IsEqualTo("foo");
270348
}
271349

272350
interaction.RegisterHandler(_ => { });
273351
interaction.RegisterHandler(_ => { });
274-
ex = Assert.Throws<UnhandledInteractionException<string, Unit>>(() =>
275-
interaction.Handle("bar").FirstAsync().Wait());
352+
ex = await Assert.ThrowsAsync<UnhandledInteractionException<string, Unit>>(() =>
353+
interaction.Handle("bar").ToTask());
276354
using (Assert.Multiple())
277355
{
278-
await Assert.That(ex.Interaction).IsSameReferenceAs(interaction);
356+
await Assert.That(ex!.Interaction).IsSameReferenceAs(interaction);
279357
await Assert.That(ex.Input).IsEqualTo("bar");
280358
}
281359
}

0 commit comments

Comments
 (0)