Use async ChangeToken.OnChange overload in KeyPerFileConfigurationProvider#67732
Open
svick wants to merge 3 commits into
Open
Use async ChangeToken.OnChange overload in KeyPerFileConfigurationProvider#67732svick wants to merge 3 commits into
svick wants to merge 3 commits into
Conversation
…vider Replace the blocking Thread.Sleep in the reload callback with an awaited Task.Delay, using the async Func<Task> overload of ChangeToken.OnChange added in dotnet/runtime#69099. This avoids blocking the file-watcher callback thread for the reload delay. Two reload tests that asserted synchronously after triggering a change relied on the previous blocking behavior; they now await the config reload token before asserting, matching the existing RaiseChangeEventWhenReloadOnChangeIsTrue test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 059bf1b8-31e2-4608-a038-210880f90d06
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the KeyPerFile configuration provider to use the async (Func<Task>) ChangeToken.OnChange overload so reload debouncing (ReloadDelay) no longer blocks the file-watcher callback thread, and adjusts related tests to await reload completion.
Changes:
- Switch
KeyPerFileConfigurationProviderreload callback fromThread.Sleeptoawait Task.Delay(...)before reloading. - Add a try/catch around
Load(reload: true)in the change callback to avoid exceptions escaping the async callback. - Update reload-related tests to be async, await the reload token, and apply timeouts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Configuration.KeyPerFile/src/KeyPerFileConfigurationProvider.cs | Uses async OnChange callback and Task.Delay for non-blocking debounce before reload. |
| src/Configuration.KeyPerFile/test/KeyPerFileTests.cs | Converts relevant tests to async and waits for the reload token before asserting. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uses the new async (
Func<Task>) overload ofChangeToken.OnChange(see dotnet/runtime#69099) inKeyPerFileConfigurationProvider.The reload callback previously blocked the file-watcher callback thread for the whole reload delay via
Thread.Sleep(Source.ReloadDelay)(default 250 ms). It nowawaitsTask.Delay(...)instead, so no thread is blocked while waiting out the debounce delay before reloading:The same change for
FileConfigurationProvideris at dotnet/runtime#130492.Test changes
Two reload tests asserted the reloaded configuration synchronously immediately after triggering a change, which only worked because the old
Thread.Sleepran the reload inline on the change-notification thread. With the non-blocking async reload, the reload now completes on a thread-pool thread, so these tests now await the config reload token before asserting — matching the existingRaiseChangeEventWhenReloadOnChangeIsTruetest:ReloadConfigWhenReloadOnChangeIsTrueNoFilesReloadWhenAddedFilesNote
This PR description was generated by GitHub Copilot.