Skip to content

FileConfigurationProvider: Handle and forward IO exceptions to OnLoadException#126093

Open
mrek-msft wants to merge 35 commits intodotnet:mainfrom
mrek-msft:dev/mrek/mefs-filecfgprov-reload-locked
Open

FileConfigurationProvider: Handle and forward IO exceptions to OnLoadException#126093
mrek-msft wants to merge 35 commits intodotnet:mainfrom
mrek-msft:dev/mrek/mefs-filecfgprov-reload-locked

Conversation

@mrek-msft
Copy link
Copy Markdown
Member

@mrek-msft mrek-msft commented Mar 25, 2026

Current behavior of FileConfigurationProvider on various types of failures at various times:

First Load OnChange Reload
data parsing failure OnLoadException callback is called
AddJsonFile throws

OnLoadException callback is called
program continues
no reload happen
IO failure on file open AddJsonFile throws Silent
program continues
no reload happen
possibly raises UnobservedTaskException later

This PR unifies it in a way that both types of failure behave same as data parsing failure.

This brings some behavioral changes

  • OnLoadException callback is newly triggered if IO error happen on both types of events (first load, reload)
  • UnobservedTaskException can no longer be observed in scenario when user registers OnLoadException callback. It can however happen when no callback is registered, so XML comment on OnLoadException was updated.
  • OnLoadException callbacks 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 and OnLoadException set Ignore = true, OnReload() was fired unconditionally even though Data was never modified. The new code fires OnReload() 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 and OnReload() still fires. This change is independent on original fix and can be reverted independently if not desirable.

Fix #113964

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 through HandleException/OnLoadException.
  • Adds an inner try/catch to wrap exceptions thrown by Load(stream) in an InvalidDataException with the standard “Failed to load…” message.
  • Adjusts the outer catch to forward the exception through HandleException (enabling ignore behavior on reload).

@mrek-msft mrek-msft marked this pull request as draft March 25, 2026 15:36
Copilot AI review requested due to automatic review settings March 26, 2026 16:02
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-filesystem
See info in area-owners.md if you want to be subscribed.

Copilot AI review requested due to automatic review settings March 26, 2026 16:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

@mrek-msft mrek-msft requested a review from tarekgh April 21, 2026 10:53
Copilot AI review requested due to automatic review settings April 21, 2026 11:08
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 21, 2026 11:19
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings April 21, 2026 13:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

}
catch (Exception ex)
{
if (reload)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a small behavior change here. Before when the first load fail (reload is false), the OnReload was called. Now it is not. This is fine change with me but would be good if we document it somehow.

catch (Exception ex)
{
// IO error on file open, preserve existing Data
HandleException(ExceptionDispatchInfo.Capture(ex));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consumers who registered OnLoadException callbacks expecting only FileNotFoundException or InvalidDataException will now receive:

  • IOException (file locked, disk error)
  • UnauthorizedAccessException (permission denied)
  • Any exception from a custom IFileInfo.CreateReadStream()

Maybe it is worth documenting it as a breaking change?

Copy link
Copy Markdown
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left minor comments I hope you address them. LGTM, otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled exception from FileConfigurationProvider when re-loading a locked file

6 participants