Moves Stream to shared location

[marek-safar]

No description provided.

[maryamariyan]

By looking at the diff between Stream class in coreclr and corert I see that:

(1)

public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, Object state)

and

public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, Object state)

throw NotImplementedException in corert.
Possible action: You could add #ifdef !CORECLR to keep the existing behavior for corert and remain to throw the exceptions.

(2)

sealed class SyncStream : Stream, IDisposable

is private in corert and internal in coreclr.
Possible action: Please see if you can make it private in coreclr as well.

(3)

public virtual Task FlushAsync(CancellationToken cancellationToken)

in corert has the extra if block check below:

            if (cancellationToken.IsCancellationRequested)
            {
                return Task.FromCanceled(cancellationToken);
            }

Possible action: You could add #ifdef !CORECLR to keep the existing behavior for corert and remain to throw the exceptions.

(4)

internal sealed class SynchronousAsyncResult : IAsyncResult

Is only used in coreclr.
Possible action: Could it be private? could it be moved to non-shared part?

[maryamariyan]

(5) More methods that would need to move to non-shared file (Stream.CoreCLR.cs):
@jkotas The methods listed below are async methods in corert

public override Task FlushAsync(CancellationToken cancellationToken)

public override Task<int> ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)

public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)

public override Task WriteAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)

public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)

non public APIs that could be moved to Stream.CoreCLR.cs:

        internal IAsyncResult BlockingBeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, Object state)

        internal static int BlockingEndRead(IAsyncResult asyncResult)

        internal IAsyncResult BlockingBeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, Object state)

        internal static void BlockingEndWrite(IAsyncResult asyncResult)

[maryamariyan]

(6) The implementations are different between coreclr and corert for below APIs:

public override IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, Object state)

public override int EndRead(IAsyncResult asyncResult)
         
public override IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, Object state)
            
public override void EndWrite(IAsyncResult asyncResult)

[maryamariyan]

@marek-safar @jkotas overall I'm not confident in this change. I see more in the diff between the two files than I pointed out to above, yet maybe some but not all the diffs in corert is a bit out of date.

I think there is still value in just conservatively reducing the visible diff rather than completely moving the file to shared. @marek-safar let me know what you think.

[jkotas]

(1) I would add these under #if CORERT

    #if CORERT
        throw new NotImplementedException(); // TODO: https://github.com/dotnet/corert/issues/3251
    #else
....

(2) Changing this to private in shared copy should work fine. A few other cosmetic diffs:

• CoreRT version has uses shorter syntax for the CanXXX methods that looks better, e.g.: public override bool CanRead => _stream.CanRead;.
• CoreRT version has DefaultCopyBufferSize. It would be nice to use the same one in the shared copy.

(3) @stephentoub Is it a good idea to have explicit check for IsCancellationRequested in Stream.FlushAsync ?

(4) (5) (6) @stephentoub Is the CoreRT implementation of old async based on TaskToApm better (it is certainly smaller) or should we keep the CoreCLR implementation based on SynchronousAsyncResult?

[stephentoub]

Is it a good idea to have explicit check for IsCancellationRequested in Stream.FlushAsync ?

corert's extra check is not necessary. The Task.Factory.StartNew call is being passed the cancellation token:

coreclr/src/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 255 to 259 in e1bbc70

	public virtual Task FlushAsync(CancellationToken cancellationToken)
	{
	return Task.Factory.StartNew(state => ((Stream)state).Flush(), this,
	cancellationToken, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);
	}

and that StartNew call will itself check the token.

Is the CoreRT implementation of old async based on TaskToApm better (it is certainly smaller) or should we keep the CoreCLR implementation based on SynchronousAsyncResult?

As currently implemented, corert's implementation is less efficient, incurring more allocation than the implementation in coreclr. We could say we don't really care about the perf of these Begin/EndRead/Write methods, but I'd be hesitant without doing more analysis.

[jkotas]

As currently implemented, corert's implementation is less efficient, incurring more allocation than the implementation in coreclr.

Let's use the coreclr implementation for the shared copy then.

[marek-safar]

(1) I would add these under #if CORERT

I am not sure this is necessary. The platform-specific checks are now in src/System.Private.CoreLib/src/System/IO/Stream.CoreCLR.cs and you can just implement them on CoreRT as always returning true or throwing NotImplementedException. This also allows us to implement them using icalls on Mono without another #if MONO around the same block.

(2) Done

(3, 4) Unchanged

[jkotas]

HasOverriddenBeginEndRead/Write is used in two different contexts. In Stream, it is used as an optimization - dummy implementation that just returns true works fine for this context. In SyncStream, it is used for correctness - dummy method that just returns true would result in hangs, it is better to throw PNSE. My suggestion was to add the CoreRT ifdef around the two SyncStream uses. It is TODO so ifdef is fine for that.

[marek-safar]

ok, ifdef-ed the SyncStream uses with PNSE

[jkotas]

@maryamariyan Do the change look good to you now?

[maryamariyan]

In corert we have NotImplementedException (refer to this).

Do we want to switch behavior to PlatformNotImplementedException?

[maryamariyan]

In corert we have NotImplementedException (refer to this).

Do we want to switch behavior to PlatformNotImplementedException?

[jkotas]

It would be nice to continue to throw the same exception.

[maryamariyan]

@jkotas some of the APIs (shown in the list below) are only async in corert and not as such in coreclr.

public override async Task FlushAsync(CancellationToken cancellationToken)
public override async Task<int> ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)
public override async ValueTask<int> ReadAsync(Memory<byte> destination, CancellationToken cancellationToken)
public override async Task WriteAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)
public override async ValueTask WriteAsync(ReadOnlyMemory<byte> source, CancellationToken cancellationToken)

[stephentoub]

@jkotas some of the APIs (shown in the list below) are only async in corert and not as such in coreclr.

async in the signature line isn't actually part of the signature; it's an implementation detail.

[maryamariyan]

Other than the exception type comment above the rest looking LGTM.

Note 1: As of this PR we knowingly decided to pick implementation of coreclr for the below APIs:

public virtual IAsyncResult BeginRead(byte[] buffer, int offset, int count, AsyncCallback callback, Object state)
public virtual int EndRead(IAsyncResult asyncResult)
public virtual Task<int> ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)
public virtual IAsyncResult BeginWrite(byte[] buffer, int offset, int count, AsyncCallback callback, Object state)
public virtual void EndWrite(IAsyncResult asyncResult)
public virtual Task WriteAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)

public override Task FlushAsync(CancellationToken cancellationToken)
public override Task<int> ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)
public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
public override Task WriteAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)
public override ValueTask WriteAsync(ReadOnlyMemory<byte> buffer, CancellationToken cancellationToken = default)

For example in corert, below shows how two APIs will be overwritten by coreclr:

#pragma warning disable 1998 // async method with no await
            public override async Task<int> ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)
            {
                cancellationToken.ThrowIfCancellationRequested();
                return 0;
            }

            public override async ValueTask<int> ReadAsync(Memory<byte> destination, CancellationToken cancellationToken)
            {
                cancellationToken.ThrowIfCancellationRequested();
                return 0;
            }
#pragma warning restore 1998

To be overwritten with

            public override Task<int> ReadAsync(Byte[] buffer, int offset, int count, CancellationToken cancellationToken)
            {
                return AsyncTaskMethodBuilder<int>.s_defaultResultTask;
            }

            public override ValueTask<int> ReadAsync(Memory<byte> buffer, CancellationToken cancellationToken = default)
            {
                return new ValueTask<int>(0);
            }

Note 2: The only other diff is that in corert we had the below 4 Debug.Assert statements in
private async Task CopyToAsyncInternal(Stream destination, int bufferSize, CancellationToken cancellationToken) that will be removed as of this PR (refer to here):

            Debug.Assert(destination != null);
            Debug.Assert(bufferSize > 0);
            Debug.Assert(CanRead);
            Debug.Assert(destination.CanWrite);

[maryamariyan]

@jkotas for some public APIs the argument names will be changed for corert. That should be fine, right?

[jkotas]

Right, assuming they are being changed to match the reference assemblies in corefx. The API compat checks should catch mismatched argument names. I am not sure why it is not happening.

[jkotas]

Thanks!