Modified Dns.GetHostAddressesAsync to be truly async #26850

JeffCyr · 2018-02-05T14:53:26Z

PR for issue dotnet/runtime#17224 (originally: https://github.com/dotnet/corefx/issues/8371)

Modified Dns.GetHostAddressesAsync to use GetAddrInfoEx when supported.

dnfclas · 2018-02-05T14:53:37Z

All CLA requirements met.

stephentoub · 2018-02-05T15:02:11Z

src/Common/src/Interop/Windows/Winsock/Interop.GetAddrInfoExW.cs

+            [In] ref NativeOverlapped overlapped,
+            [In] GetAddrInfoExCompletionCallback callback,
+            [Out] out IntPtr cancelHandle
+        );


Nit: We try to use the same names as in the native definition, so e.g.
name => pName
serviceName => pServiceName
namespaceId => lpNspId
etc.

stephentoub · 2018-02-05T15:03:07Z

src/Common/src/Interop/Windows/Winsock/Interop.GetAddrInfoExW.cs

+    {
+        internal unsafe delegate void GetAddrInfoExCompletionCallback([In] int error, [In] int bytes, [In] NativeOverlapped* overlapped);
+
+        [DllImport(Interop.Libraries.Ws2_32, ExactSpelling = true, CharSet = CharSet.Unicode, BestFitMapping = false, ThrowOnUnmappableChar = true, SetLastError = true)]


Why the ThrowOnUnmappableChar = true? That's pretty rare to see.

I copied theses properties from the getaddrinfo declaration without thinking. It doesn't make sense since the call is Unicode, I will remove ThrowOnUnmappableChar and BestFitMapping properties.

stephentoub · 2018-02-05T15:06:07Z

src/Common/src/Interop/Windows/Winsock/Interop.GetAddrInfoExW.cs

+            [In] string serviceName,
+            [In] int namespaceId,
+            [In] IntPtr providerId,
+            [In] ref AddressInfoEx hints,


Can this use in instead of ref?

The build is passing but Visual Studio is complaining with "Feature 'readonly references' is not available in C# 7.0. Please use language version 7.2 or greater."

I suggest leaving 'ref', I don't think it would change anything for a p/invoke call anyway.

stephentoub · 2018-02-05T15:07:18Z

src/Common/src/Interop/Windows/Winsock/Interop.GetAddrInfoExW.cs

+{
+    internal static partial class Winsock
+    {
+        internal unsafe delegate void GetAddrInfoExCompletionCallback([In] int error, [In] int bytes, [In] NativeOverlapped* overlapped);


Nit:
GetAddrInfoExCompletionCallback => LPLOOKUPSERVICE_COMPLETION_ROUTINE
error => dwError
bytes => dwBytes
etc.

stephentoub · 2018-02-05T15:08:04Z

src/Common/src/Interop/Windows/Winsock/Interop.GetAddrInfoExW.cs

+            [Out] out IntPtr cancelHandle
+        );
+
+        [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]


These ReliabilityContract attributes don't have a meaning in .NET Core and can be removed.

stephentoub · 2018-02-05T15:08:51Z

src/Common/src/Interop/Windows/Winsock/Interop.GetAddrInfoExW.cs

+
+        [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
+        [DllImport("ws2_32.dll", ExactSpelling = true, SetLastError = true)]
+        internal static extern int GetAddrInfoExCancel([In] ref IntPtr cancelHandle);


Same ref => in question.

stephentoub · 2018-02-05T15:13:07Z

src/System.Net.NameResolution/src/System/Net/DNS.cs

-                CancellationToken.None,
-                TaskCreationOptions.DenyChildAttach,
-                TaskScheduler.Default);
+            if (NameResolutionPal.SupportsGetAddrInfoAsync && includeIPv6 && SocketProtocolSupportPal.OSSupportsIPv6 && address == null)


Should NameResolutionPal.SupportsGetAddrInfoAsync factor in SocketProtocolSupportPal.OSSupportsIPv6 if that's part of what's required to use GetAddrInfoAsync, rather than accessing both at the call site? Or we can't do that easily because of layering?

More generally, why is OSSupportsIPv6 necessary for SupportsGetAddrInfoAsync?

GetAddrInfoEx doesn't require the OS to support IPv6. But the previous implementation was using GetHostByName instead of GetAddrInfo is OSSupportsIPv6 is false.

I'm not sure why that choice was made originally, but I thought it safe to not change this behavior.

Just saw the explanation in InternalGetHostByName

// Note : Whilst getaddrinfo is available on WinXP+, we only // use it if IPv6 is enabled (platform is part of that // decision). This is done to minimize the number of // possible tests that are needed.

Yeah, that comment is ancient. There's no reason for us to not always use getaddrinfo today, and it would simplify the code and the tests.

My concern is mainly that we not make things any worse than they currently are. So if you want to do the IPv6 checks, that's fine, but please add comments wherever you are doing so that refer back to the comment above, so it's clear why these checks are happening.

I created issue #26856 to track this.

stephentoub · 2018-02-05T15:13:51Z

src/System.Net.NameResolution/src/System/Net/DNS.cs

-                TaskCreationOptions.DenyChildAttach,
-                TaskScheduler.Default);
+            if (NameResolutionPal.SupportsGetAddrInfoAsync && includeIPv6 && SocketProtocolSupportPal.OSSupportsIPv6 && address == null)
+            {


Nit: can you add a comment about the conditions that let's us take this path?

stephentoub · 2018-02-05T15:15:53Z

src/System.Net.NameResolution/src/System/Net/DnsResolveAsyncResult.cs

+
+        internal readonly string hostName;
+        internal bool includeIPv6;
+        internal IPAddress address;


Nit: these should be prefixed with _

Can these be made readonly?

This was existing code moved from DNS.cs, but I will make the changes

stephentoub · 2018-02-05T15:16:43Z

src/System.Net.NameResolution/src/System/Net/DnsResolveAsyncResult.cs

+            base(myObject, myState, myCallBack)
+        {
+            this.includeIPv6 = includeIPv6;
+            this.address = address;


Nit: the "this"es shouldn't be necessary

stephentoub · 2018-02-05T15:16:49Z

src/System.Net.NameResolution/src/System/Net/DnsResolveAsyncResult.cs

@@ -0,0 +1,25 @@
+namespace System.Net
+{
+    internal class DnsResolveAsyncResult : ContextAwareResult


Nit: sealed

stephentoub · 2018-02-05T15:19:02Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Unix.cs

@@ -13,6 +13,8 @@ namespace System.Net
 {
    internal static partial class NameResolutionPal
    {
+        public static bool SupportsGetAddrInfoAsync => false;


Can you make this a const? That would help the compiler to eliminate branches at the call site when doing the unix build. (It can be const in the Unix build and a static property in the Windows one.)

stephentoub · 2018-02-05T15:20:46Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+        private static bool TestGetAddrInfoEx()
+        {
+            using (SafeLibraryHandle libHandle = Interop.Kernel32.LoadLibraryExW(Interop.Libraries.Ws2_32, IntPtr.Zero, 0))
+            {


Can/should we test libHandle.IsInvalid before calling GetProcAddress? e.g.

return !libHandle.IsInvalid && Interop.Kernel32.GetProcAddress(...

stephentoub · 2018-02-05T15:21:25Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+            try
+            { }
+            finally
+            {


This try/finally pattern is not needed. It's only relevant for thread aborts, which don't exist in .NET Core. This pattern can be removed throughout.

stephentoub · 2018-02-05T15:26:44Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+        {
+            using (SafeLibraryHandle libHandle = Interop.Kernel32.LoadLibraryExW(Interop.Libraries.Ws2_32, IntPtr.Zero, 0))
+            {
+                return Interop.Kernel32.GetProcAddress(libHandle, nameof(Interop.Winsock.GetAddrInfoExCancel)) != IntPtr.Zero;


We're testing for GetAddrInfoExCancel because that serves as an indication for whether GetAddrInfoExW supports overlapped execution? Can you add a comment?

Is there something we can assert to help validate that this is working? e.g. checking the Windows version and asserting that TestGetAddrInfoEx returns true on an appropriately recent version?

stephentoub · 2018-02-05T15:28:20Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+            { }
+            finally
+            {
+                // Can be casted directly to QueryContext* because the overlapped is its first field


Is this robust?

Why wouldn't it be? GetAddrInfoExContext is a struct with sequential layout and the Overlapped is its first field. It's a weird API but in c++ that's how it is used (see the asynchronous sample using CONTAINING_RECORD here https://msdn.microsoft.com/en-us/library/windows/desktop/ms738518(v=vs.85).aspx )

stephentoub · 2018-02-05T15:28:48Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+            GetAddrInfoExState state = context->GetQueryState();
+
+            if (state == null || !state.SetCallbackStartedOrCanceled())
+                return;


Who cleans up the allocated context?

The context is freed at the end of ProcessResult or in GetAddrInfoExState's finalizer when the AppDomain is unloaded.

The callback should not be called twice, but the callback could be raised at the same time the AppDomain is unloaded, so this check ensure only one path will free the context.

I will add a comment.

stephentoub · 2018-02-05T15:33:32Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+                    {
+                        ipAddress = CreateIPv4Address(result->ai_addr, result->ai_addrlen);
+                    }
+                    else if (SocketProtocolSupportPal.OSSupportsIPv6 && result->ai_family == AddressFamily.InterNetworkV6)


Will we ever get to this point if OSSupportsIPv6 is false?

Discussed above, I will leave the check here in case there is a code change in DNS.cs to call this method even if OS doesn't support IPv6

stephentoub · 2018-02-05T15:36:36Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+                               (socketAddress[7] << 24)
+                           ) & 0x00000000FFFFFFFF;
+
+            return new IPAddress(address);


Could this be:

return new IPAddress(new Span<byte>(socketAddress, 4, IPAddressParserStatics.IPv4AddressBytes));

?

stephentoub · 2018-02-05T15:38:27Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+            for (int i = 0; i < address.Length; i++)
+            {
+                address[i] = socketAddress[i + 8];
+            }


Let's avoid allocating an array here. This should be doable with span now, e.g.

return new IPAddress(new Span<byte>(socketAddress, 8, IPv6AddressBytes), scope);

stephentoub · 2018-02-05T15:39:54Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+        private static unsafe IPAddress CreateIPv6Address(byte* socketAddress, int addressLength)
+        {
+            const int IPv6SocketAddressSize = 28;
+            const int IPv6AddressBytes = 16;


This is already defined on the internal IPAddressParserStatics class and should be usable here.

stephentoub · 2018-02-05T15:40:52Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+        {
+            private IntPtr m_context;
+            private int m_callbackStartedOrCanceled;
+            private DnsResolveAsyncResult m_asyncResult;


Nits:
readonly on these?
m_ => _

stephentoub · 2018-02-05T15:41:42Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+
+        private sealed unsafe class GetAddrInfoExState : CriticalFinalizerObject, IDisposable
+        {
+            private IntPtr m_context;


Can this be done as a SafeHandle-derived type rather than CriticalFinalizerObject?

There is no reference counting and this handle is not used in a pinvoke, so I didn't see the gain in using SafeHandle. I used CriticalFinalizerObject for the CER, but since they don't exist in .Net Core I can make it derive from Object instead.

stephentoub · 2018-02-05T15:42:01Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+                m_asyncResult = asyncResult;
+            }
+
+            [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]


As noted earlier, these ReliabilityContract attributes can be removed.

stephentoub · 2018-02-05T15:46:23Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+
+            public void CompleteAsyncResult(object o)
+            {
+                Task.Run(() => m_asyncResult.InvokeCallback(o));


Do we need to queue the callback?

Assuming yes, this will allocate a delegate and closure. It can instead be written as:

Task.Factory.StartNew(s => { var t = (Tuple<DnsResolveAsyncResult, object>)s; t.Item1(t.Item2); }, Tuple.Create(_asyncResult, o), CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default);

That will incur the overhead of allocating just the tuple rather than the overhead of allocating the closure for the state as well as the delegate to a method on that closure.

stephentoub · 2018-02-05T15:46:58Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+            [ReliabilityContract(Consistency.WillNotCorruptState, Cer.Success)]
+            private void ReleaseResources(bool cancelQuery)
+            {
+                var ptr = Interlocked.Exchange(ref m_context, IntPtr.Zero);


Nit: var => IntPtr

stephentoub · 2018-02-05T15:47:57Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+                Overlapped = new NativeOverlapped();
+                Result = null;
+
+                var handle = GCHandle.Alloc(state, GCHandleType.Normal);


Nit: var => GCHandle

stephentoub · 2018-02-05T15:49:05Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+
+            public GetAddrInfoExState GetQueryState()
+            {
+                var stateHandle = Interlocked.Exchange(ref QueryStateHandle, IntPtr.Zero);


Nit: var => IntPtr

stephentoub

This looks good, @JeffCyr. Thanks for doing this. Have you done any perf analysis, to see how throughput compares? The primary purpose here is scalability and playing nicely with others, so a small perf hit is ok, it'd just be nice to know if there is one.

JeffCyr · 2018-02-08T19:40:22Z

Performance analysis

There is an overhead with GetAddrInfoEx, it is noticeable when requesting a hostname which is already in the local machine cache. However it is unlikely to be a performance bottleneck because an application will unlikely make thousands of dns queries per second.

The non-cached test shows the performance gain of this PR, the old implementation is throttled by the .Net ThreadPool thread creation rate (by starving it). The new implementation is unthrottled, the caveat is that it is now possible to reach the Response Rate Limiting of the DNS server, this may cause timeout or errors.

Sequential cached test

One thread sequentially makes 20 000 requests for the same (pre-cached) hostname.

Test	request/s
Dns.GetHostEntry	5125 request/s
Dns.GetHostEntryAsync (old)	4115 request/s
Dns.GetHostEntryAsync (new)	3400 request/s

Parallel cached test

One thread starts 20 000 requests in parallel and waits for completion for the same (pre-cached) hostname.

Test	request/s
Dns.GetHostEntryAsync (old)	20616 request/s
Dns.GetHostEntryAsync (new)	12996 request/s

Parallel non-cached test

One thread starts X requests in parallel and waits for completion, a random non-existant domain is used for each request.

X = 10	Time
Dns.GetHostEntryAsync (old)	00:00:01.6670757
Dns.GetHostEntryAsync (new)	00:00:00.8452532

X = 50	Time
Dns.GetHostEntryAsync (old)	00:00:04.9970314
Dns.GetHostEntryAsync (new)	00:00:00.8513915

X = 100	Time
Dns.GetHostEntryAsync (old)	00:00:09.9531006
Dns.GetHostEntryAsync (new)	00:00:00.9996548

X = 200	Time
Dns.GetHostEntryAsync (old)	00:00:16.6067046
Dns.GetHostEntryAsync (new)	00:00:01.1124727

JeffCyr · 2018-02-09T15:56:33Z

@dotnet-bot test Outerloop Windows x64 Debug Build please

jkotas · 2018-02-09T18:14:35Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+
+        private static bool GetAddrInfoExSupportsOverlapped()
+        {
+            using (SafeLibraryHandle libHandle = Interop.Kernel32.LoadLibraryExW(Interop.Libraries.Ws2_32, IntPtr.Zero, 0))


Pass in LOAD_LIBRARY_SEARCH_SYSTEM32 as flags.

stephentoub · 2018-02-10T02:01:23Z

Thanks for the perf data! I think this makes sense to go ahead with (once remaining feedback is addressed). @geoffkizer, any overarching concerns?

JeffCyr · 2018-02-10T13:54:47Z

@dotnet-bot test Outerloop Windows x64 Debug Build please

JeffCyr · 2018-02-10T16:46:33Z

Ok I addressed the last comments. I cleaned up the code a bit after removing the cancellation code. Ready for the final (I hope!) review.

stephentoub · 2018-02-11T14:31:31Z

src/Common/src/Interop/Windows/Winsock/Interop.GetAddrInfoExW.cs

+        internal static extern unsafe void FreeAddrInfoEx([In] AddressInfoEx* pAddrInfo);
+
+        [DllImport("ws2_32.dll", ExactSpelling = true, SetLastError = true)]
+        internal static extern int GetAddrInfoExCancel([In] ref IntPtr lpHandle);


This can be removed now, right? Looks like the only remaining usage is just using it for its name.

stephentoub · 2018-02-11T14:33:28Z

src/Common/src/System/Net/SocketAddressPal.Unix.cs

+        public static void GetIPv6Address(byte[] buffer, Span<byte> address, out uint scope)
+        {
+            GetIPv6Address(new ReadOnlySpan<byte>(buffer), address, out scope);
+        }


Do we need this overload? I'd expect the call site passing in the byte[] to just work with the span-based overload due to the implicit array->span cast.

stephentoub · 2018-02-11T14:33:41Z

src/Common/src/System/Net/SocketAddressPal.Windows.cs

@@ -39,6 +39,11 @@ public static unsafe void SetPort(byte[] buffer, ushort port)
        }

        public static unsafe uint GetIPv4Address(byte[] buffer)
+        {
+            return GetIPv4Address(new ReadOnlySpan<byte>(buffer));
+        }


Same "do we need this overload" question

stephentoub · 2018-02-11T14:33:55Z

src/Common/src/System/Net/SocketAddressPal.Windows.cs

+        public static void GetIPv6Address(byte[] buffer, Span<byte> address, out uint scope)
+        {
+            GetIPv6Address(new ReadOnlySpan<byte>(buffer), address, out scope);
+        }


stephentoub · 2018-02-11T14:42:50Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+            public static GetAddrInfoExContext* AllocateContext()
+            {
+                var context = (GetAddrInfoExContext*)Marshal.AllocHGlobal(Size);
+                *context = new GetAddrInfoExContext();


Nit: might be slightly more clear if this were just *context = default;, but not a big deal.

stephentoub · 2018-02-11T14:43:28Z

src/System.Net.NameResolution/src/System/Net/NameResolutionPal.Windows.cs

+                return GCHandle.ToIntPtr(handle);
+            }
+
+            public static GetAddrInfoExState FromHandle(IntPtr handle)


Nit: FromHandleAndFree? Might help make the cleanup more clear for someone reading the call site.

stephentoub

Looks great. Thanks, @JeffCyr! I left a few more small comments to try to help tidy thing up, but otherwise I think this can be merged.

* Modified Dns.GetHostAddressesAsync to be truly async * Applied code review recommendations * Unix build fix * Unix build fix dotnet#2 * Unix build fix dotnet#3 * NETFX build fix * Fixed useGetHostByName logic * Simplified ProcessResult code * Cleaned up cancel code * cleanup

…6850) * Modified Dns.GetHostAddressesAsync to be truly async * Applied code review recommendations * Unix build fix * Unix build fix dotnet/corefx#2 * Unix build fix dotnet/corefx#3 * NETFX build fix * Fixed useGetHostByName logic * Simplified ProcessResult code * Cleaned up cancel code * cleanup Commit migrated from dotnet/corefx@d3ff31e

Modified Dns.GetHostAddressesAsync to be truly async

e31d807

stephentoub reviewed Feb 5, 2018

View reviewed changes

stephentoub approved these changes Feb 8, 2018

View reviewed changes

Simplified ProcessResult code

a9a3403

JeffCyr force-pushed the getaddrinfoex branch from 9d7914f to a9a3403 Compare February 9, 2018 13:43

jkotas reviewed Feb 9, 2018

View reviewed changes

Cleaned up cancel code

91864e5

stephentoub reviewed Feb 11, 2018

View reviewed changes

stephentoub approved these changes Feb 11, 2018

View reviewed changes

cleanup

524b5fa

stephentoub approved these changes Feb 11, 2018

View reviewed changes

stephentoub merged commit d3ff31e into dotnet:master Feb 12, 2018

JeffCyr deleted the getaddrinfoex branch February 12, 2018 00:20

karelz added this to the 2.1.0 milestone Mar 10, 2018

MichalStrehovsky mentioned this pull request Nov 21, 2018

UWP Cant Submit to store Error API FreeAddrInfoEx microsoft/dotnet#924

Open

Aaronontheweb mentioned this pull request Nov 22, 2018

DotNetty Remote Transport Issues with .NET Core 2.1 akkadotnet/akka.net#3506

Closed

davidsh mentioned this pull request Feb 10, 2020

Dns.GetHostAddressesAsync fails with SocketException when called during impersonation dotnet/runtime#29935

Closed

wfurt mentioned this pull request Jan 13, 2021

[release/5.0] make async name resolution working from impersonificated context dotnet/runtime#46897

Merged

wfurt mentioned this pull request Jan 29, 2021

[release/5.0] Support Async DNS lookup on older Windows from impersonificated conte… dotnet/runtime#47657

Merged

Modified Dns.GetHostAddressesAsync to be truly async #26850

Modified Dns.GetHostAddressesAsync to be truly async #26850

Conversation

JeffCyr commented Feb 5, 2018 • edited by karelz Loading

dnfclas commented Feb 5, 2018 • edited Loading

stephentoub Feb 5, 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

stephentoub Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffCyr Feb 5, 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

Choose a reason for hiding this comment

stephentoub Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Feb 5, 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

stephentoub Feb 5, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

JeffCyr commented Feb 8, 2018 • edited Loading

Performance analysis

Sequential cached test

Parallel cached test

Parallel non-cached test

JeffCyr commented Feb 9, 2018

Choose a reason for hiding this comment

stephentoub commented Feb 10, 2018

JeffCyr commented Feb 10, 2018

JeffCyr commented Feb 10, 2018

stephentoub Feb 11, 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 left a comment

Choose a reason for hiding this comment

JeffCyr commented Feb 5, 2018 •

edited by karelz

Loading

dnfclas commented Feb 5, 2018 •

edited

Loading

stephentoub Feb 5, 2018 •

edited

Loading

stephentoub Feb 5, 2018 •

edited

Loading

stephentoub Feb 5, 2018 •

edited

Loading

JeffCyr Feb 5, 2018 •

edited

Loading

stephentoub Feb 5, 2018 •

edited

Loading

stephentoub Feb 5, 2018 •

edited

Loading

stephentoub Feb 5, 2018 •

edited

Loading

JeffCyr commented Feb 8, 2018 •

edited

Loading

stephentoub Feb 11, 2018 •

edited

Loading