Implement OnChange mechanism using state machine approach#125065
Implement OnChange mechanism using state machine approach#125065rosebyte wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-extensions-primitives |
There was a problem hiding this comment.
Pull request overview
This PR updates Microsoft.Extensions.Primitives’ ChangeToken.OnChange implementation to avoid unbounded recursion (and StackOverflowException) when a producer repeatedly returns already-changed tokens, by switching to an iterative/state-machine style callback processor and adding a guard to fail fast after excessive consecutive synchronous changes.
Changes:
- Replaces the recursive
OnChangere-registration flow with an iterative callback processing loop guarded by interlocked/volatile state. - Adds a max-consecutive-synchronous-changes threshold (4096) that throws
InvalidOperationExceptionto prevent tight-loop CPU churn. - Adds unit tests covering the non-recursive processing path and the new failure behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Primitives/src/ChangeToken.cs | Implements the new state-machine callback processing loop and introduces the consecutive-changes guard/exception. |
| src/libraries/Microsoft.Extensions.Primitives/tests/ChangeTokenTest.cs | Adds tests validating iterative processing and the new guard exception behavior. |
| private sealed class ChangeTokenRegistration<TState> : IDisposable | ||
| { | ||
| private const int MaxConsecutiveSynchronousChanges = 4096; | ||
| private const string ConsecutiveChangesErrorMessage = "The change token producer returned an already changed token too many consecutive times. Ensure the producer eventually returns a fresh, non-signaled token instance."; |
There was a problem hiding this comment.
New InvalidOperationException message is hard-coded as a const string. In this library, exception messages are typically sourced from SR/Strings.resx via ThrowHelper (for consistency and localization). Consider adding a new SR resource + ExceptionResource entry and throwing via ThrowHelper instead of embedding the full message here.
| private const string ConsecutiveChangesErrorMessage = "The change token producer returned an already changed token too many consecutive times. Ensure the producer eventually returns a fresh, non-signaled token instance."; | |
| private static readonly string ConsecutiveChangesErrorMessage = "The change token producer returned an already changed token too many consecutive times. Ensure the producer eventually returns a fresh, non-signaled token instance."; |
| if (RegisterChangeTokenCallback(token)) | ||
| { | ||
| if (++consecutiveSynchronousChanges >= MaxConsecutiveSynchronousChanges) | ||
| { |
There was a problem hiding this comment.
When MaxConsecutiveSynchronousChanges is exceeded, an InvalidOperationException is thrown without disposing the currently-registered callback subscription. If this happens during the initial OnChange() call (i.e., the synchronous-change loop starts during construction), the caller never receives an IDisposable to dispose and the existing registration can remain rooted. Consider disposing the current registration (e.g., calling Dispose() on the registration object / exchanging _disposable to the disposed sentinel) before throwing to avoid leaving callbacks registered/leaking state.
| { | |
| { | |
| // Ensure the current registration is disposed before throwing to avoid leaking callbacks/state. | |
| Dispose(); |
Fixes #60183
If ChangeToken producer keeps returning changed tokens, we quickly get to StackOverflowException with the current recursive implementation, so this PR proposes to use state machine approach instead, however, if the producer keeps returning changed tokens, we don't throw and keep repeating the process in a tight loop with potential CPU churn, therefore, we added a guard that counts consecutive changed tokens returned and throws if they reach 4096 consequtive iterations (this number should be hight enough to avoid false positives and low enough to fail fast).