Expose and roll out ValueTask extensibility #27497

stephentoub · 2018-02-27T03:43:37Z

This commit does several things:

Exposes the new ValueTask extensibility model being added in coreclr. The ValueTask-related files will separately be mirrored over to corefx to enable the netstandard build of System.Threading.Tasks.Extensions.
Adapts all Stream-derived types to return ValueTask instead of Task from WriteAsync.
Changes the new WebSocket SendAsync method to return ValueTask instead of Task, and updates the ManagedWebSocket implementation accordingly. Most SendAsyncs on ManagedWebSocket should now return a ValueTask that's either completed synchronously (no allocation) or using a pooled object. It now uses the underlying transport's new WriteAsync overload that returns ValueTask.
Switches more uses of ReadAsync and WriteAsync over to the new overloads, including in Process, DeflateStream, BrotliStream, File, HttpClient, SslStream, WebClient, BufferedStream, CryptoStream,
Removed some unnecessary array clearing from various routines using ArrayPool (after the clearing was added we changed our minds and decided clearing was only necessary in very specific circumstances)
Implements a custom IValueTaskSource in Socket, such that async receives and sends become allocation-free (ammortized). NetworkStream then inherits this functionality, such that its new ReadAsync and WriteAsync are also allocation-free (in the unbounded channel implementations; we can subsequently add it in for bounded).
Implements a custom IValueTaskSource in System.Threading.Channels, such that reading and writing are ammortized allocation-free up to one concurrent reader and writer.
A few random things I noticed as I was going through, e.g. missing ConfigureAwait, some incorrect synchronization in tests, etc.
Adds a ton of new tests, mainly in System.Threading.Tasks.Extensions, System.Threading.Channels, and System.Net.Sockets.

Fixes https://github.com/dotnet/corefx/issues/27445
Depends on dotnet/coreclr#16618

stephentoub · 2018-02-28T06:24:36Z

cc: @KrzysztofCwalina, @davidfowl, @geoffkizer, @kouvel, @benaadams, @tarekgh, @anurse

geoffkizer · 2018-02-28T11:26:59Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

@@ -838,7 +838,7 @@ private void WriteToBuffer(ReadOnlyMemory<byte> source)
            }
        }

-        private Task WriteWithoutBufferingAsync(ReadOnlyMemory<byte> source)
+        private ValueTask WriteWithoutBufferingAsync(ReadOnlyMemory<byte> source)


Why did you change this one? It probably doesn't matter, but I'm just curious.

Nevermind, I see why.

geoffkizer · 2018-02-28T11:28:50Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

@@ -1171,7 +1171,7 @@ private void ReadFromBuffer(Span<byte> buffer)
            Debug.Assert(count <= _readLength - _readOffset);

            if (NetEventSource.IsEnabled) Trace($"Copying {count} bytes to stream.");
-            await destination.WriteAsync(_readBuffer, _readOffset, count, cancellationToken).ConfigureAwait(false);
+            await destination.WriteAsync(new ReadOnlyMemory<byte>(_readBuffer, _readOffset, count), cancellationToken).ConfigureAwait(false);


Nit: could use RemainingBuffer here

RemainingBuffer is new ReadOnlyMemory<byte>(_readBuffer, _readOffset, _readLength - _readOffset) whereas this is new ReadOnlyMemory<byte>(_readBuffer, _readOffset, count). I could do RemainingBuffer.Slice(0, count), but I figured why incur the extra slicing when I could just create the right size to begin with.

analogrelay · 2018-02-28T17:14:27Z

We're not using the WebSocket.SendAsync/ReceiveAsync overloads that take a Memory<byte> yet, but thanks for the heads-up!

analogrelay

WebSocket changes LGTM

davidfowl · 2018-02-28T17:18:43Z

We're not using the WebSocket.SendAsync/ReceiveAsync overloads that take a Memory yet, but thanks for the heads-up!

Yes we are.

analogrelay · 2018-02-28T18:19:47Z

Yes we are.

Oh right, you've been changing my code :)

geoffkizer · 2018-03-01T08:06:38Z

src/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs

-                        t = lastTask != null && lastTask.Result == bytesTransferred ?
-                            lastTask :
-                            (saea._successfullyCompletedTask = Task.FromResult(bytesTransferred));
+                        t = Task.FromResult(bytesTransferred);


Update comment here

geoffkizer · 2018-03-01T08:24:22Z

src/System.Net.Sockets/tests/FunctionalTests/System.Net.Sockets.Tests.csproj

@@ -119,5 +119,8 @@
  <ItemGroup>
    <EmbeddedResource Include="Resources\$(AssemblyName).rd.xml" />
  </ItemGroup>
+  <ItemGroup>


VS had been adding these automatically. I revert then when I remember to.

geoffkizer · 2018-03-01T08:35:46Z

...tem.Net.WebSockets.WebSocketProtocol/src/System/Net/WebSockets/ManagedWebSocketExtensions.cs

@@ -49,6 +49,32 @@ internal static Task<int> ReadAsync(this Stream stream, Memory<byte> destination
                }
            }
        }
+
+        internal static ValueTask WriteAsync(this Stream stream, ReadOnlyMemory<byte> source, CancellationToken cancellationToken = default)


These extension methods seem really weird to me, but I guess they already exist (at least for Read), so that's okay, I suppose? What exactly is going on here?

What exactly is going on here?

We have the ManagedWebSocket.cs implementation that's compiled into multiple assemblies. It's compiled into System.Net.WebSockets.dll, which is part of netcoreapp and can always use all the latest bells and whistles, including the new Read/WriteAsync overloads on Stream. It's also copiled into System.Net.WebSockets.WebSocketProtocol.dll, which is an OOB assembly we release specifically to target netstandard2.0, and in particular so that ASP.NET can use ManagedWebSocket whether targeting netstandard2.0 or netcoreapp. When built in this configuration, ManagedWebSocket can't use the new Stream overloads, so we have two choices: either use ifdef'ing in ManagedWebSocket to have two different implementations for the parts of the code where we use the new functionality, or use partial classes to provide polyfills when building for netstandard. We chose the latter, and that's what these are, implementations of the new APIs, with the same signature, but that aren't actually overloads and are instead just static extensions with the same signature otherwise. The implementation is essentially the same as what the base Stream implementation provides in netcoreapp: try to extract the array if there is one and then just delegate to the array overload, or fall back to renting an array, copying data, and using the array-based overload.

Thanks, that makes sense.

geoffkizer · 2018-03-01T08:39:48Z

src/System.Runtime/ref/System.Runtime.cs

@@ -6337,6 +6337,18 @@ public sealed partial class AsyncStateMachineAttribute : System.Runtime.Compiler
    {
        public AsyncStateMachineAttribute(System.Type stateMachineType) : base (default(System.Type)) { }
    }
+    public partial struct AsyncValueTaskMethodBuilder


Maybe a stupid question, but why are all these structs partial?

I don't know; the ref generator does it this way. I presume to make it easier to have all of the core code in one file, and then have refs for different platforms that add additional APIs.

geoffkizer · 2018-03-01T08:52:38Z

I looked at everything except the Channels stuff, and aside from a couple minor issues/questions above, it all looks good to me.

I can look at Channels tomorrow if that would be helpful.

stephentoub · 2018-03-01T14:54:19Z

I can look at Channels tomorrow if that would be helpful.

If you have the time, thanks.

tarekgh · 2018-03-01T18:15:00Z

@stephentoub did you run the Channels stress test with your changes here? if not, would be good to try it too.

geoffkizer · 2018-03-01T18:29:34Z

src/System.Threading.Channels/src/System/Threading/Channels/AsyncOperation.cs

+                    return ValueTaskSourceStatus.Pending;
+
+                case States.CompletionSet:
+                case States.Released:


Why isn't Released an error? It seems like there's nothing good that can come from accessing the object in this state, right?

I can probably make it one now. Previously when we just had IsCompleted, I had to roll them together based on an implementation detail elsewhere, but I should be able to address this now. I will if I can. I believe that branch is now actually effectively dead code.

geoffkizer · 2018-03-01T18:31:08Z

src/System.Threading.Channels/src/System/Threading/Channels/AsyncOperation.cs

+                ThrowIncorrectCurrentIdException();
+            }
+
+            if (!IsCompleted)


This will allow GetResult to succeed if we're in the Released state, right? Do we want to do that?

Or does the _currentId check prevent this? If so, should we assert we're not in released state? Maybe we can't reliably do that.

Or does the _currentId check prevent this?

Right.

If so, should we assert we're not in released state?

I try to avoid adding asserts for things that are actually triggerable using public APIs. In this case such an assert would even be triggered by our tests that validate an exception is thrown when GetResult is called again.

geoffkizer · 2018-03-01T18:41:58Z

src/System.Threading.Channels/src/System/Threading/Channels/AsyncOperation.cs

+
+        public bool TrySetResult(T result)
+        {
+            if (Interlocked.CompareExchange(ref _state, (int)States.CompletionReserved, (int)States.Owned) == (int)States.Owned)


I'm not entirely clear on why you have the CompletionReserved state. What is this guarding against? I suppose it prevents accidentally overwriting the result if this is called twice, but it seems like you're already in a bad place if TrySetResult is called twice.

Also, why return false here as opposed to throw? What's the user of this supposed to do with a return value of false?

I'm not entirely clear on why you have the CompletionReserved state.

I had a reason, which now escapes me. I'm going to leave it for now, as it's tied in to the whole state machine, but can subsequently look into removing it.

What's the user of this supposed to do with a return value of false?

There can be multiple operations racing to complete one of these if cancellation is involved. The synchronization and bool return is used in coordinating that.

I had a reason, which now escapes me.

Oh, right, it was also because of cancellation. Cancellation races with another completion, the completion is about to store the value, cancellation comes along and completes the operation, the awaiter for that operation wakes up and retrieves the cancellation result, the object goes back to being pooled, another operation comes along and starts using it, and then that original operation writes its result over someone else's object. Before we do any writing, we need to ensure we own the object.

geoffkizer · 2018-03-01T19:05:21Z

src/System.Threading.Channels/src/System/Threading/Channels/AsyncOperation.cs

+        private ExecutionContext _executionContext;
+        private short _currentId;
+
+        public ValueTask CreateNonGenericValueTask() => new ValueTask(this, _currentId);


Naming on these is a little weird. It confused me when I saw it used, but it's really just a trivial thing -- get the ValueTask for this TaskSource. The equivalent on TCS is just .Task, right? Can we just use .ValueTask here, or .[Non]GenericValueTask?

You mean just removing the "Create" and making it a property? Sure. Or, maybe I'll do ValueTask and ValueTaskOfT, or something like that.

geoffkizer · 2018-03-01T19:06:28Z

src/System.Threading.Channels/src/System/Threading/Channels/AsyncOperation.cs

+        }
+    }
+
+    internal abstract class ResettableValueTaskSource<T> : ResettableValueTaskSource, IValueTaskSource, IValueTaskSource<T>


Personally I prefer seeing different classes for void/T. I find it less confusing to use in practice.

geoffkizer · 2018-03-01T19:13:34Z

src/System.Threading.Tasks.Extensions/tests/ManualResetValueTaskSource.cs

+        }
+    }
+
+    internal sealed class ManualResetValueTaskSource<T> : IValueTaskSource<T>, IValueTaskSource


This seems pretty close to a general purpose ValueTaskSource :)

It is, but for a specific set of scenarios. It wouldn't work, for example, in a situation where you need a different base type (e.g. SocketAsyncEventArgs), or where you effectively want it auto-reset once the last user is done with it (e.g. as in AsyncOperation), or other such things.

When you say "need a different base type" I assume you mean "want to avoid an extra allocation by implementing IValueTaskSource on an existing class". Right?

It's nice that we did this with internally in Sockets, but honestly, I question whether it really matters. The whole point of both SAEA and IValueTaskSource is that they should be reused, ideally many, many times. So the allocation cost is amortized and thus insignificant.

That's partly why I would really like to see some sort of general purpose class available. Sure, if the extra-super-optimized case you would need a struct, and delegate the interface impl to it, etc, etc. But in most real world cases, I'm very skeptical that the extra alloc matters -- especially when compared to what users are probably doing today, which is allocating a TaskCompletionSource per operation.

I assume you mean "want to avoid an extra allocation by implementing IValueTaskSource on an existing class". Right?

Yes.

The whole point of both SAEA and IValueTaskSource is that they should be reused, ideally many, many times.

I would say for IValueTaskSource that they "can" be reused many, many times. In channels, for example, you can have many producers and consumers all accessing a channel concurrently. The most value for the least effort comes from caching a single IValueTaskSource instance and reusing that one, but when multiple producers or multiple consumers are all partying at the same time, only one of each is getting the reusable one, and the rest are all getting throwaways. We could pool more, of course, but the management of that isn't free, and adds complexity. So I do expect these to be used in non-reuse scenarios and I still want them to be as cheap as possible then. I could of course have two different implementations for the two different cases, but that also adds complexity.

That's partly why I would really like to see some sort of general purpose class available.

No one is arguing with that. But even if you take the base task piece away, there are still differences in semantics that leads to needing to figure out which scenarios you care about and what knobs you want to expose. This test ManualResetValueTaskSource wouldn't actually work as the implementation for any of the ones in the product because, well, they all automatically reset rather than manually resetting.

geoffkizer · 2018-03-01T19:14:12Z

src/System.Threading.Tasks.Extensions/tests/ManualResetValueTaskSource.cs

+
+        public void Reset()
+        {
+            _completed = false;


Should this throw if it hasn't been consumed?

It could. This method is never actually called currently :)

geoffkizer · 2018-03-01T19:19:38Z

A few comments/questions above, but nothing serious. Overall this looks great.

stephentoub · 2018-03-01T21:10:08Z

did you run the Channels stress test with your changes here?

Yup. Thanks.

ahsonkhan · 2018-03-01T21:18:09Z

src/Common/src/CoreLib/System/Threading/Tasks/IValueTaskSource.cs

@@ -0,0 +1,83 @@
+// Licensed to the .NET Foundation under one or more agreements.


Would there be mirroring issues if you include it here while it is already in coreclr? cc @Anipik

Also the path here is Threading/Tasks/IValueTaskSource.cs but in coreclr it was Threading/Tasks/Sources/IValueTaskSource.cs

This is a stray file and should be deleted. The file has already mirrored to corefx, it's just at a different path; this is an old copy at a different location that somehow snuck in.

ahsonkhan · 2018-03-01T21:28:52Z

src/System.IO.FileSystem/src/System/IO/File.cs

@@ -857,7 +856,7 @@ public static Task WriteAllBytesAsync(string path, byte[] bytes, CancellationTok

            using (FileStream fs = new FileStream(path, FileMode.Create, FileAccess.Write, FileShare.Read, DefaultBufferSize, FileOptions.Asynchronous | FileOptions.SequentialScan))
            {
-                await fs.WriteAsync(bytes, 0, bytes.Length, cancellationToken).ConfigureAwait(false);
+                await fs.WriteAsync(new ReadOnlyMemory<byte>(bytes, 0, bytes.Length), cancellationToken).ConfigureAwait(false);


Can we use the single arg constructor here instead so it avoids unnecessary checks? Anywhere we have array, 0, array.Length, we can simply pass array.

Yup, will change. I did that in most places, but missed this one.

ahsonkhan · 2018-03-01T21:37:05Z

src/System.IO.Compression/src/System/IO/Compression/DeflateZLib/DeflateStream.cs

@@ -801,8 +798,7 @@ public CopyToAsyncStream(DeflateStream deflateStream, Stream destination, int bu
                    int bytesRead = _deflateStream._inflater.Inflate(_arrayPoolBuffer, 0, _arrayPoolBuffer.Length);


Use the span based overload here to avoid passing in 0, length.

bartonjs · 2018-03-01T21:40:17Z

src/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs

@@ -238,7 +238,7 @@ public override void WriteByte(byte value)
        public override int Read(byte[] buffer, int offset, int count)
        {
            CheckReadArguments(buffer, offset, count);
-            return ReadAsyncCore(buffer, offset, count, default(CancellationToken), useAsync: false).ConfigureAwait(false).GetAwaiter().GetResult();
+            return ReadAsyncCore(buffer, offset, count, default(CancellationToken), useAsync: false).GetAwaiter().GetResult();


Almost everwhere else that had ConfigureAwait(false) kept it. But here you removed it.

Since you understand it way better than I do, I'll trust you if removing it is right, but I wanted to call it out for confirmation.

ConfigureAwait is only relevant when await is being used, as it influences which awaiter is used and thus how its OnCompleted behaves. Here we're just synchronously blocking waiting for the operation to complete, so ConfigureAwait is a nop.

ahsonkhan · 2018-03-01T21:42:18Z

src/System.Net.Sockets/src/System.Net.Sockets.csproj

@@ -410,6 +410,7 @@
    <Reference Include="System.Security.Claims" />
    <Reference Include="System.Security.Principal.Windows" />
    <Reference Include="System.Threading" />
+    <Reference Include="System.Threading.ThreadPool" />


nit: sort ordering

ahsonkhan · 2018-03-01T21:44:55Z

src/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs

            /// <summary>Whether exceptions that emerge should be wrapped in IOExceptions.</summary>
            internal bool _wrapExceptionsInIOExceptions;
        }
+
+        /// <summary>A SocketAsyncEventArgs that can be awaited to get the result of an operation.</summary>
+        internal sealed class AwaitableSocketAsyncEventArgs : SocketAsyncEventArgs, IValueTaskSource, IValueTaskSource<int>


Is this class a copy/paste from src/System.Net.Sockets/src/System/Net/Sockets/NetworkStream.cs or are there changes here we should review?

The one from NetworkStream was moved here, then augmented with the interfaces. I removed it from NetworkStream along with its CopyToAsync override, as with these changes and with the base Stream.CopyToAsync being updated to use the new overloads, the overridden CopyToAsync wasn't adding enough additional value to be worth the extra code/complexity.

ahsonkhan · 2018-03-01T21:55:10Z

src/System.Net.Sockets/src/System/Net/Sockets/Socket.Tasks.cs

+            /// <summary>Gets the result of the completion operation.</summary>
+            /// <returns>Number of bytes transferred.</returns>
+            /// <remarks>
+            /// Unlike Task's awaiter's GetResult, this does not block until the operation completes: it must only


nit: Task's awaiter's GetResult is difficult to read.

ahsonkhan · 2018-03-01T21:55:46Z

src/System.Net.Sockets/tests/FunctionalTests/NetworkStreamTest.netcoreapp.cs

+        {
+            await RunWithConnectedNetworkStreamsAsync(async (server, client) =>
+            {
+                byte[] writeBuffer = new byte[10 * 1024 * 1024];


nit: use var

ahsonkhan · 2018-03-01T22:04:01Z

src/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs

@@ -18,6 +18,7 @@ public UnixDomainSocketTest(ITestOutputHelper output)
            _log = TestLogging.GetInstance();
        }

+        [ActiveIssue(27542)]
        [OuterLoop] // TODO: Issue #11345


This issue has been closed, update the comment? https://github.com/dotnet/corefx/issues/11345
Do we need to point it to this one now? https://github.com/dotnet/corefx/issues/14519

cc @davidsh

11345

There are tons of references to this still around the repo. They should be cleaned up, but I'm not going to do that in this change.

The test should still be left as outerloop.

ahsonkhan · 2018-03-01T22:07:03Z

...tem.Net.WebSockets.WebSocketProtocol/src/System/Net/WebSockets/ManagedWebSocketExtensions.cs

+            {
+                byte[] buffer = ArrayPool<byte>.Shared.Rent(source.Length);
+                source.Span.CopyTo(buffer);
+                return new ValueTask(FinishWriteAsync(stream.WriteAsync(buffer, 0, source.Length, cancellationToken), buffer));


Is buffer.Length == source.Length here?
Why not use the RoM overload of WriteAsync instead?

As noted above, this code has to also run in a netstandard2.0 assembly (consumed by ASP.NET Core in particular).

Is buffer.Length == source.Length here?

Not necessarily. buffer.Length >= source.Length. The ArrayPool could have returned a longer array.

Why not use the RoM overload of WriteAsync instead?

This is the implementation of that "overload". See #27497 (comment).

Not necessarily. buffer.Length >= source.Length. The ArrayPool could have returned a longer array.

Got it.

This is the implementation of that "overload".

Lol, that's right. I didn't connect the dots.

ahsonkhan · 2018-03-01T22:14:34Z

src/System.Threading.Channels/src/System.Threading.Channels.csproj

@@ -37,6 +37,7 @@
    <Reference Include="System.Runtime" />
    <Reference Include="System.Runtime.Extensions" />
    <Reference Include="System.Threading" />
+    <Reference Include="System.Threading.ThreadPool" />


nit: sort order, here and above.

ahsonkhan · 2018-03-01T22:17:12Z

src/System.Threading.Channels/src/System/Threading/Channels/AsyncOperation.cs

+            }
+
+            ExceptionDispatchInfo error = _error;
+            T result = _result;


Why do we store the _result in a local before returning it? Same with error.

Edit: Is it because after the _state changes, the value could change?

Because once _state is set to Released below, someone else can immediately start using this object, so we need the state captured locally.

Is it because after the _state changes, the value could change?

Right

ahsonkhan · 2018-03-01T22:31:36Z

src/System.Threading.Tasks.Extensions/tests/ValueTaskTests.cs

        {
-            Task<int> source = Task.Delay(1).ContinueWith(_ => 42);
-            ValueTask<int> t = new ValueTask<int>(source);
+            ValueTask<int> t = new ValueTask<int>(ManualResetValueTaskSource.Completed<int>(42, null), 0);


nit: use var

ahsonkhan · 2018-03-01T22:33:42Z

src/System.Threading.Tasks.Extensions/tests/AsyncValueTaskMethodBuilderTests.cs

+            var dsm = new DelegateStateMachine();
+            TaskAwaiter<int> t = new TaskCompletionSource<int>().Task.GetAwaiter();
+
+            Assert.InRange(numAwaits, 1, int.MaxValue);


What's the point of this check?

This is copy-and-paste of the generic one, which is from a while ago. I assume I was thinking I wanted to ensure that the input test data made sense, so that if the test then failed, it wasn't because of bad inputs.

ahsonkhan

LGTM, modulo nits

This commit does several things: - Exposes the new `ValueTask` extensibility model being added in coreclr. The ValueTask-related files will separately be mirrored over to corefx to enable the netstandard build of System.Threading.Tasks.Extensions. - Adapts all `Stream`-derived types to return `ValueTask` instead of `Task` from `WriteAsync`. - Changes the new `WebSocket` `SendAsync` method to return `ValueTask` instead of `Task`, and updates the `ManagedWebSocket` implementation accordingly. Most `SendAsync`s on `ManagedWebSocket` should now return a `ValueTask` that's either completed synchronously (no allocation) or using a pooled object. It now uses the underlying transport's new `WriteAsync` overload that returns `ValueTask`. - Switches more uses of `ReadAsync` and `WriteAsync` over to the new overloads, including in Process, DeflateStream, BrotliStream, File, HttpClient, SslStream, WebClient, BufferedStream, CryptoStream, - Removed some unnecessary array clearing from various routines using ArrayPool (after the clearing was added we changed our minds and decided clearing was only necessary in very specific circumstances) - Implements a custom `IValueTaskSource` in Socket, such that async receives and sends become allocation-free (ammortized). `NetworkStream` then inherits this functionality, such that its new `ReadAsync` and `WriteAsync` are also allocation-free (in the unbounded channel implementations; we can subsequently add it in for bounded). - Implements a custom `IValueTaskSource` in System.Threading.Channels, such that reading and writing are ammortized allocation-free up to one concurrent reader and writer. - A few random things I noticed as I was going through, e.g. missing ConfigureAwait, some incorrect synchronization in tests, etc. - Adds a ton of new tests, mainly in System.Threading.Tasks.Extensions, System.Threading.Channels, and System.Net.Sockets.

stephentoub · 2018-03-02T14:57:20Z

Cherry-picked into #27640 (with a few additional fixes added there)

ahsonkhan assigned stephentoub Feb 28, 2018

stephentoub force-pushed the valuetaskextensibility branch from e4a7364 to a341b2e Compare February 28, 2018 06:21

stephentoub changed the title ~~[WIP] corefx changes for ValueTask extensibility~~ Expose and roll out ValueTask extensibility Feb 28, 2018

geoffkizer reviewed Feb 28, 2018

View reviewed changes

analogrelay reviewed Feb 28, 2018

View reviewed changes

stephentoub mentioned this pull request Feb 28, 2018

Update WebSocket status when the underlying connection is closed #27556

Merged

stephentoub force-pushed the valuetaskextensibility branch 2 times, most recently from 34a565f to ccfd967 Compare March 1, 2018 04:10

geoffkizer reviewed Mar 1, 2018

View reviewed changes

stephentoub force-pushed the valuetaskextensibility branch from ccfd967 to 76e9044 Compare March 1, 2018 15:55

joshfree added this to the 2.1.0 milestone Mar 1, 2018

joshfree requested review from tarekgh, kouvel, ahsonkhan, a user and GrabYourPitchforks March 1, 2018 18:10

tarekgh approved these changes Mar 1, 2018

View reviewed changes

geoffkizer reviewed Mar 1, 2018

View reviewed changes

ahsonkhan reviewed Mar 1, 2018

View reviewed changes

stephentoub force-pushed the valuetaskextensibility branch from 76e9044 to 1e3f404 Compare March 1, 2018 21:22

ahsonkhan reviewed Mar 1, 2018

View reviewed changes

bartonjs reviewed Mar 1, 2018

View reviewed changes

ahsonkhan reviewed Mar 1, 2018

View reviewed changes

ahsonkhan approved these changes Mar 1, 2018

View reviewed changes

stephentoub force-pushed the valuetaskextensibility branch from 1e3f404 to 276d656 Compare March 2, 2018 00:49

stephentoub force-pushed the valuetaskextensibility branch from 276d656 to 9152f9f Compare March 2, 2018 01:51

stephentoub closed this Mar 2, 2018

stephentoub mentioned this pull request Mar 22, 2018

Proposal: Allow [AsyncMethodBuilder(...)] on methods dotnet/csharplang#1407

Open

stephentoub deleted the valuetaskextensibility branch May 2, 2018 03:03

		@@ -0,0 +1,83 @@
		// Licensed to the .NET Foundation under one or more agreements.

		@@ -801,8 +798,7 @@ public CopyToAsyncStream(DeflateStream deflateStream, Stream destination, int bu
		int bytesRead = _deflateStream._inflater.Inflate(_arrayPoolBuffer, 0, _arrayPoolBuffer.Length);

Expose and roll out ValueTask extensibility #27497

Expose and roll out ValueTask extensibility #27497

Conversation

stephentoub commented Feb 27, 2018 • edited Loading

stephentoub commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

analogrelay commented Feb 28, 2018

analogrelay left a comment

Choose a reason for hiding this comment

davidfowl commented Feb 28, 2018

analogrelay commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Mar 1, 2018

stephentoub commented Mar 1, 2018

tarekgh commented Mar 1, 2018

Choose a reason for hiding this comment

stephentoub Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Mar 1, 2018

stephentoub commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Feb 27, 2018 •

edited

Loading

stephentoub Mar 1, 2018 •

edited

Loading

stephentoub Mar 1, 2018 •

edited

Loading

stephentoub Mar 1, 2018 •

edited

Loading

ahsonkhan Mar 1, 2018 •

edited

Loading

stephentoub Mar 1, 2018 •

edited

Loading

ahsonkhan Mar 1, 2018 •

edited

Loading

ahsonkhan Mar 1, 2018 •

edited

Loading

ahsonkhan Mar 1, 2018 •

edited

Loading