Runtime: HttpWebRequest request stream returns different exceptions from Desktop

[davidsh]

The request stream returned from HttpWebRequest.GetRequestStream is always a MemoryStream on .NET Core. On .NET Framework, it is a System.Net.ConnectStream (an internal class).

Due to this, there are small behavior difference w.r.t. the exception returned while doing argument checking:

See:
```c#
[Fact]
public void WriteAsync_OffsetPlusCountExceedsBufferLength_ThrowsArgumentException()
{
using (Stream stream = GetRequestStream())
{
if (PlatformDetection.IsFullFramework)
{
// In .NET Framework, the request stream is a System.Net.ConnectStream
// which throws ArgumentOutOfRangeException in this case.
Assert.Throws(() => { Task t = stream.WriteAsync(buffer, 0, buffer.Length+1); });
}
else
{
// In .NET Core, the request stream is a System.IO.MemoryStream
// which throws ArgumentException in this case.
Assert.Throws(() => { Task t = stream.WriteAsync(buffer, 0, buffer.Length+1); });
}
}
}

    [Fact]
    public void WriteAsync_OffsetPlusCountMaxValueExceedsBufferLength_Throws()
    {
        using (Stream stream = GetRequestStream())
        {
            if (PlatformDetection.IsFullFramework)
            {
                // In .NET Framework, the request stream is a System.Net.ConnectStream
                // which throws ArgumentOutOfRangeException in this case.
                Assert.Throws<ArgumentOutOfRangeException>(() => { Task t = stream.WriteAsync(buffer, int.MaxValue, int.MaxValue); });

            }
            else
            {
                // In .NET Core, the request stream is a System.IO.MemoryStream
                // which throws ArgumentException in this case.
                Assert.Throws<ArgumentException>(() => { Task t = stream.WriteAsync(buffer, int.MaxValue, int.MaxValue); });
            }
        }
    }

```

Discovered while investigating dotnet/runtime#20873 and related to dotnet/runtime#18632.

For app-compat, we should consider fixing this. The simplest fix would be to create a derived class of MemoryStream() and make sure all Write* methods are overridden with matching argument validation before calling the base methods.

[davidsh]

The specific argument validation done in .NET Framework for ConnectStream is:

c# if (buffer==null) { throw new ArgumentNullException("buffer"); } if (offset<0 || offset>buffer.Length) { throw new ArgumentOutOfRangeException("offset"); } if (size<0 || size>buffer.Length-offset) { throw new ArgumentOutOfRangeException("size"); }

This differs from MemoryStream.

[davidsh]

cc: @stephentoub @karelz @CIPop

[karelz]

Triage:
Catching the exceptions is rare. ArgumentOutOfRangeException would be better as it is more specific and likely being caught.

P2 for 2.0
Very simple fix.

[karelz]

@hughbe @WinCPP are you interested? It would be nice to have in 2.0 ...

[WinCPP]

@karelz Please assign it to me...

@davidsh Was trying to figure out the source of exception location for changes... Is it in this file (link)

I believe at Line 22 we want to assign new instance of the new derived class. It would have to be defined in the same project with a name, say, MemoryStreamForRequest and override Write* therein...

So wanted to confirm if I am looking into correct project / file... Thanks!

[davidsh]

@WinCPP Thank you for your interest in this.

In terms of what code to change, yes, you'll want to create a derived class of MemoryStream and override the Write* methods to do parameter validation and then call the base methods to do the actual work.

Please review this .NET Framework source to make sure you match behavior.

https://github.com/Microsoft/referencesource/blob/master/System/net/System/Net/_ConnectStream.cs#L801

We'll also need new unit tests included in any PR.

[karelz]

Thanks @WinCPP!

[WinCPP]

@davidsh just asking... instead of creating a derived class of MemoryStream, how about doing the necessary validations that throw expected exceptions right in the RequestStream.Write* methods (here)?

Another thought being that we are validating the inputs to the API and those could be kept separate from the underlying stream, by having the validations directly done in the API, unless we are planning to reuse the derived class in another place, else the derived class is an additional unnecessary type...?

[davidsh]

how about doing the necessary validations that throw expected exceptions right in the RequestStream.Write* methods (here)?

Yes, that is a better idea. Please go with that.