Runtime: System.Net.Tests.HttpRequestStreamTests.CanWrite_Get_ReturnsFalse and CanRead_Get_ReturnsFalse fails on UAPAOT: NotSupported_UnwritableStream/NotSupported_UnreadableStream

[CIPop]

System.Net.Tests.HttpRequestStreamTests.CanWrite_Get_ReturnsFalse [FAIL]
        Assert.Throws() Failure
        Expected: typeof(System.InvalidOperationException)
        Actual:   typeof(System.NotSupportedException): NotSupported_UnwritableStream. For more information, visit http://go.microsoft.com/fwlink/?Lin
  kId=623485
        Stack Trace:
              at xunit.console.netcore!<BaseAddress>+0xa35f68
              at System.Net.Tests.HttpRequestStreamTests.<>c__DisplayClass16_0.<CanWrite_Get_ReturnsFalse>b__1()


System.Net.Tests.HttpResponseStreamTests.CanRead_Get_ReturnsFalse [FAIL]
        Assert.Throws() Failure
        Expected: typeof(System.InvalidOperationException)
        Actual:   typeof(System.NotSupportedException): NotSupported_UnreadableStream. For more information, visit http://go.microsoft.com/fwlink/?Lin
  kId=623485
        Stack Trace:
              at xunit.console.netcore!<BaseAddress>+0xb41398
              at System.Net.Tests.HttpResponseStreamTests.<>c__DisplayClass12_0.<CanRead_Get_ReturnsFalse>b__1()
     System.Net.Tests.HttpResponseStreamTests.Write_NullBuffer_ThrowsArgumentNullException [FAIL]

[DavidGoll]

Need to root-cause this. Need to re-run with ILC build type=check and debug in a debugger.

[caesar-chen]

Re-run with type=check doesn't give more useful information.

System.Net.Tests.HttpResponseStreamTests.CanRead_Get_ReturnsFalse [FAIL]
        Assert.Throws() Failure
        Expected: typeof(System.InvalidOperationException)
        Actual:   typeof(System.NotSupportedException): Stream does not support reading.
        Stack Trace:
              at Task`1 System.IO.Stream.ReadAsync(Byte[], Int32, Int32, CancellationToken) + 0x98
              at Task`1 System.IO.Stream.ReadAsync(Byte[], Int32, Int32) + 0xa3
              at System.Net.Tests.HttpResponseStreamTests.<>c__DisplayClass5_0.<CanRead_Get_ReturnsFalse>b__1()
              at TResult System.Func`1.Invoke() + 0xb2

I will investigate this.

[caesar-chen]

So for this test, CanRead_Get_ReturnsFalse()

``` C#
[Fact]
public async Task CanRead_Get_ReturnsFalse()
{
using (HttpListenerResponse response = await _helper.GetResponse())
using (Stream outputStream = response.OutputStream)
{
Assert.False(outputStream.CanRead);

            Assert.Throws<InvalidOperationException>(() => outputStream.Read(new byte[0], 0, 0));
            await Assert.ThrowsAsync<InvalidOperationException>(() => outputStream.ReadAsync(new byte[0], 0, 0));
            Assert.Throws<InvalidOperationException>(() => outputStream.EndRead(null));
        }
    }

```

The problematic code is this line await Assert.ThrowsAsync<InvalidOperationException>(() => outputStream.ReadAsync(new byte[0], 0, 0));. After comment it out, the test will pass on ILC.

My guess for throwing NotSupportedException instead of the expected InvalidOperationException is:
The operation is asynchronous(ReadAsync). Compiler did the check first and saw bool CanRead => false;, then directly throw (System.NotSupportedException): Stream does not support reading before the code actually executing the asynchronous operation and throw InvalidOperationException at runtime.

@davidsh

@yizhang82 @tijoytom Could you please confirm?

[tijoytom]

@caesar1995 Do you have the call stack? I might be case that we are throwing the wrong exception.

[caesar-chen]

@tijoytom Could you provide me some instructions on how to do this please?

I tried to attach xunit.console.netcore.exe to windbg, after execution completes, this is what I get:

ModLoad: 16db0000 16dc8000   MdTransform.dll
ModLoad: 16dd0000 16de8000   MdTransform.dll
ModLoad: 16df0000 16e2e000   NativeFormatWriter.dll
ModLoad: 16e50000 16e8e000   NativeFormatWriter.dll
ModLoad: 6d0f0000 6d118000   C:\WINDOWS\SysWOW64\ntmarta.dll
eax=00000000 ebx=00000000 ecx=00000002 edx=00000000 esi=00000000 edi=77bc99e0
eip=77b1edfc esp=00aff3ec ebp=00aff4c0 iopl=0         nv up ei pl nz na po nc
cs=0023  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000202
ntdll!NtTerminateProcess+0xc:
77b1edfc c20800          ret     8

0:000> !pe
There is no current managed exception on this thread

0:000> !clrstack
OS Thread Id: 0x39e4 (0)
Child SP       IP Call Site
GetFrameContext failed: 1

[tijoytom]

@CIPop @davidsh Can you please help @caesar1995 with the call-stack ?

[CIPop]

@tijoytom, @jkotas Looking at the code, this could be an ILC optimization bug. I'm thinking that our Begin/End functions are inlined given the size which means that HasOverridenBeginEndRead will not be able to find the method metadata.

@caesar1995, if this is the case, we could solve this by overriding ReadAsync and make it throw our expected exception.

---

In detail:

I think the exception is thrown by Stream here:
https://github.com/dotnet/coreclr/blob/6ba74dc2a7194f8d6c86c3aeab572a074ef645c8/src/mscorlib/src/System/IO/__Error.cs#L58

The OutputStream is a HttpResponseStream and has public override bool CanRead => false;. Even more, HttpResponseStream doesn't implement the Task based ReadAsync/WriteAsync methods. It does implement the Begin/End APM pattern and our test expects that BeginRead will be called and the exception is thrown here.

For some reason it looks like that path is optimized and that somehow tricks Stream into thinking that HasOverridenBeginEndRead is false.

This means that Read will be called which first checks if (!CanRead) throw Error.GetReadNotSupported();

[jkotas]

HasOverridenBeginEndRead will not be able to find the method metadata

This logic is not implemented at all for UAPAOT:
https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/IO/Stream.cs#L663

Implementing it is tracked in TFS 451292 / TFS 451317.

[CIPop]

Thanks @jkotas.
In today's SCRUM we've decided to work around this by implementing all Stream.*Async overrides in HttpListener. @caesar1995 is going to work on this.

There's a chance this will help with some of our hanging tests found in dotnet/corefx#22110.

[caesar-chen]

It's not possible to implemente all Stream.*Async overrides in HttpListener. For
C# public Task<int> ReadAsync(Byte[] buffer, int offset, int count)
we cannot override inherited member 'Stream.ReadAsync(byte[], int, int)' because it is not marked virtual, abstract, or override.

We are able to override the public virtual Task<int> ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken).
But the problem is, our test is calling the ReadAsync with 3 parameters, therefore, it will fail again with the same System.NotSupportedException.

The workaround could be:

1. Implement the ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken), and change our test to use this ReadAsync with cancellationToken
2. Keep test disabled, and wait for TFS 451292 / TFS 451317.

@davidsh @CIPop @DavidGoll

[davidsh]

public virtual Task ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)

You can have the 3-parameter method call the 4-parameter method like this:

c# public virtual Task<int> ReadAsync(Byte[] buffer, int offset, int count) { return ReadAsync(buffer, offset, count, CancellationToken.None) }

In other words, our code can call the 4-parameter one by passing in a None token if we don't have one.

[justinvp]

This is the implementation of the 3-parameter overload on Stream:

c# public Task<int> ReadAsync(Byte[] buffer, int offset, int count) { return ReadAsync(buffer, offset, count, CancellationToken.None); }

It is not virtual and just calls the 4-parameter overload, which is virtual.

If you want to change the behavior of ReadAsync in a subclass of Stream, all you need to do is override the 4-parameter overload. You should not add a new 3-parameter overload to the subclass.

[CIPop]

@jkotas @stephentoub we still have a test disabled.
We need a code change in CoreFX to re-enable the test once it's passing. Do you know how long we need to wait for the corert change to propagate?

[jkotas]

Do you know how long we need to wait for the corert change to propagate?

It takes several days. @MichalStrehovsky Could you please push CoreRT -> ProjectN when you get a chance to pick the fix for this?

[MichalStrehovsky]

@MichalStrehovsky Could you please push CoreRT -> ProjectN when you get a chance to pick the fix for this?

Done: dotnet/corert#4180. This will be in tonight's Project N build.

[caesar-chen]

I'm able to verify that after this PR: https://github.com/dotnet/corert/pull/4174, the problem goes away. I will submit a PR to re-enable the tests.