Added async overload of ChangeToken.OnChange#129624
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends Microsoft.Extensions.Primitives.ChangeToken with Task-returning OnChange overloads so consumers can run asynchronous work and delay re-registration until that work completes, and it refactors the internal registration logic to share more between sync and async paths.
Changes:
- Add
ChangeToken.OnChange(Func<IChangeToken?>, Func<Task>)andChangeToken.OnChange<TState>(Func<IChangeToken?>, Func<TState, Task>, TState)public overloads. - Refactor registration implementation into shared base + sync/async derived registrations.
- Update/add unit tests covering async behavior, disposal while in-flight, and overload binding.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Primitives/src/ChangeToken.cs | Adds async overloads and refactors internal registration to support deferred re-registration after Task completion. |
| src/libraries/Microsoft.Extensions.Primitives/ref/Microsoft.Extensions.Primitives.cs | Updates ref assembly to include the new public overloads. |
| src/libraries/Microsoft.Extensions.Primitives/tests/ChangeTokenTest.cs | Adjusts existing tests for overload binding and adds new tests for async/disposal/coalescing semantics. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/libraries/Microsoft.Extensions.Primitives/src/ChangeToken.cs:165
- SetDisposable assumes the incoming disposable is non-null and calls Dispose() on it in the disposed-sentinel race. However, IChangeToken implementations (including this test suite's TestChangeToken) can return null from RegisterChangeCallback, which would make those Dispose() calls throw NullReferenceException under disposal races. Make SetDisposable accept IDisposable? and null-check before disposing so this stays race-safe even when the registration is null.
private void SetDisposable(IDisposable disposable)
{
// We don't want to transition from _disposedSentinel => anything since it's terminal
// but we want to allow going from previously assigned disposable, to another
// disposable.
IDisposable? current = Volatile.Read(ref _disposable);
// If Dispose was called, then immediately dispose the disposable
if (current == _disposedSentinel)
{
disposable.Dispose();
return;
}
// Otherwise, try to update the disposable
IDisposable? previous = Interlocked.CompareExchange(ref _disposable, disposable, current);
if (previous == _disposedSentinel)
{
// The subscription was disposed so we dispose immediately and return
disposable.Dispose();
}
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
rosebyte
left a comment
There was a problem hiding this comment.
Only a couple of non-blocking comments.
| { | ||
| // The consumer is invoked synchronously here, so that synchronous exceptions from it are propagated | ||
| // to the code that triggers the change token, just like the sync overload does. | ||
| Task consumerTask = _changeTokenConsumer(State) ?? throw new InvalidOperationException("The task returned by changeTokenConsumer must not be null."); |
There was a problem hiding this comment.
We have a method for IOE too in ThrowHelper IIRC.
| protected void RegisterChangeTokenCallback(IChangeToken? token) | ||
| { | ||
| if (token is null) | ||
| { | ||
| return; | ||
| } | ||
| IDisposable registraton = token.RegisterChangeCallback(s => ((ChangeTokenRegistration<TState>?)s)!.OnChangeTokenFired(), this); | ||
| IDisposable? registration = token.RegisterChangeCallback(static s => ((ChangeTokenRegistration<TState>?)s)!.OnChangeTokenFired(), this); |
Fixes #69099.
Adds async (
Func<Task>) overloads ofChangeToken.OnChange, so callers can run asynchronous logic when a change token fires without resorting toasync voidor blocking:These match the shape approved in #69099.
Behavior
The existing
ChangeTokenRegistration<TState>is refactored into an abstract base class holding the registration/disposal state machine, withSyncChangeTokenRegistrationandAsyncChangeTokenRegistrationsubclasses. The synchronous overloads behave exactly as before.For the async overloads:
Taskcompletes. Changes that occur while the consumer's task is still in flight are coalesced into a single subsequent invocation.TaskScheduler.UnobservedTaskException). This is documented in the API remarks.Breaking change
This is a source (not binary) breaking change, as called out in the proposal's Risk section: existing code that passes an
asynclambda toOnChangepreviously bound to theActionoverload (compiling toasync void/ fire-and-forget). With these overloads present, such call sites now bind to theFunc<Task>overload and re-registration is deferred until the returned task completes. The new behavior is generally more correct. A throwing statement lambda can also become ambiguous betweenActionandFunc<Task>(CS0121); the two pre-existing tests affected were updated with explicit(Action)casts.Tests
[ConditionalTheory(... IsMultithreadingSupported)].Note
This PR description was generated by GitHub Copilot.