Skip to content

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Sep 27, 2021

This updates WebApplicationBuilder to only load IConfigurationProviders once. Before this change, they would be loaded twice. Once by Configurationmanager and then again by the inner generic host's ConfigurationBuilder.

This causes problems with StreamConfigurationSources like the one used by builder.Configuration.AddJsonStream(jsonStream) throwing errors similar to the following:

System.Text.Json.JsonReaderException
  HResult=0x80131500
  Message=The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true. LineNumber: 0 | BytePositionInLine: 0.
  Source=System.Text.Json
  StackTrace:
   at System.Text.Json.ThrowHelper.ThrowJsonReaderException(Utf8JsonReader& json, ExceptionResource resource, Byte nextByte, ReadOnlySpan`1 bytes)
   at System.Text.Json.Utf8JsonReader.Read()
   at System.Text.Json.JsonDocument.Parse(ReadOnlySpan`1 utf8JsonSpan, JsonReaderOptions readerOptions, MetadataDb& database, StackRowStack& stack)
   at System.Text.Json.JsonDocument.Parse(ReadOnlyMemory`1 utf8Json, JsonReaderOptions readerOptions, Byte[] extraRentedArrayPoolBytes, PooledByteBufferWriter extraPooledByteBufferWriter)
   at System.Text.Json.JsonDocument.Parse(ReadOnlyMemory`1 json, JsonDocumentOptions options)
   at System.Text.Json.JsonDocument.Parse(String json, JsonDocumentOptions options)
   at Microsoft.Extensions.Configuration.Json.JsonConfigurationFileParser.ParseStream(Stream input)
   at Microsoft.Extensions.Configuration.Json.JsonStreamConfigurationProvider.Load(Stream stream)
   at Microsoft.Extensions.Configuration.StreamConfigurationProvider.Load()
   at Microsoft.Extensions.Configuration.ConfigurationRoot..ctor(IList`1 providers)
   at Microsoft.Extensions.Configuration.ConfigurationBuilder.Build()
   at Microsoft.Extensions.Hosting.HostBuilder.BuildAppConfiguration()
   at Microsoft.Extensions.Hosting.HostBuilder.Build()
   at Microsoft.AspNetCore.Builder.WebApplicationBuilder.Build()

Fixes #37030
Fixes #37046

@halter73
Copy link
Member Author

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1280231592

@davidfowl
Copy link
Member

I think this change might be too braking. Nothing looks at sources but lots of things look at providers. Having them wrapped is pretty breaking. For example GetDebugView is completely broken by this change

@halter73 halter73 requested a review from Eilon September 27, 2021 22:37
@halter73
Copy link
Member Author

I think this change might be too braking. Nothing looks at sources but lots of things look at providers. Having them wrapped is pretty breaking. For example GetDebugView is completely broken by this change

GetDebugView() is fixed pretty trivially by forwarding ToString() which I just did in this commit. Do you have examples of any code looking at the actual type of the providers @davidfowl?

@davidfowl
Copy link
Member

I generally don't feel comfortable with wrapping providers because they are exposed on IConfigurationRoot. I'm super paranoid about doing anything funky that changes the behavior after the service provider is built. There's too much code running not to break stuff

@halter73 halter73 force-pushed the halter73/load-less branch 4 times, most recently from 5a0b4c8 to 3440cd3 Compare September 28, 2021 03:38
@halter73 halter73 requested a review from Pilchie as a code owner September 30, 2021 23:46
@halter73 halter73 force-pushed the halter73/load-less branch 4 times, most recently from 399e5d4 to 7fd31c1 Compare September 30, 2021 23:55
@halter73 halter73 requested a review from a team October 1, 2021 00:01
@halter73
Copy link
Member Author

halter73 commented Oct 1, 2021

@dotnet/minimal-apis I think this is ready for another review.

I've updated the approach to reverse the direction we're chaining configuration. Before, we chained the HostBuilder's IConfiguration to the WebApplicationBuilder's ConfigurationManager after the host was built. Now we chain the ConfigurationManager to the HostBuilders IConfigurationBuilder when we start building the host. This also makes the ConfigurationManager the IConfiguration that's resolved from DI rather than the HostBuilder's IConfiguration.

This forces us to copy IConfigurationProviders in the opposite direction we did before, and we still have to wrap these providers to prevent double loads. However, we expect this will mostly be test providers, so not preserving the provider types should be less of an issue.

@halter73 halter73 force-pushed the halter73/load-less branch from 8def990 to cbac727 Compare October 1, 2021 21:06
@halter73 halter73 force-pushed the halter73/load-less branch from cbac727 to e76ef50 Compare October 5, 2021 22:18
@halter73 halter73 enabled auto-merge (squash) October 5, 2021 22:20
@halter73 halter73 merged commit 1a4db56 into main Oct 5, 2021
@halter73 halter73 deleted the halter73/load-less branch October 5, 2021 23:45
@ghost ghost added this to the 7.0-preview1 milestone Oct 5, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
4 participants