HttpWebRequest.GetRequestStreamAsync doesn't open the connection to the server - we always buffer the content

[stephentoub]

The corefx implementation of HttpWebRequest.GetRequestStreamAsync effectively just returns an in-memory stream. The connection to the server isn't made and the data isn't uploaded until Stream.Dispose is called on the that Stream. This is problematic for several reasons, including that the actual asynchronous operation ends up happening in the synchronous Dispose call on the stream, and deadlocks can easily happen if code assumes that the server will respond once the supplied data has been written (but before Dispose is called).

[davidsh]

This is actually the same design as .NET Framework (Desktop). The only difference is that in Desktop, there is a property on HttpWebRequest (.AllowWriteStreamBuffering) which can toggle this behavior.

Eventually, when we add all the API surface back, we will have to solve it then. The difficulty at this point, though, is that the HttpWebRequest API surface is built on top of HttpClient and it has a different data model for handling when data actually starts going to a server.

[davidsh]

I am removing the 'bug' label because it isn't a bug, per se. It is the proper behavior if the .AllowWriteStreamBuffering option is TRUE, which it always is right now in the current implementation.

[stephentoub]

Ok. Thanks, David.

[Im5tu]

Will you actually come around to fixing these sorts of things though considering how it could break applications? I'm guessing I'm saying why not fix it now?

[karelz]

We should evaluate it for 2.0 based on usage data of AllowWriteStreamBuffering.

[karelz]

@weshaggard can you please help us evaluate usage of AllowWriteStreamBuffering?

[karelz]

Usage data is on apisof.net (thanks @stephentoub for pointing it out): https://apisof.net/catalog/System.Net.HttpWebRequest.AllowWriteStreamBuffering

• nuget org - 1.1%
• API Port Telemetry - 3.1%

That seems above the threshold we went after in .NET Standard 2.0 - @danmosemsft, @weshaggard what do you think?

[karelz]

Our threshold was around 0.1%, so this is clearly in from that point of view.

@davidsh @CIPop - can you guys please cost it roughly? (you said it is complex/costly), we will have to roll it up to our management chains for 2.0

[weshaggard]

@karelz I'm not sure I understand the question. AllowWriteStreamBuffering is part of .NET Standard 2.0 so the API has been added back, is the question whether or not we should fix the implementation to do the buffering correctly again? If so I would yes we probably should in order to be compatible as that flag is set in the default set of flags we use (https://github.com/dotnet/corefx/blob/ec2a6190efa743ab600317f44d757433e44e859b/src/System.Net.Requests/src/System/Net/HttpWebRequest.cs#L84).

[karelz]

@weshaggard the buffering is hidden under AllowWriteStreamBuffering = true. It is not (EDITED) the default.
We decided to leave it in 2.0 for now and figure out the cost.

[karelz]

I appologize (@weshaggard corrected me) - the flag is set in Default on .NET Core.
@davidsh @CIPop does Desktop have the same Default value with the flag set?
When the flag set, what does the buffering mean? Is the in-memory stream considered the buffer? (on Desktop and on .NET Core)

[davidsh]

When the flag set, what does the buffering mean? Is the in-memory stream considered the buffer? (on Desktop and on .NET Core)

The default value is TRUE (which works). And yes, "buffering" means that the data is held in a MemoryStream. The dev gets the request stream and writes to it. But those writes are not sent on the wire but fill up a MemoryStream. Later, when the request is sent to the HTTP server, the MemoryStream is replayed (usually by grabbing the stored buffer) and sent on the wire.

The Desktop and .NET Core both use a MemoryStream for the buffering.

[karelz]

Thanks!
@weshaggard I noticed that you looked at (private) enum value instead of on the property with the same name AllowWriteStreamBuffering

I think our usage analysis above is valid. Assuming people will (mostly) use the property only to set it to non-default value, which is false

[vitek-karas]

Per the code at https://github.com/dotnet/corefx/blob/master/src/System.Net.Requests/src/System/Net/HttpWebRequest.cs#L137
the value of the AllowReadStream property is now always false... not sure if that is intentional, but the above discussion says otherwise (that we always buffer and thus the property should always be true).

[davidsh]

Per the code at https://github.com/dotnet/corefx/blob/master/src/System.Net.Requests/src/System/Net/HttpWebRequest.cs#L137
the value of the AllowReadStream property is now always false... not sure if that is intentional, but the above discussion says otherwise (that we always buffer and thus the property should always be true).

Actually, I should not have linked these issues. The new issue you entered dotnet/corefx#18668 has nothing to do with this issue. Let's continue discussion on that issue instead.

[karelz]

Triage discussion: While important, this is P2 at this moment and won't make it into 2.0, unless we get additional feedback from customers.

[jokker87]

Do i get this right, even when I set AllowWriteStreamBuffering to false, in .net core, when i call GetRequestStream, the stream always gets buffered in a MemoryStream till the stream is closed? So we have a memory issue when uploading big files to an endpoint?

[davidsh]

Do i get this right, even when I set AllowWriteStreamBuffering to false, in .net core, when i call GetRequestStream, the stream always gets buffered in a MemoryStream till the stream is closed? So we have a memory issue when uploading big files to an endpoint?

Yes. We recommend that you use HttpClient APIs and not the legacy HttpWebRequest APIs. HttpClient has APIs that can support the similar scenario where uploading the request body from a large source (i.e. filestream) will not be completely buffered into memory first. Instead, it will read directly from the filestream and write directly to the network.

[karelz]

Triage: Related to @alnikola's effort in HttpWebRequest. This may be non-trivial.

[liveans]

Fixed by #95001