[Addresses #9466] API to configure TempDirectory used by HttpBuffering #11569

cjaliaga · 2019-06-25T21:46:26Z

Addresses #9466

This is still not ready but before implementing it in all the callers I'd like to check if the logic looks right.

Currently the HttpBufferingOptions is the one configured in the DI, and IOptions<HttpBufferingOptions> resolved in the callers, such EnableRewind or the MVC formatters.

For the creation of the Streams, a new Factory is added (HttpFileBufferingFactory) in Microsoft.AspNetCore.Http which gets in the ctor the options. This factory could be resolved instead of being manually created, in that case, what should be the right place to be registered into the Service Collection?

Another option would be making the factory more generic, just getting the TempDirectoryPath as string in the ctor instead of being tied to the HttpBufferingOptions, so it could be added to the WebUtilities project.

@Tratcher, @anurse could you please take a look and let me know what you think?

Tratcher · 2019-07-01T16:29:36Z

src/Http/Http/src/IFileBufferingStreamFactory.cs

+
+namespace Microsoft.AspNetCore.Http
+{
+    public interface IFileBufferingStreamFactory


This would likely go in the Http.Abstractions package.

I was thinking on putting IBufferedWriteStream on WebUtilities but in that case Http.Abstractions would have a dependency on it, is that OK?

Hmm, tradeoffs. Let's not change the dependencies right now. That means IFileBufferingStreamFactory stays in Http and IBufferedWriteStream in WebUtilities.

Tratcher · 2019-07-01T16:30:03Z

src/Http/Http/src/IFileBufferingStreamFactory.cs

+{
+    public interface IFileBufferingStreamFactory
+    {
+        FileBufferingReadStream CreateReadStream(Stream inner, int bufferThreshold, long? bufferLimit = null);


Why can't this return Stream?

The consumers are actually expecting a Stream, so it can return it. The only reason was because the consumers of the writer need the FileBufferingWriteStream explicitly so I kept this one same way. Moving this to return Stream sounds better.

https://github.com/aspnet/AspNetCore/blob/6152fc37b7ab4d61ac17f4aa1c5109fd5ee8b2e8/src/Mvc/Mvc.Formatters.Xml/src/XmlDataContractSerializerOutputFormatter.cs#L291

The only reason was because the consumers of the writer need the FileBufferingWriteStream explicitly

I expect we could remove that requirement. DrainBufferAsync looks like a fancy CopyToAsync.

I expect we could remove that requirement. DrainBufferAsync looks like a fancy CopyToAsync.

By default CopyTo throws NotSupportedException when a stream cannot be read, like in this case. Could it be confusing if we allow to copy this stream but reading it is not allowed? I don't know, however, if this behavior is usually honored.

You're right, that came up in the design so we added DrainBufferAsync. I'd rather not have the concrete type exposed here. Maybe we can abstract it.

public interface IBufferedWriteStream { Stream Buffer { get; } Task DrainBufferAsync(Stream destination); }

If I got it right, FileBufferingWriteStream would be implementing IBufferedWriteStream and returned from the factory instead of the concrete type.

Regarding of Buffer property, and also that we need the actual Stream in the consumers, what is the purpose of the property and how are we getting the Stream?

I have the feeling that property could be the answer since IBufferedWriteStream doesn't know it's a Stream (cannot extend the class), so TL;DR, could be the implementation this one and the way of getting it instead of casting on the consumers?

Stream Buffer => this;

Stream Buffer => this;

Exactly

src/Http/Http/src/Features/FormFeature.cs

Tratcher · 2019-07-01T16:32:35Z

src/Http/Http/src/Internal/BufferingHelper.cs

@@ -21,15 +22,17 @@ public static HttpRequest EnableRewind(this HttpRequest request, int bufferThres
            var body = request.Body;
            if (!body.CanSeek)
            {
-                var fileStream = new FileBufferingReadStream(body, bufferThreshold, bufferLimit, AspNetCoreTempDirectory.TempDirectoryFactory);
+                var httpBufferingOptions = request.HttpContext.RequestServices.GetRequiredService<IOptions<HttpBufferingOptions>>();
+                var factory = new HttpFileBufferingStreamFactory(httpBufferingOptions);


Why isn't the factory being resolved from DI? Yes something would need to register it, probably Hosting with the rest of the default services.
https://github.com/aspnet/AspNetCore/blob/70b478136a702d2b6061ac9b159029189ad230d0/src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs#L84
https://github.com/aspnet/AspNetCore/blob/70b478136a702d2b6061ac9b159029189ad230d0/src/Hosting/Hosting/src/WebHostBuilder.cs#L284

Great, I thought about register and resolve the factory but didn't know where to register it. Hosting sounds good, thanks :)

I can register together the IPostConfiguration for the options and the factory, probably with an Extension method, like AddHttpBuffering()?

The scope I believe it should be a singleton, out of curiosity, why the DefaultHttpContextFactory in the Generic is a Singleton and in the other one is Transient? Should I follow same pattern here?

I can register together the IPostConfiguration for the options and the factory, probably with an Extension method, like AddHttpBuffering()?

Sounds good.

The scope I believe it should be a singleton, out of curiosity, why the DefaultHttpContextFactory in the Generic is a Singleton and in the other one is Transient? Should I follow same pattern here?

I don't recall, but follow what we do in Generic, it's more recent.

src/Http/Http/test/Features/FormFeatureTests.cs

src/Http/Http/src/HttpBufferingOptions.cs

…mFactory.

cjaliaga · 2019-07-07T03:40:24Z

src/Http/WebUtilities/src/IBufferedWriteStream.cs

+
+namespace Microsoft.AspNetCore.WebUtilities
+{
+    public interface IBufferedWriteStream : IAsyncDisposable, IDisposable


Marking this as IDisposable helps in situations like this one:

https://github.com/aspnet/AspNetCore/blob/d6c629a005a859ca653cbb2ec96d4b6c385584cf/src/Mvc/Mvc.ViewFeatures/src/ViewComponentResultExecutor.cs#L157

And it also would simplify this:

https://github.com/aspnet/AspNetCore/blob/d6c629a005a859ca653cbb2ec96d4b6c385584cf/src/Mvc/Mvc.NewtonsoftJson/src/NewtonsoftJsonOutputFormatter.cs#L169

What do you think?

Looks good.

…sed.

davidfowl · 2019-07-07T23:08:01Z

I haven't been following this closely but I'm not a fan of adding more services to hosting to enable this to work. I strongly recommend looking for an alternative design. We've been slowly moving away from adding these "default services" to hosting (see #5944).

cjaliaga · 2019-07-09T01:20:44Z

Would it make sense to add the required services with AddHttpBuffering() in the places that is used? Currently in Mvc:

Mvc.Formatters.Xml\src\XmlDataContractSerializerOutputFormatter.cs
Mvc.Formatters.Xml\src\XmlSerializerOutputFormatter.cs
Mvc.NewtonsoftJson\src\NewtonsoftJsonOutputFormatter.cs
Mvc.NewtonsoftJson\src\NewtonsoftJsonResultExecutor.cs
Mvc.ViewFeatures\src\ViewComponentResultExecutor.cs

For these ones, Mvc.Core or the individual component?

It's also used in Http\src\Internal\BufferingHelper.cs, called from from FormFeature:

https://github.com/aspnet/AspNetCore/blob/a4a08a68f3a01b6f89a357de2cef67b6b823b348/src/Http/Http/src/Features/FormFeature.cs#L140

https://github.com/aspnet/AspNetCore/blob/a4a08a68f3a01b6f89a357de2cef67b6b823b348/src/Http/Http/src/Features/FormFeature.cs#L187-L191

but exposed publicly via HttpRequest extension methods in

https://github.com/aspnet/AspNetCore/blob/a4a08a68f3a01b6f89a357de2cef67b6b823b348/src/Http/Http/src/Extensions/HttpRequestRewindExtensions.cs#L21-L24

So, in these cases, do you know of another place more specific than hosting that could work? Having a fallback in case is not registered (not sure if this would be clean)?

Tratcher · 2019-07-09T22:20:44Z

No, features exposed directly from HttpContext need to work with pre-registered services. Adding it by default in Hosting should be adequate.

Tratcher · 2019-07-09T22:22:04Z

@davidfowl I'll follow-up with you directly.

Tratcher · 2019-07-10T18:38:40Z

Offline discussion:

Options and services for things used by HttpContext are problematic.
A temp dir is a more fundamental host level concern, we should move this to IHostEnvironment
https://github.com/aspnet/Extensions/blob/master/src/Hosting/Abstractions/src/IHostEnvironment.cs
or IWebHostEnvironment
https://github.com/aspnet/AspNetCore/blob/master/src/Hosting/Abstractions/src/IWebHostEnvironment.cs
then it would be configured through the host APIs, not via options and services.

@glennc to check if IHostEnvironment vs IWebHostEnvironment makes the most sense.

cjaliaga · 2019-07-11T22:02:52Z

Thanks @Tratcher, I like it. This way it's not just the temp dir for HttpBuffering but a way for setting/getting the temp dir for everyone.

The consumers still need to create the Buffering Read and Write Stream, in this case if we use the host APIs to configure the TempDirectory, we check if the Directory exists on configuration and everything there, could we resolve the HostEnvironment and do this for the accessor?

new FileBufferingWriteStream(tempFileDirectoryAccessor: () => _environment.TempDirectory);

Or is still better and safer to pass a Func like this in case there's something in the middle breaking it?
https://github.com/aspnet/AspNetCore/blob/a4a08a68f3a01b6f89a357de2cef67b6b823b348/src/Http/Http/src/HttpFileBufferingStreamFactory.cs#L47-L52

Regarding the default path, are we still keeping the default behavior in the Streams for getting the accessor or deprecating/marking obsolete (this is the breaking-change itself)?

https://github.com/aspnet/AspNetCore/blob/a4a08a68f3a01b6f89a357de2cef67b6b823b348/src/Http/WebUtilities/src/FileBufferingWriteStream.cs#L58

Tratcher · 2019-07-11T22:09:53Z

Or is still better and safer to pass a Func like this in case there's something in the middle breaking it?

Yeah, reading and validating TempFileDirectory should be lazy because it may not be created until after startup, and could even be deleted later.

Regarding the default path, are we still keeping the default behavior in the Streams for getting the accessor or deprecating/marking obsolete (this is the breaking-change itself)?

Yes, we'll keep any back-compat that's reasonably possible, but may decided to Obsolete(Warning) some of them.

cjaliaga · 2019-07-19T01:20:59Z

Yeah, reading and validating TempFileDirectory should be lazy because it may not be created until after startup, and could even be deleted later.

Makes sense. We could keep the factory then but just in WebUtilities this way:

public class FileBufferingStreamFactory : IFileBufferingStreamFactory
{
     public FileBufferingStreamFactory(string tempPath)
     {
         _tempPath = tempPath;
         _getTempDirectory = () => _tempDirectory;
     }

     private string _tempDirectory
     {
          get
           {
               if (!Directory.Exists(_tempPath))
               {
                   throw new DirectoryNotFoundException(_tempPath);
               }

               return _tempPath;
           }
       }
}

And create it when needed resolving HostEnvironment to get TempDirectory. Or just keep creating the BufferingStreams in the consumers manually.

@glennc to check if IHostEnvironment vs IWebHostEnvironment makes the most sense.

Glenn, could you please take a look to see in which one it suits better? Specially as the former would make it cross-repo.

cjaliaga · 2019-07-23T01:41:00Z

@Tratcher I've started with this on a new branch (cjaliaga@750cc03), adding it on IWebHostEnvironment for the time being.

Is it better to create a new PR with the new branch linking this one or to reset this one to master and do it here to keep the PR active?

analogrelay · 2019-11-05T21:45:45Z

This got backburnered during the run up to 3.0. We'll try to look at this for 5.0 now!

Tratcher · 2019-11-11T22:18:13Z

Let's close this in favor of #12555

analogrelay added area-middleware and removed area-middleware labels Jun 25, 2019

Tratcher reviewed Jul 1, 2019

View reviewed changes

cjaliaga force-pushed the cjaliaga/http-buffering-options branch from 1f3d239 to 6152fc3 Compare July 1, 2019 21:35

analogrelay added this to the 3.0.0-preview8 milestone Jul 2, 2019

cjaliaga mentioned this pull request Jul 3, 2019

FileBufferingReadStream _maxRentedBufferSize is used as an exclusive bound #9838

Closed

cjaliaga force-pushed the cjaliaga/http-buffering-options branch from 6152fc3 to b4d76d8 Compare July 4, 2019 01:01

cjaliaga added 9 commits July 6, 2019 05:48

HttpFileBufferingFactory and buffering options.

933c935

EnableRewind with HttpFileBufferingFactory.

6c51f97

Registered IOptions<HttpBufferingOptions> in FormFeatureTests.

5f89991

XmlDataContractSerializarOutputFormatter using HttpFileBufferingStrea…

77e4bcb

…mFactory.

Update refs

ec0d270

Clean TempDirectory checks.

a33df59

HttpBuffering extensions and configuration.

ff104ac

HttpBuffering factory, Xml formatters, ServiceCollection extensions.

50f46c2

Current EnableRewind for MultipartSection obsolete

9831e36

cjaliaga force-pushed the cjaliaga/http-buffering-options branch from 5468745 to 9831e36 Compare July 6, 2019 04:16

cjaliaga added 2 commits July 7, 2019 03:48

NewtonsoftJson using IFileBufferingStreamFactory.

25d232d

IBufferedWriteStream disposable. ViewComponent with factory.

d6c629a

cjaliaga commented Jul 7, 2019

View reviewed changes

cjaliaga added 3 commits July 7, 2019 07:01

Update refs.

8f8659c

Simplified fileBufferingWriteStream dispose.

8211084

Removed EnableRewind without factory as it's internal and no longer u…

a4a08a6

…sed.

cjaliaga mentioned this pull request Jul 25, 2019

API to configure the TempDirectory used by ASP.NET Core #12555

Closed

pranavkm modified the milestones: 3.0.0-preview8, 5.0.0-alpha1 Aug 13, 2019

analogrelay assigned Tratcher Nov 5, 2019

analogrelay modified the milestones: blazor-wasm, 5.0.0-preview1 Nov 5, 2019

Tratcher closed this Nov 11, 2019

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

[Addresses #9466] API to configure TempDirectory used by HttpBuffering #11569

[Addresses #9466] API to configure TempDirectory used by HttpBuffering #11569

Uh oh!

Conversation

cjaliaga commented Jun 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Jul 7, 2019

Uh oh!

cjaliaga commented Jul 9, 2019

Uh oh!

Tratcher commented Jul 9, 2019

Uh oh!

Tratcher commented Jul 9, 2019

Uh oh!

Tratcher commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjaliaga commented Jul 11, 2019

Uh oh!

Tratcher commented Jul 11, 2019

Uh oh!

cjaliaga commented Jul 19, 2019

Uh oh!

cjaliaga commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analogrelay commented Nov 5, 2019

Uh oh!

Tratcher commented Nov 11, 2019

Uh oh!

Uh oh!

Tratcher commented Jul 10, 2019 •

edited

Loading

cjaliaga commented Jul 23, 2019 •

edited

Loading