Move the write side of SslStream to AsyncAwait #23606

Drawaes · 2017-08-28T15:00:44Z

BUILDS on the removal of pinnable buffers so can't be merged until then..... (#23572)

Changes the write side to async/await. Clears out a bunch of cruft from the APM model.

Will move closer to allowing the SemaphoreSlim unix solution, as well as clarity on the flow that is required.

Is a first step, readside would be next and then the underlying handshake/renegotiation.

Benchmarks and traces to come, but it makes the flow a lot more readable (initial indications is that the aspnet core tests are about 5% faster and with traces I hope to show that is due to lower allocations).

If I had my own way I would convert the sync methods to ReadAsync().GetAwaiter().GetResult() as it is truly async in nature and by not doing that two paths need to be maintained which leads to a higher cognitive load in a security library. That however is a decision that others need to think about.

Cheers

Tim

stephentoub · 2017-08-28T15:39:48Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

@@ -1112,7 +1118,8 @@ private static void PartialFrameCallback(AsyncProtocolRequest asyncRequest)
        //
        private static void ReadFrameCallback(AsyncProtocolRequest asyncRequest)
        {
-            if (NetEventSource.IsEnabled) NetEventSource.Enter(null);
+            if (NetEventSource.IsEnabled)
+                NetEventSource.Enter(null);


There are a bunch of changes to these one line event source calls. Any reason? Across the System.Net libs, we standardized on generally using the single-line syntax, treating the if (NetEventSource.IsEnabled) as a prefixed guard.

FYI, there is a setting in the repo that causes that to happen if you ctrl-k,d which I just checked. It's a bit of a pain because if you ever do it (or if it's muscle memory :P ) then it breaks all that formatting.

Drawaes · 2017-08-28T15:42:06Z

Let me fix that. I didn't touch them but I did ctrl k,d

stephentoub · 2017-08-28T15:42:10Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

+            if (lockState != LockHandshake)
+            {
+                return Task.CompletedTask;
+            }


Nit: blank line before the lock

stephentoub · 2017-08-28T15:44:17Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

+
+                _lockWriteState = LockPendingWrite;
+                TaskCompletionSource<int> completionSource = new TaskCompletionSource<int>();
+                _queuedWriteStateRequest = completionSource;


Rather than allocating a TaskCompletionSource for each of these, consider using AsyncTaskMethodBuilder. It's a struct, so won't require an allocation each time. You just need to make sure that you access its Task property before any other thread could try to SetResult/Exception on it, as Task lazily-initializes the task. You also need to be careful about not copying it before accessing its Task property, as it's a mutable struct.

Oh, I just realized _queuedWriteStateRequest is typed as object rather than as TaskCompletionSource<int>. Nevermind then, since the ATMB would just get boxed anyway.

stephentoub · 2017-08-28T15:47:52Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

+                default:
+                    // Async write is pending, start it on other thread.
+                    // Consider: we could start it in on this thread but that will delay THIS handshake completion
+                    ThreadPool.QueueUserWorkItem(new WaitCallback(CompleteRequestWaitCallback), obj);


Can we cache this delegate rather than allocating it on each write that pends?

We could you mean that last delegate? Its the original code move to pattern matching and is super infrequent. But I will change

Right, I meant the WaitCallback.

stephentoub · 2017-08-28T15:47:59Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

+                    return;
+                case TaskCompletionSource<int> completionSource:
+                    //Async write is pending, should it be started on another thread?
+                    //Or is it fine to do it here


What does this comment mean?

Basically below they do a task.new () wonder if I should do similar?

Oh, you mean do you need to queue the completion of it? That depends: might there be continuations off of this that we don't want to run as part of this call? If we do want to offload it, there are two options: if you only want to sometimes offload it, you can just queue the call to SetResult, as is done below. But if you always want to make sure that continuations run asynchronously from the SetResult call, you can pass TaskCreationOptions.RunContinuationsAsynchronously to the TaskCompletionSource's ctor.

I will go with the second one. We don't want the result from the action calling SetResult to be delayed by a write that is pending.

stephentoub · 2017-08-28T15:48:35Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

@@ -1449,6 +1420,33 @@ private void FinishHandshake(Exception e, AsyncProtocolRequest asyncRequest)
            }
        }

+        private void ProcessPendingWrite()
+        {


Do we require being called while holding the lock? If so, that'd be good to assert here. Or if we require that the lock isn't held when this is called, that'd be good to assert, too.

I added an assert, but it didn't register it in Github as changed for some reason, maybe the line numbers moved too much.

Drawaes · 2017-08-28T15:49:13Z

I am happy to do it. Here is my thought process on this. It is only ever hit during a renegotiation and a write is attempted at the same time. The actual lock case is far away from a hotpath so I didn't want to complicate it or risk getting the renegotiation wrong as it's been a source of TLS problems over the years. Infact http/2 does not allow it's use at all.

So I am happy to change it but I wonder if simplicity is better in this case?

stephentoub · 2017-08-28T15:52:28Z

src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs

-                throw new InvalidOperationException(SR.Format(SR.net_io_invalidendcall, "EndWrite"));
+                try
+                {
+                    await task;


.ConfigureAwait(false) on all awaits

stephentoub · 2017-08-28T15:55:02Z

src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs

+        {
+            // Request a write IO slot.
+            Task ioSlot = _sslState.CheckEnqueueWriteAsync();
+            if (!ioSlot.IsCompleted)


Is it possible ioSlot could complete as faulted rather than successfully? If so, I'd suggest making this IsCompletedSuccessfully rather than IsCompleted. That won't hurt the fast case, and if there is a chance it could fail, it'll ensure the exception is propagated rather than dropped, as we'll call WaitForWriteIOSlot which will await it.

It would only do so if someone was mixing both a sync renegotiation and an async write. Can change but iscompletedsucessfully .net desktop or core only?

IsCompletedSuccessfully is currently only in .NET Core. But then, so is all of this code, no?

Honestly the interaction between corefx -> core -> AOT -> desktop confuses me, if it's core only that's cool :) I only really code in that these days anyway.

stephentoub · 2017-08-28T15:55:31Z

src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs

+                ProtocolToken message = new ProtocolToken(null, status);
+                ArrayPool<byte>.Shared.Return(rentedBuffer);
+                throw new IOException(SR.net_io_encrypt, message.GetException());
+            }


Nit: add blank line after closing brace

stephentoub · 2017-08-28T15:57:41Z

src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs

+            {
+                try
+                {
+                    await t;


Did you mean writeTask?

stephentoub · 2017-08-28T15:58:43Z

src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs

+            }
+        }
+
+        internal async Task WriteAsyncChunked(byte[] buffer, int offset, int count, CancellationToken cancellationToken)


Nit: please use an Async suffix on all async methods

stephentoub · 2017-08-28T16:01:34Z

src/System.Net.Security/src/System/Net/Security/SslStream.cs

+
+        public override Task WriteAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken)
+        {
+            return _sslState.SecureStream.WriteAsync(buffer, offset, count, cancellationToken);


Does the wrapped stream's WriteAsync do argument validation, or does that need to be done here?

Argument validation is done in sslstreaminternal

stephentoub · 2017-08-28T16:12:26Z

So I am happy to change it but I wonder if simplicity is better in this case?

Which comment is this in response to?

Drawaes · 2017-08-28T16:49:57Z

It was the TCS but you changed your mind anyway :)

Drawaes · 2017-08-28T18:20:35Z

Right I have made all changes I believe.

Drawaes · 2017-08-28T23:14:54Z

@dotnet-bot Test Outerloop Windows x64 Debug Build
@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot Test Outerloop Linux x64 Release Build

…and open for future changes

stephentoub · 2017-08-30T12:49:23Z

Benchmarks and traces to come

Any progress on that?

stephentoub · 2017-08-30T12:50:21Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

@@ -21,6 +21,7 @@ internal class SslState
        private static AsyncProtocolCallback s_partialFrameCallback = new AsyncProtocolCallback(PartialFrameCallback);
        private static AsyncProtocolCallback s_readFrameCallback = new AsyncProtocolCallback(ReadFrameCallback);
        private static AsyncCallback s_writeCallback = new AsyncCallback(WriteCallback);
+        private static WaitCallback s_completeWaitCallback = new WaitCallback(CompleteRequestWaitCallback);


Nit: it'd be nice if this delegate and the ones above it were readonly

stephentoub · 2017-08-30T12:50:33Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

-        internal bool CheckEnqueueWrite(AsyncProtocolRequest asyncRequest)
+        internal Task CheckEnqueueWriteAsync()
+        {
+            //Clear previous request.


Nit: space after //

stephentoub · 2017-08-30T12:52:37Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

+            {
+                if (_lockWriteState != LockHandshake)
+                {
+                    CheckThrow(true);


Nit: can we name the argument in the call? I don't know what "true" means here.

stephentoub · 2017-08-30T12:55:21Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

+
+            lock (this)
+            {
+                if (_lockWriteState != LockHandshake)


Is there a race condition here? I realize this predates your PR, but it looks strange. Outside of the lock the field is being manipulated by an interlocked, and then here there's a check protected by a lock, but that lock and interlocked have nothing to do with each other... could another thread be doing that interlocked operation at the same time we're checking/setting _lockWriteState here?

stephentoub · 2017-08-30T12:59:27Z

src/System.Net.Security/src/System/Net/Security/SslState.cs

@@ -1449,6 +1430,34 @@ private void FinishHandshake(Exception e, AsyncProtocolRequest asyncRequest)
            }
        }

+        private void ProcessPendingWrite()
+        {
+            Debug.Assert(_lockWriteState == LockPendingWrite || _lockWriteState == LockHandshake);


Thanks for adding the assert. This is good to have, but I'd actually been asking whether we can/should add an assert like:

Debug.Assert(Monitor.IsEntered(this));

geoffkizer · 2017-08-30T22:11:50Z

Here's a different approach that might allow us to maintain a single codepath for sync and async. The idea is to hide the sync/async behind a common (async) interface, and await this as usual, but then genericize the implementation methods via a struct that implements this interface. Thoughts?

        interface IStreamHolder
        {
            ValueTask<int> ReadAsync(ArraySegment<byte> buffer);
            Task WriteAsync(ArraySegment<byte> buffer);
        }

        struct StreamSyncHolder : IStreamHolder
        {
            Stream _stream;

            public StreamSyncHolder(Stream stream)
            {
                _stream = stream;
            }

            public ValueTask<int> ReadAsync(ArraySegment<byte> buffer)
            {
                return new ValueTask<int>(_stream.Read(buffer.Array, buffer.Offset, buffer.Count));
            }

            public Task WriteAsync(ArraySegment<byte> buffer)
            {
                _stream.Write(buffer.Array, buffer.Offset, buffer.Count);
                return Task.CompletedTask;
            }
        }

        struct StreamAsyncHolder : IStreamHolder
        {
            Stream _stream;

            public StreamAsyncHolder(Stream stream)
            {
                _stream = stream;
            }

            public ValueTask<int> ReadAsync(ArraySegment<byte> buffer)
            {
                return new ValueTask<int>(_stream.ReadAsync(buffer.Array, buffer.Offset, buffer.Count));
            }

            public Task WriteAsync(ArraySegment<byte> buffer)
            {
                return _stream.WriteAsync(buffer.Array, buffer.Offset, buffer.Count);
            }
        }

        static void DoIt(Stream s)
        {
            DoItInternalAsync(new StreamSyncHolder(s)).GetAwaiter().GetResult();
        }

        static Task DoItAsync(Stream s)
        {
            return DoItInternalAsync(new StreamAsyncHolder(s));
        }

        static async Task DoItInternalAsync<T>(T streamHolder) where T : IStreamHolder
        {
            byte[] buffer = System.Text.Encoding.ASCII.GetBytes("Hello world!\n");

            await streamHolder.WriteAsync(new ArraySegment<byte>(buffer, 0, buffer.Length));
            await streamHolder.WriteAsync(new ArraySegment<byte>(buffer, 0, buffer.Length));
            await streamHolder.WriteAsync(new ArraySegment<byte>(buffer, 0, buffer.Length));
        }

        static void Main(string[] args)
        {
            var s = Console.OpenStandardOutput();

            DoIt(s);
            DoItAsync(s).Wait();
        }

Drawaes · 2017-08-30T22:22:56Z

Are we paying for an interface dispatch now in this? Also I am not 100% sure this will solve the issue. The hot path needs to "skip" all the awaiting and just get on with it (the hotpath for async that is). It might work if you then did a GetAwaiter().GetResult() on the result and let the read/write non async flow through the IsComplete path. I would say, this should potentially be a follow up, to reduce this.

Drawaes · 2017-08-30T22:26:07Z

Follow up PR I mean.

Drawaes · 2017-08-30T22:32:24Z

Actually you won't I missed the generic... should work. I like the idea, but would prefer if possible to clear the outstanding issues (and those for the other PR) then I will happily prototype and attempt to crush the sync/async split. Basically my end goals are

Get the semaphore in
Make the code a lot cleaner/simpler/clearer

Number 2 has the effect of any future code changes or improvements should be a lot easier, and have a lower barrier of entry for others in the community to get involved. With that in mind your idea above is a good one for number 2!

benaadams · 2017-08-30T22:41:11Z

Don't need the holder for async?

geoffkizer · 2017-08-30T23:58:03Z

Don't need the holder for async?

If you have a pre-existing interface, then you could use that instead of the holder for async. That's not the case here.

geoffkizer · 2017-08-31T00:25:19Z

src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs

+            if (count <= _sslState.MaxDataSize)
+            {
+                //Single write
+                Task t = WriteSingleChunk(buffer, offset, count, cancellationToken);


WriteSingleChunk can throw. Don't we need to deal with that here? (e.g. clear _nestedWrite)

geoffkizer · 2017-08-31T00:25:55Z

src/System.Net.Security/src/System/Net/Security/SslStreamInternal.cs

+        private async Task WaitForWriteIOSlot(Task lockTask, byte[] buffer, int offset, int count, CancellationToken cancellationToken)
+        {
+            await lockTask.ConfigureAwait(false);
+            await WriteSingleChunk(buffer, offset, count, cancellationToken).ConfigureAwait(false);


Won't this cause us to go do the CheckEnqueueWriteAsync again?

The lock is re-entrant , I will double check the logic, but it keeps the flow simple for the "fastpath". If we actually hit this lock it's a 1 in 100 if that. (Only if there is a renegotiation and we try to write). At that point it's slow anyway as we had to wait for a couple of network round trips so a check of an Interlock it won't really hurt our performance.

The reason it's re-entrant because we set it to "LockWrite" when we want to lock and only check that it isn't "LockHandshake".

Yeah, I don't think the perf matters, just trying to understand the logic.

We should add a commenting explaining this.

:) its a tangled best this lot in general, the more eyes and understanding and the simpler we can make it for those that follow the better!

benaadams · 2017-08-31T01:50:15Z

If you have a pre-existing interface, then you could use that instead of the holder for async. That's not the case here.

Oh... I see what you are doing there. That's twisted. Did something similar in Kestrel, wasn't proud.

Prefer the naming conventions we; shamefully, came up with there:

interface IStreamAdapter
{
}

struct StreamSyncAdapter : IStreamAdapter
{
}

struct StreamAsyncAdapter : IStreamAdapter
{
}

Though is because you can't handle it via compiler dotnet/roslyn#15822 (e.g. using struct to force templating rather than shared generics)

benaadams · 2017-08-31T01:51:06Z

Good call though; will use this pattern in other places.

Drawaes · 2017-08-31T02:12:16Z

It has also sparked some ideas for me :) I had prototyped the read side with the flag as discussed previously (IsAsync) but that interface idea is cleaner for sure.

geoffkizer · 2017-08-31T11:12:50Z

I stole the basic idea here from the C# "shapes" proposal: dotnet/csharplang#164

The interesting thing here is that "shapes" would basically be a lot of syntactic sugar on top of this basic approach (at least as I understand it). So while the syntax above may be a bit icky at times, the approach seems solid to me.

Drawaes · 2017-08-31T11:16:45Z

Nah... you came up with a unique and novel idea with no outside help at all... :) anyway I like it. I am undecided. Do I try to ram it into this PR or do I fix this PR try to get it merged and submit another "merging both flows" PR?

geoffkizer · 2017-08-31T11:20:00Z

If we think we're going to just turn around and change this code again anyway, then I'd prefer to see it all done in one change, personally, instead of making one change and then redoing it significantly.

On the other hand, if we want to get the write code in so we're unblocked on other issues, I'm fine with that too.

geoffkizer · 2017-08-31T11:21:11Z

And btw, "adapter" seems like a good naming convention to me.

Drawaes · 2017-08-31T11:25:29Z

Okay I will change this to remove the sync path. I also have an easy PR to remove that ref outbuffer stuff which I might slip in first to clean up the code a bit.

Drawaes · 2017-08-31T11:26:14Z

Closing for now while I refactor to remove dual sync/async path. Will be resubmitted soonish

Drawaes · 2017-08-31T14:39:14Z

Sorry @geoffkizer and @stephentoub I didn't know where else I could ask this, but I was wondering, if you pin something that is already pinned do you save anytime? like

Pin buffer
do your thing
Call some other class that pins the same object
release pin on buffer

is that any more efficient than

Pin buffer
Do your thing
release pin
Call other class that pins the same object?

stephentoub · 2017-08-31T14:44:44Z

if you pin something that is already pinned do you save anytime?

No. But certain approaches to pinning are more efficient than others. fixed has very low cost unless a GC happens while in that region, whereas GCHandle.Alloc(..., GCHandleType.Pinned) does have overhead. Since our discussion yesterday, I've been prototyping a change to SocketAsyncEventArgs that will only have the buffer pinned while operations are in-progress and that pays for the GCHandle costs only when an operation actually completes asynchronously. Looks promising. I'll vet it some more and, assuming it holds up, I'll likely employ it as part of a switch from byte[] to Memory<T>, as we won't be able to do the reference-equality checks with Memory<T> anyway.

Drawaes · 2017-08-31T14:50:24Z

👍 is there going to be the equiv of ArrayPool for buffer out of the box? Last thing, would be interesting if you get rid of the Pin and hold and merge it, what difference it might make to aspnet's benchmarks because pipelines will almost never give sockets the same buffer over and over. (and they are using sockets now ...)

stephentoub · 2017-08-31T15:09:22Z

is there going to be the equiv of ArrayPool for buffer out of the box?

What are you asking for specifically? Obviously every array that comes out of ArrayPool can be wrapped in a Memory<T>. Are you asking whether there are plans to have a set of already pinned buffers? A pool of native memory wrapped in Memory<T>? Something else? Regardless, @KrzysztofCwalina can speak to it.

Drawaes · 2017-08-31T16:05:45Z

Just that there would be an ArrayPool<Memory> just to avoid the allocation of wrapping the Memory class each time you take a pooled array. Nothing to do with pinning.

stephentoub · 2017-08-31T16:07:38Z

Memory<T> is a struct. There is no allocation required to do new Memory<T>(array);.

Drawaes · 2017-08-31T16:09:38Z

Well not an ArrayPool more of a MemoryPool.Shared

Drawaes · 2017-08-31T16:11:59Z

really? I thought Memory was a rename of Buffer (which was Memory) and Buffer was always a class. I need to keep up I guess ;)

stephentoub · 2017-08-31T16:12:50Z

I thought Memory was a rename of Buffer (which was Memory) and Buffer was always a class

OwnedBuffer<T> (now OwnedMemory<T>) is a class. Buffer<T> (now Memory<T>) is a struct.

dnfclas added the cla-already-signed label Aug 28, 2017

stephentoub reviewed Aug 28, 2017

View reviewed changes

davidsh added the area-System.Net.Security label Aug 28, 2017

karelz assigned Drawaes and geoffkizer Aug 29, 2017

Drawaes added 2 commits August 30, 2017 03:02

Removed the heritage APM from the writeside making the logic clearer …

f119ccd

…and open for future changes

Reacting to review

f9b806b

stephentoub reviewed Aug 30, 2017

View reviewed changes

geoffkizer reviewed Aug 31, 2017

View reviewed changes

Drawaes closed this Aug 31, 2017

Drawaes deleted the SslStreamRemodelDeux branch August 31, 2017 15:05

karelz modified the milestone: 2.1.0 Sep 6, 2017

Move the write side of SslStream to AsyncAwait #23606

Move the write side of SslStream to AsyncAwait #23606

Conversation

Drawaes commented Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Drawaes Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Drawaes commented Aug 28, 2017

Choose a reason for hiding this comment

stephentoub Aug 28, 2017 • 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

stephentoub Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drawaes commented Aug 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Aug 28, 2017

Drawaes commented Aug 28, 2017

Drawaes commented Aug 28, 2017

Drawaes commented Aug 28, 2017

stephentoub commented Aug 30, 2017

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 Aug 30, 2017

Drawaes commented Aug 30, 2017

Drawaes commented Aug 30, 2017

Drawaes commented Aug 30, 2017

benaadams commented Aug 30, 2017

geoffkizer commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drawaes Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Aug 31, 2017

benaadams commented Aug 31, 2017

Drawaes commented Aug 31, 2017 • edited Loading

geoffkizer commented Aug 31, 2017 • edited Loading

Drawaes commented Aug 31, 2017

geoffkizer commented Aug 31, 2017

geoffkizer commented Aug 31, 2017

Drawaes commented Aug 31, 2017

Drawaes commented Aug 31, 2017

Drawaes commented Aug 31, 2017

stephentoub commented Aug 31, 2017 • edited Loading

Drawaes commented Aug 31, 2017 • edited Loading

stephentoub commented Aug 31, 2017

Drawaes commented Aug 31, 2017

stephentoub commented Aug 31, 2017 • edited Loading

Drawaes commented Aug 31, 2017

Drawaes commented Aug 31, 2017

stephentoub commented Aug 31, 2017 • edited Loading

Drawaes commented Aug 28, 2017 •

edited

Loading

Drawaes Aug 28, 2017 •

edited

Loading

stephentoub Aug 28, 2017 •

edited

Loading

stephentoub Aug 28, 2017 •

edited

Loading

Drawaes Aug 31, 2017 •

edited

Loading

Drawaes commented Aug 31, 2017 •

edited

Loading

geoffkizer commented Aug 31, 2017 •

edited

Loading

stephentoub commented Aug 31, 2017 •

edited

Loading

Drawaes commented Aug 31, 2017 •

edited

Loading

stephentoub commented Aug 31, 2017 •

edited

Loading

stephentoub commented Aug 31, 2017 •

edited

Loading