FileConfigurationProvider: Handle and forward IO exceptions to OnLoadException - #126093
#126093FileConfigurationProvider: Handle and forward IO exceptions to OnLoadException#126093mrek-msft merged 35 commits intodotnet:maindotnet/runtime:mainfrom mrek-msft:dev/mrek/mefs-filecfgprov-reload-lockedmrek-msft/runtime:dev/mrek/mefs-filecfgprov-reload-lockedCopy head branch name to clipboard
Conversation
There was a problem hiding this comment.
Pull request overview
Unifies FileConfigurationProvider failure handling so IO failures during file open are handled the same way as parsing failures with respect to OnLoadException (including on reload-on-change), preventing unobserved/unhandled exceptions like the one reported in #113964.
Changes:
- Moves stream opening (
OpenRead) inside the exception-handling region so IO errors are routed throughHandleException/OnLoadException. - Adds an inner
try/catchto wrap exceptions thrown byLoad(stream)in anInvalidDataExceptionwith the standard “Failed to load…” message. - Adjusts the outer catch to forward the exception through
HandleException(enabling ignore behavior on reload).
tarekgh
left a comment
There was a problem hiding this comment.
Left minor comments I hope you address them. LGTM, otherwise.
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Added When you commit this breaking change:
Tagging @dotnet/compat for awareness of the breaking change. |
|
/backport to release/11.0-preview7 |
|
Started backporting to |
|
@mrek-msft backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Forward IO exceptions to OnLoadException
Applying: Removed duplicate Data dict recreation
Applying: Add tests
Applying: Throw alaways InvalidDataException and clear Data only on data error.
Applying: Improve test attributes assignment.
Applying: Fix typos
Applying: Change exception back to rpopagating IOException directly and improve file path formatting logic.
Applying: Fix missing PhysicalPath in error string shown file name selection
Applying: Improve comment.
Applying: Remove duplicate test.
Applying: Add missing ActiveIssue attribute to possibly flaky test.
Applying: Add comment recommended by Copilot
Applying: Check for type of observed exception.
Applying: Dispose IConfigurationRoot
Applying: Update comment and its location based on Copilot hints
Applying: Do not fire OnReload if no Data change happened.
Applying: Add checking config value after faulty reload
Applying: Move and improve comment.
Applying: Fix double empty line
Applying: Add docs mentioning consequences of not registering OnLoadException.
Applying: Improve test coverage to handle Copilot discovered edge cases
Applying: exclude IOException from handling on "parse error" path.
Applying: Improve comment
Applying: Improve comment
Applying: Fix comment grammar
Applying: Improve FileLoadExceptionContext.Exception comment as requested in PR review
Applying: Removal of nested try..catch and handling deletion explicitly
Applying: Handle file not found in the same way on both paths.
Applying: Fix whitespace formatting
Applying: Copilot improved comment
Applying: Removed old comment
Applying: Fix typo in comment
Applying: Propagate DirectoryNotFoundException properly.
error: sha1 information is lacking or useless (src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0033 Propagate DirectoryNotFoundException properly.
Error: The process '/usr/bin/git' failed with exit code 128 |
… IO exceptions to OnLoadException (#131427) Backport of #126093 to release/11.0-preview7 ## Customer Impact Long standing issue reported by customer - [X] Customer reported - [ ] Found internally ## Regression Long standing issue. - [ ] Yes - [X] No ## Testing 6 tests were added. ## Risk Low. It is backport to Preview 7 version after branch were snapped but before code complete. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Current behavior of FileConfigurationProvider on various types of failures at various times:
OnLoadExceptioncallback is calledAddJsonFilethrowsOnLoadExceptioncallback is calledprogram continues
no reload happen
AddJsonFilethrowsprogram continues
no reload happen
possibly raises
UnobservedTaskExceptionlaterThis PR unifies it in a way that both types of failure behave same as data parsing failure.
This brings some behavioral changes
OnLoadExceptioncallback is newly triggered if IO error happen on both types of events (first load, reload)UnobservedTaskExceptioncan no longer be observed in scenario when user registersOnLoadExceptioncallback. It can however happen when no callback is registered, so XML comment onOnLoadExceptionwas updated.OnLoadExceptioncallbacks now receives not only InvalidDataException, but also IOException (or any other exception which (possibly custom) implementation of IFileProvider can throw). If user cast to InvalidDataException, he may miss other exceptions or get cast exceptions.Based on follow up Copilot code reviews, I did few more behavior changes at edge cases, they do not relate directly to issue and can be reverted independently if not desired.
OnReload()now fires only when Data actually changed. Previously on first load, if a parse or IO error occurred andOnLoadExceptionsetIgnore = true,OnReload()was fired unconditionally even though Data was never modified. The new code firesOnReload()only when Data was successfully loaded or explicitly cleared (e.g., on reload or optional missing file). For consecutive reloads with parse errors, Data is cleared andOnReload()still fires. This change is independent on original fix and can be reverted independently if not desirable.Fix #113964