Skip to content

Commit bd27fbc

Browse files
authored
Fix suspension persistence to materialize app state before shutdown save (#4353)
<!-- 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 #4352 ## What is the new behavior? ### Summary - Fixes `ReactiveUI` suspension persistence so `ShouldPersistState` now forces app state materialization before `SaveState` runs, covering first-launch shutdown paths and missed launch-signal ordering. - Adds regression tests for typed and untyped hosts that verify created state, loaded state, and persist-token disposal, including the repro ordering from [issue #4352](#4352). - Reviewer context: this addresses the shutdown/disposal failure reproduced in [JosiahDanger/SuspensionDisposalBug](https://github.com/JosiahDanger/SuspensionDisposalBug). ### Testing - `dotnet build src\tests\ReactiveUI.Tests\ReactiveUI.Tests.csproj --no-restore` - Ran focused TUnit executables for `SuspensionHostExtensionsTests` and `SuspensionHostExtensionsAotTests` on `net8.0`, `net9.0`, and `net10.0` - Ran a broader `SuspensionHost*` sweep on `net10.0`; all passed ## 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 303d184 commit bd27fbc

5 files changed

Lines changed: 364 additions & 8 deletions

File tree

src/ReactiveUI/Interactions/Interaction.cs

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -170,13 +170,10 @@ protected Func<IInteractionContext<TInput, TOutput>, IObservable<Unit>>[] GetHan
170170
protected virtual IOutputContext<TInput, TOutput> GenerateContext(TInput input) => new InteractionContext<TInput, TOutput>(input);
171171

172172
/// <summary>
173-
/// Yields once so asynchronous handlers are not invoked inside the current scheduler trampoline.
173+
/// Yields through the default task scheduler so asynchronous handlers are not invoked inside the current scheduler trampoline.
174174
/// </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-
}
175+
/// <returns>A task that completes after the current scheduler trampoline has yielded.</returns>
176+
private static Task YieldToCurrentContext() => Task.Run(static () => { });
180177

181178
/// <summary>
182179
/// Registers a normalized interaction handler.

src/ReactiveUI/Suspension/SuspensionHostExtensions.cs

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,9 @@ public static IDisposable SetupDefaultSuspendResume(this ISuspensionHost item, I
200200
.Subscribe(_ => item.Log().Info("Invalidated app state")));
201201

202202
ret.Add(item.ShouldPersistState
203-
.SelectMany(x => _suspensionDriver.SaveState(item.AppState!).Finally(x.Dispose))
203+
.SelectMany(x => EnsureLoadAppStateOnce(item, _suspensionDriver)
204+
.SelectMany(_ => _suspensionDriver!.SaveState(item.AppState!))
205+
.Finally(x.Dispose))
204206
.LoggedCatch(item, Observables.Unit, "Tried to persist app state")
205207
.Subscribe(_ => item.Log().Info("Persisted application state")));
206208

@@ -246,7 +248,9 @@ public static IDisposable SetupDefaultSuspendResume<TAppState>(this ISuspensionH
246248
.Subscribe(_ => item.Log().Info("Invalidated app state")));
247249

248250
ret.Add(item.ShouldPersistState
249-
.SelectMany(x => _suspensionDriver.SaveState(item.AppStateValue!, typeInfo).Finally(x.Dispose))
251+
.SelectMany(x => EnsureLoadAppStateOnce(item, _suspensionDriver, typeInfo)
252+
.SelectMany(_ => _suspensionDriver!.SaveState(item.AppStateValue!, typeInfo))
253+
.Finally(x.Dispose))
250254
.LoggedCatch(item, Observables.Unit, "Tried to persist app state")
251255
.Subscribe(_ => item.Log().Info("Persisted application state")));
252256

@@ -301,6 +305,25 @@ private static IObservable<Unit> EnsureLoadAppState(this ISuspensionHost item, I
301305
return Observable.Return(Unit.Default);
302306
}
303307

308+
/// <summary>
309+
/// Runs the pending one-time untyped app-state load, or materializes state directly if the pending loader has already been consumed.
310+
/// </summary>
311+
/// <param name="item">The suspension host.</param>
312+
/// <param name="driver">The suspension driver.</param>
313+
/// <returns>A completed observable.</returns>
314+
[RequiresUnreferencedCode(
315+
"This overload may invoke ISuspensionDriver.LoadState(), which is commonly reflection-based. " +
316+
"Prefer EnsureLoadAppStateOnce<TAppState>(ISuspensionHost<TAppState>, ISuspensionDriver?, JsonTypeInfo<TAppState>) for trimming/AOT scenarios.")]
317+
[RequiresDynamicCode(
318+
"This overload may invoke ISuspensionDriver.LoadState(), which is commonly reflection-based. " +
319+
"Prefer EnsureLoadAppStateOnce<TAppState>(ISuspensionHost<TAppState>, ISuspensionDriver?, JsonTypeInfo<TAppState>) for trimming/AOT scenarios.")]
320+
private static IObservable<Unit> EnsureLoadAppStateOnce(ISuspensionHost item, ISuspensionDriver? driver)
321+
{
322+
var ensureLoadAppState = Interlocked.Exchange(ref _ensureLoadAppStateFunc, null);
323+
324+
return ensureLoadAppState?.Invoke() ?? item.EnsureLoadAppState(driver);
325+
}
326+
304327
/// <summary>
305328
/// Ensures a one-time typed app state load from storage using source-generated JSON metadata (trimming/AOT friendly).
306329
/// </summary>
@@ -341,4 +364,20 @@ private static IObservable<Unit> EnsureLoadAppState<TAppState>(this ISuspensionH
341364

342365
return Observable.Return(Unit.Default);
343366
}
367+
368+
/// <summary>
369+
/// Runs the pending one-time typed app-state load, or materializes state directly if the pending loader has already been consumed.
370+
/// </summary>
371+
/// <typeparam name="TAppState">The application state type.</typeparam>
372+
/// <param name="item">The typed suspension host.</param>
373+
/// <param name="driver">The suspension driver.</param>
374+
/// <param name="typeInfo">Source-generated metadata for <typeparamref name="TAppState"/>.</param>
375+
/// <returns>A completed observable.</returns>
376+
private static IObservable<Unit> EnsureLoadAppStateOnce<TAppState>(ISuspensionHost<TAppState> item, ISuspensionDriver? driver, JsonTypeInfo<TAppState> typeInfo)
377+
where TAppState : class
378+
{
379+
var ensureLoadAppState = Interlocked.Exchange(ref _ensureLoadAppStateFunc, null);
380+
381+
return ensureLoadAppState?.Invoke() ?? item.EnsureLoadAppState(driver, typeInfo);
382+
}
344383
}

src/tests/ReactiveUI.Tests/InteractionsTest.cs

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,130 @@ public async Task ObservableHandlersShouldNotBlockNestedInteractionsBeforeReturn
225225
await Assert.That(nestedHandledBeforeParentReturned).IsTrue();
226226
}
227227

228+
/// <summary>
229+
/// Tests that task handler exceptions are propagated to the interaction observer.
230+
/// </summary>
231+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
232+
[Test]
233+
public async Task TaskHandlerExceptionsShouldPropagate()
234+
{
235+
var interaction = new Interaction<Unit, string>();
236+
var expected = new InvalidOperationException("task handler failed");
237+
238+
interaction.RegisterHandler(_ => Task.FromException(expected));
239+
240+
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => interaction.Handle(Unit.Default).ToTask());
241+
await Assert.That(ex).IsSameReferenceAs(expected);
242+
}
243+
244+
/// <summary>
245+
/// Tests that task handlers can complete without handling the interaction.
246+
/// </summary>
247+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
248+
[Test]
249+
public async Task TaskHandlersThatCompleteWithoutOutputShouldFallThroughToNextHandler()
250+
{
251+
var interaction = new Interaction<Unit, string>();
252+
253+
interaction.RegisterHandler(static context => context.SetOutput("fallback"));
254+
interaction.RegisterHandler(static _ => Task.CompletedTask);
255+
256+
var result = await interaction.Handle(Unit.Default);
257+
258+
await Assert.That(result).IsEqualTo("fallback");
259+
}
260+
261+
/// <summary>
262+
/// Tests that task handlers which do not set output still surface the unhandled interaction.
263+
/// </summary>
264+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
265+
[Test]
266+
public async Task TaskHandlersThatCompleteWithoutOutputShouldCauseUnhandledInteractionException()
267+
{
268+
var interaction = new Interaction<string, Unit>();
269+
270+
interaction.RegisterHandler(static _ => Task.CompletedTask);
271+
272+
var ex = await Assert.ThrowsAsync<UnhandledInteractionException<string, Unit>>(() =>
273+
interaction.Handle("task").ToTask());
274+
275+
using (Assert.Multiple())
276+
{
277+
await Assert.That(ex!.Interaction).IsSameReferenceAs(interaction);
278+
await Assert.That(ex.Input).IsEqualTo("task");
279+
}
280+
}
281+
282+
/// <summary>
283+
/// Tests that exceptions thrown while creating observable handlers are propagated.
284+
/// </summary>
285+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
286+
[Test]
287+
public async Task ObservableHandlerFactoryExceptionsShouldPropagate()
288+
{
289+
var interaction = new Interaction<Unit, string>();
290+
var expected = new InvalidOperationException("observable handler factory failed");
291+
292+
interaction.RegisterHandler<Unit>(_ => throw expected);
293+
294+
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => interaction.Handle(Unit.Default).ToTask());
295+
await Assert.That(ex).IsSameReferenceAs(expected);
296+
}
297+
298+
/// <summary>
299+
/// Tests that errors produced by observable handlers are propagated.
300+
/// </summary>
301+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
302+
[Test]
303+
public async Task ObservableHandlerErrorsShouldPropagate()
304+
{
305+
var interaction = new Interaction<Unit, string>();
306+
var expected = new InvalidOperationException("observable handler failed");
307+
308+
interaction.RegisterHandler(_ => Observable.Throw<Unit>(expected));
309+
310+
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() => interaction.Handle(Unit.Default).ToTask());
311+
await Assert.That(ex).IsSameReferenceAs(expected);
312+
}
313+
314+
/// <summary>
315+
/// Tests that observable handlers can complete without handling the interaction.
316+
/// </summary>
317+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
318+
[Test]
319+
public async Task ObservableHandlersThatCompleteWithoutOutputShouldFallThroughToNextHandler()
320+
{
321+
var interaction = new Interaction<Unit, string>();
322+
323+
interaction.RegisterHandler(static context => context.SetOutput("fallback"));
324+
interaction.RegisterHandler(static _ => Observable.Empty<Unit>());
325+
326+
var result = await interaction.Handle(Unit.Default);
327+
328+
await Assert.That(result).IsEqualTo("fallback");
329+
}
330+
331+
/// <summary>
332+
/// Tests that observable handlers which do not set output still surface the unhandled interaction.
333+
/// </summary>
334+
/// <returns>A <see cref="Task" /> representing the asynchronous operation.</returns>
335+
[Test]
336+
public async Task ObservableHandlersThatCompleteWithoutOutputShouldCauseUnhandledInteractionException()
337+
{
338+
var interaction = new Interaction<string, Unit>();
339+
340+
interaction.RegisterHandler(static _ => Observable.Empty<Unit>());
341+
342+
var ex = await Assert.ThrowsAsync<UnhandledInteractionException<string, Unit>>(() =>
343+
interaction.Handle("observable").ToTask());
344+
345+
using (Assert.Multiple())
346+
{
347+
await Assert.That(ex!.Interaction).IsSameReferenceAs(interaction);
348+
await Assert.That(ex.Input).IsEqualTo("observable");
349+
}
350+
}
351+
228352
/// <summary>
229353
/// Tests that handlers can opt not to handle the interaction.
230354
/// </summary>

src/tests/ReactiveUI.Tests/Suspension/SuspensionHostExtensionsAotTests.cs

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,135 @@ public async Task SetupDefaultSuspendResume_Typed_ShouldPersistState_CallsDriver
254254
await Assert.That(driver.LastSavedState).IsSameReferenceAs(appState);
255255
}
256256

257+
[Test]
258+
public async Task SetupDefaultSuspendResume_Typed_ShouldPersistCreatedState_WhenNoPersistedStateAndPersistOccursBeforeGetAppState()
259+
{
260+
var createdState = new TestAppState { Value = 321 };
261+
var createNewAppStateCallCount = 0;
262+
var persistTokenDisposed = false;
263+
using var host = new SuspensionHost<TestAppState>
264+
{
265+
CreateNewAppStateTyped = () =>
266+
{
267+
createNewAppStateCallCount++;
268+
return createdState;
269+
},
270+
IsLaunchingNew = Observable.Never<Unit>(),
271+
IsResuming = Observable.Never<Unit>(),
272+
ShouldInvalidateState = Observable.Never<Unit>()
273+
};
274+
275+
var driver = new TestSuspensionDriver<TestAppState>();
276+
var persistSubject = new Subject<IDisposable>();
277+
host.ShouldPersistState = persistSubject.ObserveOn(ImmediateScheduler.Instance);
278+
279+
using var disposable = host.SetupDefaultSuspendResume(TestAppStateContext.Default.TestAppState, driver);
280+
var persistToken = Disposable.Create(() => persistTokenDisposed = true);
281+
282+
persistSubject.OnNext(persistToken);
283+
284+
await Assert.That(driver.LoadStateCallCount).IsEqualTo(1);
285+
await Assert.That(createNewAppStateCallCount).IsEqualTo(1);
286+
await Assert.That(host.AppStateValue).IsSameReferenceAs(createdState);
287+
await Assert.That(driver.SaveStateCallCount).IsEqualTo(1);
288+
await Assert.That(driver.LastSavedState).IsSameReferenceAs(createdState);
289+
await Assert.That(persistTokenDisposed).IsTrue();
290+
}
291+
292+
[Test]
293+
public async Task SetupDefaultSuspendResume_Typed_ShouldPersistCreatedState_WhenLaunchSignalWasRaisedBeforeSetup()
294+
{
295+
var createdState = new TestAppState { Value = 654 };
296+
var createNewAppStateCallCount = 0;
297+
using var host = new SuspensionHost<TestAppState>
298+
{
299+
CreateNewAppStateTyped = () =>
300+
{
301+
createNewAppStateCallCount++;
302+
return createdState;
303+
},
304+
ShouldInvalidateState = Observable.Never<Unit>()
305+
};
306+
307+
var launchSubject = new Subject<Unit>();
308+
var resumeSubject = new Subject<Unit>();
309+
var persistSubject = new Subject<IDisposable>();
310+
host.IsLaunchingNew = launchSubject.ObserveOn(ImmediateScheduler.Instance);
311+
host.IsResuming = resumeSubject.ObserveOn(ImmediateScheduler.Instance);
312+
host.ShouldPersistState = persistSubject.ObserveOn(ImmediateScheduler.Instance);
313+
314+
launchSubject.OnNext(Unit.Default);
315+
316+
var driver = new TestSuspensionDriver<TestAppState>();
317+
using var disposable = host.SetupDefaultSuspendResume(TestAppStateContext.Default.TestAppState, driver);
318+
319+
persistSubject.OnNext(Disposable.Empty);
320+
321+
await Assert.That(driver.LoadStateCallCount).IsEqualTo(1);
322+
await Assert.That(createNewAppStateCallCount).IsEqualTo(1);
323+
await Assert.That(host.AppStateValue).IsSameReferenceAs(createdState);
324+
await Assert.That(driver.SaveStateCallCount).IsEqualTo(1);
325+
await Assert.That(driver.LastSavedState).IsSameReferenceAs(createdState);
326+
}
327+
328+
[Test]
329+
public async Task SetupDefaultSuspendResume_Typed_ShouldPersistLoadedState_WhenPersistOccursBeforeGetAppState()
330+
{
331+
var loadedState = new TestAppState { Value = 987 };
332+
var createNewAppStateCallCount = 0;
333+
using var host = new SuspensionHost<TestAppState>
334+
{
335+
CreateNewAppStateTyped = () =>
336+
{
337+
createNewAppStateCallCount++;
338+
return new TestAppState();
339+
},
340+
IsLaunchingNew = Observable.Never<Unit>(),
341+
IsResuming = Observable.Never<Unit>(),
342+
ShouldInvalidateState = Observable.Never<Unit>()
343+
};
344+
345+
var driver = new TestSuspensionDriver<TestAppState> { StateToLoad = loadedState };
346+
var persistSubject = new Subject<IDisposable>();
347+
host.ShouldPersistState = persistSubject.ObserveOn(ImmediateScheduler.Instance);
348+
349+
using var disposable = host.SetupDefaultSuspendResume(TestAppStateContext.Default.TestAppState, driver);
350+
351+
persistSubject.OnNext(Disposable.Empty);
352+
353+
await Assert.That(driver.LoadStateCallCount).IsEqualTo(1);
354+
await Assert.That(createNewAppStateCallCount).IsEqualTo(0);
355+
await Assert.That(host.AppStateValue).IsSameReferenceAs(loadedState);
356+
await Assert.That(driver.SaveStateCallCount).IsEqualTo(1);
357+
await Assert.That(driver.LastSavedState).IsSameReferenceAs(loadedState);
358+
}
359+
360+
[Test]
361+
public async Task SetupDefaultSuspendResume_Typed_ShouldDisposePersistTokenAfterSave()
362+
{
363+
var appState = new TestAppState { Value = 111 };
364+
var persistTokenDisposed = false;
365+
using var host = new SuspensionHost<TestAppState>
366+
{
367+
AppStateValue = appState,
368+
IsLaunchingNew = Observable.Never<Unit>(),
369+
IsResuming = Observable.Never<Unit>(),
370+
ShouldInvalidateState = Observable.Never<Unit>()
371+
};
372+
373+
var driver = new TestSuspensionDriver<TestAppState>();
374+
var persistSubject = new Subject<IDisposable>();
375+
host.ShouldPersistState = persistSubject.ObserveOn(ImmediateScheduler.Instance);
376+
377+
using var disposable = host.SetupDefaultSuspendResume(TestAppStateContext.Default.TestAppState, driver);
378+
var persistToken = Disposable.Create(() => persistTokenDisposed = true);
379+
380+
persistSubject.OnNext(persistToken);
381+
382+
await Assert.That(driver.SaveStateCallCount).IsEqualTo(1);
383+
await Assert.That(persistTokenDisposed).IsTrue();
384+
}
385+
257386
[Test]
258387
public async Task SetupDefaultSuspendResume_Typed_ShouldInvalidateState_CallsDriverInvalidateState()
259388
{

0 commit comments

Comments
 (0)