Bypass CreateReadStream for PhysicalFileInfo in FileConfigurationProvider - #131486
Draft
rosebyte wants to merge 1 commit into
Draft
Bypass CreateReadStream for PhysicalFileInfo in FileConfigurationProvider#131486rosebyte wants to merge 1 commit into
rosebyte wants to merge 1 commit into
Conversation
|
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. |
Contributor
|
Tagging subscribers to this area: @dotnet/area-extensions-filesystem |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR narrows the FileConfigurationProvider “physical path” fast path so it only bypasses IFileInfo.CreateReadStream() when the IFileInfo is exactly PhysicalFileInfo, preserving the sync-IO optimization for the built-in physical provider while restoring correct behavior for custom/delegating providers that transform file contents in CreateReadStream().
Changes:
- Update
FileConfigurationProviderto use the syncFileStreamfast path only whenfileInfo.GetType() == typeof(PhysicalFileInfo). - Add coverage to ensure providers that return a
PhysicalPathbut transform content viaCreateReadStream()are not bypassed. - Adjust the existing
FileInfoImpl.CreateReadStream()test helper to open the physical file so existing exception-path tests remain meaningful with the new gating.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationProvider.cs | Restricts the fast-path FileStream open to exact PhysicalFileInfo instances instead of any non-null PhysicalPath. |
| src/libraries/Microsoft.Extensions.Configuration.FileExtensions/tests/FileConfigurationProviderTest.cs | Adds a new [Theory] covering physical/delegating/derived/custom providers and updates a test IFileInfo helper to open the actual file stream. |
Comment on lines
+198
to
+199
| Assert.Equal(expectedContent, provider.Content); | ||
| Assert.Equal(expectedSynchronousRead, provider.ReadSynchronously); |
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.
Fixes #47455
Since .NET 5, a custom
IFileProviderwhoseIFileInforeports aPhysicalPathbut transforms the content inCreateReadStreamis silently ignored by the configuration system. The classic case is an encryptedappsettings.jsonthat a provider decrypts on read:AddJsonFilenow hands the parser the raw encrypted bytes straight off the disk rather than the decrypted stream.PhysicalPathis documented as "the path to the file, including the file name, or null if the file is not directly accessible". It says nothing about the bytes at that path being the bytesCreateReadStreamwould produce. That equivalence is an implementation detail ofPhysicalFileInfo, not part of theIFileInfocontract, so relying on it breaks any provider that layers on top of a real file.Root cause
#37846 added a fast path to
FileConfigurationProvider.Loadthat opens aFileStreamwith async I/O disabled, since the configuration system reads synchronously and theFileOptions.AsynchronousthatPhysicalFileInfo.CreateReadStreamrequests is pure overhead here. The optimisation is worth having; the trouble is only that it was gated onfileInfo.PhysicalPath != null, which is true for far more implementations than the one it was written for.Why the check is on the file info rather than the file provider
The earlier attempt at this, #47934 (closed for inactivity, not on merit), checked
source.FileProvider.GetType() == typeof(PhysicalFileProvider)instead. Checking the file info is strictly better:PhysicalFileProvider.GetFileInforeturns onlyNotFoundFileInfoorPhysicalFileInfo, andNotFoundFileInfois already filtered out by the!file.Existsbranch above. So wherever the provider check would fire, the file info check fires too.CompositeFileProviderand anything else that surfaces an innerPhysicalFileProvider'sPhysicalFileInfounchanged would have been quietly deoptimised by the provider check.CompositeFileProvideris common enough in ASP.NET Core hosting that this matters.FileConfigurationSource.FileProvideris a mutable public property read afresh on everyLoad, so a provider-type flag computed in the constructor (as suggested in review on the old PR) could be wrong by the time it is used. A per-call check on the file info has no such asymmetry.PhysicalFileInfo.PhysicalPathreturnsFileInfo.FullName, which is never null, so the null check is not merely redundant after the change but provably unnecessary.No new dependency is introduced:
Microsoft.Extensions.FileProviders.Physicalis already an unconditionalProjectReferenceof this project, used byFileConfigurationSource.ResolveFileProvider. There is no trim or AOT concern either, as this is type identity only and no reflection.