Production code changes to aid with test stabilization by sharwell · Pull Request #25558 · dotnet/roslyn

sharwell · 2018-03-17T15:56:13Z

This pull request contains the changes to production code resulting from a larger effort to stabilize flaky tests.

Move the listener/waiter test hooks to the workspaces layer
Cancel pending asynchronous operations when IdleProcessor stops
Extract interface ISerializer from Serializer
Convert ISerializer to workspace service ISerializerService
Add service creation hooks for HostServices implementations
Avoid possible MEF composition in Workspace.Dispose
Make AssetService.s_serializer a per-instance field
Make PrimaryWorkspace a MEF component instead of static
Make SolutionService.PrimaryWorkspace a per-instance property
Add the ability to hook RoslynServices.HostServices during testing
Place ITextBufferCloneService in ITextBuffer.Properties instead of separate tracking in SnapshotSourceText

Customer scenario

This is a change to improve the testability of our code. It is not intended to produce any user-visible behavior or performance changes.

Bugs this fixes

n/a (but supports fixing many flaky test issues)

Workarounds, if any

None

Risk

Relatively low. The risk here primarily lies in a few changes to internal APIs. Downstream consumers accessing PrimaryWorkspace, Serializer, or a couple Roslyn remoting types through IVT will need to update their code to avoid this path.

Performance impact

No expected performance impact in production.

Is this a regression from a previous update?

This is a developer/contributor regression. Our tests have become more sensitive to mutable data sharing across test boundaries as we adopt updated dependencies that impose stricter requirements for asynchronous code, increasing the chances that tests will fail in CI builds.

Root cause analysis

See previous.

How was the bug found?

CI test process dump analysis.

Test documentation updated?

No change to testing approaches results directly from this change.

CyrusNajmabadi · 2018-03-17T21:21:59Z

src/Features/Core/Portable/Shared/TestHooks/Legacy/ListenerForwarders.cs

+[assembly: TypeForwardedTo(typeof(Microsoft.CodeAnalysis.Shared.TestHooks.IAsynchronousOperationWaiter))]
+[assembly: TypeForwardedTo(typeof(Microsoft.CodeAnalysis.Shared.TestHooks.IAsynchronousOperationListener))]
+[assembly: TypeForwardedTo(typeof(Microsoft.CodeAnalysis.Shared.TestHooks.IAsyncToken))]
+[assembly: TypeForwardedTo(typeof(Microsoft.CodeAnalysis.Shared.TestHooks.TaskExtensions))]


Out of curiosity, why are these needed? for F#/TS/JS?

➡️ Considering the impact of other changes coming in this PR, I'm guessing these are not going to be necessary in the final revision. However, I decided to make the relocation separate from the removal of the original types.

Do we have some tracking bug to track (eventually) getting rid of them?

This change was intended to avoid the need to have WorkspacesTestUtilities reference Features. This is no longer avoidable, and WorkspacesTestUtilities ended up taking a reference all the way to RemoteWorkspaces. However, this change may still be beneficial as part of work to ensure all asynchronous operations started by Roslyn are tracked.

CyrusNajmabadi · 2018-03-17T21:23:52Z

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.SemanticChangeProcessor.cs

+                        },
+                        CancellationToken.None,
+                        TaskContinuationOptions.ExecuteSynchronously,
+                        TaskScheduler.Default);


Would benefit from comments explaining what's going on here. A comment also explaining that this should run unilaterally, regardless of how AsyncProcessorTask completes might be good.

CyrusNajmabadi · 2018-03-17T21:24:23Z

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.SemanticChangeProcessor.cs

+                            },
+                            CancellationToken.None,
+                            TaskContinuationOptions.ExecuteSynchronously,
+                            TaskScheduler.Default);


duplicate code. Can we we share with above?

Should we be using LazyCancellation here? in other words, is it ok if this code runs before some antecedent tasks completes, but after the cancellation token controlling another intermediary tasks completes? In other words, should this be using the semantics of SafeContinueWith?

Doesn't LazyCancellation only have an effect on continuations that specify a CancellationToken?

I don't think so (but i'm not totally sure). I thought it was more about the antecedent tasks and if they used a cancellation token. in other words, if i have tasks:

A <- B <- C (i.e. B depends on A and C depends on B)

And 'A' is running, but B gets canceled. The question is: should 'C' run? LazyCancellation (to me) says: "No. All antecedent tasks are not completed. Even though B transitioned ot Canceled, C should not run yet because 'A' is still running".

This is regardless of the cancellation token passed to C.

@stephentoub For guidance.

💭 I looked at https://referencesource.microsoft.com and it seems to only impact cases where a cancellation token is involved.

@CyrusNajmabadi is correct re "I thought it was more about the antecedent tasks and if they used a cancellation token." If you have:

Task a = ...; Task b = a.ContinueWith(..., token); Task c = b.ContinueWith(...);

and while a is running the token has cancellation requested, should c be allowed to run immediately or must it wait for a to finish.

@stephentoub I believe you may have misunderstood the context. In my code, I pass CancellationToken.None to ContinueWith. I am claiming that TaskContinuationOptions.LazyCancellation will never have an impact on a call to ContinueWith called in this manner.

I didn't look at your code; I was commenting on the behavior of LazyCancellation and what it actually does.

Given the code:

Task a = ...; Task b = a.ContinueWith(..., token); Task c = b.ContinueWith(...);

c can end up running concurrently with a. To prevent that, you can make it:

Task a = ...; Task b = a.ContinueWith(..., token, TaskContinuationOptions.LazyCancellation, TaskScheduler.Default); Task c = b.ContinueWith(...);

and then c will not run concurrently with a. If that token is None, then it doesn't matter because it won't be canceled prior to a completing (it could still be canceled by other means, e.g. if you passed TaskContinuationOptions.OnlyOnFaulted and the antecedent completed in another state, but in such cases the antecedent needs to be completed for that cancellation to happen).

If this is the case, can we comment that it's ok for us to not pass LazyCancellation here as it seems to only be relevant for the ContinueWith's that take cancellation tokens?

Thanks!

CyrusNajmabadi · 2018-03-17T21:27:43Z

src/Workspaces/Core/Portable/Execution/SerializerService.cs

-
-        public Serializer(HostWorkspaceServices workspaceServices)
+        [Obsolete("This exported object must be obtained through the MEF export provider.", error: true)]
+        public SerializerService(HostWorkspaceServices workspaceServices)


cute. i like that. we should do that in a lot of places :)

➡️ #25560

CyrusNajmabadi · 2018-03-17T21:29:16Z

src/Workspaces/Core/Portable/Execution/SerializerServiceFactory.cs

+    [ExportWorkspaceServiceFactory(typeof(ISerializerService), layer: ServiceLayer.Default), Shared]
+    internal class SerializerServiceFactory : IWorkspaceServiceFactory
+    {
+        [Obsolete("This is the factory method for " + nameof(SerializerService) + ".", error: true)]


Could we have our own subclass for this attribute? it reads as if: "this is obsolete and hsould not be used. people should transition to something else". But, in reality, it just means: "it's only for use by mef. we want to keep it, but you should not reference it directly in your code."

Something like "MefConstructorAttribute : ObsoleteAttribute"?

➡️ ObsoleteAttribute is sealed, but I'm considering creating a class that holds a const string for the message with a comment on that declaration.

CyrusNajmabadi

Minor nits.

jaredpar · 2018-03-19T16:57:18Z

src/Workspaces/Core/Portable/Workspace/PrimaryWorkspace.cs

    {
-        private static readonly ReaderWriterLockSlim s_registryGate = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
-        private static Workspace s_primaryWorkspace;
+        private readonly ReaderWriterLockSlim s_registryGate = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);


s_registryGate [](start = 46, length = 14)

s_ prefix no longer needed as this is an instance variable.

➡️ Fixed in 9c28f4a

jaredpar · 2018-03-19T16:57:34Z

src/Workspaces/Core/Portable/Workspace/PrimaryWorkspace.cs

-        private static readonly ReaderWriterLockSlim s_registryGate = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
-        private static Workspace s_primaryWorkspace;
+        private readonly ReaderWriterLockSlim s_registryGate = new ReaderWriterLockSlim(LockRecursionPolicy.NoRecursion);
+        private Workspace s_primaryWorkspace;


s_primaryWorkspace [](start = 26, length = 18)

s_ prefix no longer needed as this is an instance variable.

➡️ Fixed in 9c28f4a

jaredpar · 2018-03-19T17:01:44Z

src/VisualStudio/Core/Def/Implementation/Remote/ServiceHubRemoteHostClient.Connections.cs

            private static bool s_infoBarReported = false;

-            private static void ShowInfoBar()
+            private static void ShowInfoBar(Workspace workspace)


Shouldn't we assert when this is null? The API is a no-op if this is null hence seems like a required paramter.

I have no idea. I'm not familiar with this code and was simply trying to preserve behavior in the absence of static state class PrimaryWorkspace.

jaredpar · 2018-03-19T17:03:47Z

src/Workspaces/Core/Desktop/Workspace/Host/Mef/MefV1HostServices.cs

            return new MefV1HostServices(container);
        }

+        internal static void HookServiceCreation(CreationHook hook)


internal static void HookServiceCreation(CreationHook hook) [](start = 7, length = 60)

Should we assert that s_CreationHook is null? Lacking that seems like we will risk introducing bugs by silently overwriting the previous vaule.

➡️ No. In the cleanup step for the test hook, the creation hook is replaced by a non-null hook that produces an informative exception/test failure in the event code attempts to create an ExportProvider after the test exits the isolation boundary.

For cases where the overwrite is inteended a force parameter could be employed.

In reply to: 175523407 [](ancestors = 175523407)

sharwell · 2018-03-19T17:40:06Z

📝 @CyrusNajmabadi @jaredpar I'm going to apply code changes resulting from code review after I work with HeeJae to complete the final behavior step.

jaredpar · 2018-03-19T18:08:53Z

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.SemanticChangeProcessor.cs

+                    AsyncProcessorTask.ContinueWith(
+                        _ =>
+                        {
+                            foreach (var (documentId, data) in _pendingWork)


documentId, [](start = 42, length = 11)

Consider using _ instead of documentId to make it clear it's not used.

➡️ Fixed in 9c28f4a

jaredpar · 2018-03-19T18:09:56Z

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.SemanticChangeProcessor.cs

+                        AsyncProcessorTask.ContinueWith(
+                            _ =>
+                            {
+                                foreach (var (projectId, data) in _pendingWork)


projectId [](start = 46, length = 9)

consider using _ instead of projecTId to make it clear it's not used.

➡️ Fixed in 9c28f4a

jaredpar · 2018-03-19T18:10:47Z

src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.SemanticChangeProcessor.cs

+                                data.AsyncToken.Dispose();
+                            }
+
+                            _pendingWork.Clear();


_pendingWork.Clear(); [](start = 28, length = 21)

Why aren't the operations on _pendingWork guarded by the appropriate gate here?

➡️ Fixed in 9c28f4a

jaredpar · 2018-03-19T18:11:28Z

src/VisualStudio/Core/Def/Implementation/Progression/GraphProvider.cs

        protected AbstractGraphProvider(
            IGlyphService glyphService,
            SVsServiceProvider serviceProvider,
-            CodeAnalysis.Workspace workspace,


CodeAnalysis.Workspace workspace, [](start = 11, length = 34)

A bit disturbing this parameter was provided before and simply ignored.

jaredpar · 2018-03-19T18:14:19Z

src/Workspaces/Core/Portable/Workspace/PrimaryWorkspace.cs


 namespace Microsoft.CodeAnalysis
 {
-    internal static class PrimaryWorkspace


internal static class PrimaryWorkspace [](start = 4, length = 38)

I support all removal of global mutable statics.

jasonmalinowski

Hitting request changes because I'm worried about the PrimaryWorkspace change, and specifically that SourceTextContainers are now holding onto workspaces. It's often the case that the extension methods that pass "null" might be the first use of an ITextBuffer (or will be after any refactoring) and thus we'll end up disabling cloning for derived SourceTexts. If this causes a perf regression that blocks insertion that would be a problem. I suggested an alternative use which is still a mutable static but a more constrained mutable static that if only set in VisualStudioWorkspace or the like would mean we can mitigate that risk, until we get @olegtk to give us a proper API and remove the need entirely.

If we can confirm there isn't an RPS regression I'll happily pinch my nose and hit signoff.

I could also imagine the PrimaryWorkspace thing could break Visual Studio for Mac. Any breakage is good in that they should stop using just as we should stop using it, but heads up to @KirillOsenkov either way.

Massive thank you for breaking this up commit-by-commit. Was easy to follow and was pretty easy to guess where you were going.

jasonmalinowski · 2018-03-20T02:54:51Z

src/Features/Core/Portable/Shared/TestHooks/Legacy/ListenerForwarders.cs

+[assembly: TypeForwardedTo(typeof(Microsoft.CodeAnalysis.Shared.TestHooks.IAsynchronousOperationWaiter))]
+[assembly: TypeForwardedTo(typeof(Microsoft.CodeAnalysis.Shared.TestHooks.IAsynchronousOperationListener))]
+[assembly: TypeForwardedTo(typeof(Microsoft.CodeAnalysis.Shared.TestHooks.IAsyncToken))]
+[assembly: TypeForwardedTo(typeof(Microsoft.CodeAnalysis.Shared.TestHooks.TaskExtensions))]


Do we have some tracking bug to track (eventually) getting rid of them?

jasonmalinowski · 2018-03-20T03:01:56Z

src/Workspaces/Core/Desktop/Workspace/Host/Mef/MefV1HostServices.cs

    {
+        internal delegate MefV1HostServices CreationHook(IEnumerable<Assembly> assemblies);
+
+        private static CreationHook s_CreationHook;


Would like a comment that this is test-only. I can imagine people doing really terrible things with this thinking this is a "feature".

➡️ Added comments in 37a1d2c

jasonmalinowski · 2018-03-20T03:03:33Z

src/Workspaces/Core/Portable/Workspace/Host/Mef/MefHostServices.cs

+        internal static void HookServiceCreation(CreationHook hook)
+        {
+            s_CreationHook = hook;
+            s_defaultHost = null;


Comment why. I can guess, but I'm not necessarily the next person to touch this. 😄

➡️ Added comments in 37a1d2c

jasonmalinowski · 2018-03-20T03:04:14Z

src/Workspaces/Core/Portable/Workspace/Host/Mef/MefHostServices.cs

    {
+        internal delegate MefHostServices CreationHook(IEnumerable<Assembly> assemblies, bool requestingDefaultHost);
+
+        private static CreationHook s_CreationHook;


Ditto: comment this is test only. Even better: name the field appropriately.

➡️ Added comments in 37a1d2c

jasonmalinowski · 2018-03-20T03:28:07Z

src/EditorFeatures/Text/Extensions.cs

    {
        public static SourceTextContainer AsTextContainer(this ITextBuffer buffer)
-            => TextBufferContainer.From(buffer);
+            => TextBufferContainer.From(workspace: null, buffer);


This one worries me, since it's entirely likely the very first use of an ITextBuffer goes through this API (at least it is in my project system refactoring), which then means it's not going to get a workspace and never will. This may very well be causing a perf regression if the cloning stuff is necessary. Will probably have to ask for an RPS run then prior to this being merged unless we have an alternate solution, since taking an RPS regression now would cause codeflow to miss a few deadlines.

To be honest, it looks like this is all so we can call ITextBufferCloneService.Clone(ITextImage), which we could just have a static function that's Function<ITextImage, ITextBuffer> and we set that in VisualStudioWorkspace so it's OK for VS, and not set it in tests. Yes, statics are evil, but hey, at least instead of the static for PrimaryWorkspace now it's a single-use static that doesn't open pandora's box. Wishing I did that years ago now...

And that lives long enough until we pester @olegtk or somebody to give us a proper API here, because ITextImage.WithChanges() that returns an ITextImage would be both straightforward and also really useful.

Can you review commit 1653801 and let me know if the approach in that change is preferable? If so I will add it to this pull request.

➡️ Fixed in 4924d70

CyrusNajmabadi · 2018-03-20T07:46:15Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

-                    }
-
-                    return null;
+                    return _workspace?.Services.GetService<ITextBufferCloneService>();


i woudl feel better if this service was simply passed in in the start, rather than having to hold onto the _workspace itself. is that doable?

➡️ I replied to Jason's post regarding this with a proposed alternative. I'm waiting to hear back from reviewers.

jaredpar

🕐

revoking review

jaredpar · 2018-03-20T15:23:25Z

Sorry my review status is flipping. Was experimenting with CodeFlow features.

…parate tracking in SnapshotSourceText

jasonmalinowski · 2018-03-22T00:03:23Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs


                // check whether we can use text buffer factory
-                var factory = TextBufferFactory;
+                var factory = _textBufferCloneServiceOpt;


Nit: now that that's readonly, we don't have to worry about capturing to a local which was previously required. (But hey, it's shorter than inlining, so...)

I would likely change it (at least rename) if the code was more complex, but the method seems straightforward enough to keep.

jasonmalinowski · 2018-03-22T00:06:06Z

src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

@@ -82,8 +80,8 @@ public static SourceText From(Workspace workspace, ITextSnapshot editorSnapshot)
                if (!s_textSnapshotMap.TryGetValue(editorSnapshot, out var snapshot))
                {
                    // Avoid capturing `workspace` on the fast path


Nit: comment name needs updating.

I'll update this in a commit in my follow-up pull request to avoid needing to rebuild everything again.

jasonmalinowski · 2018-03-22T00:08:13Z

src/EditorFeatures/Text/Extensions.cs


        public static SourceText AsText(this ITextSnapshot textSnapshot)
-            => SnapshotSourceText.From(workspace: null, textSnapshot);
+            => SnapshotSourceText.From(textBufferCloneServiceOpt: null, textSnapshot);


Should this try fetching the service like TextBufferContainer does?

Unfortunately ITextSnapshot doesn't expose any properties to try and fetch from.

jasonmalinowski · 2018-03-22T00:09:40Z

...EditorFeatures/Text/Implementation/TextBufferFactoryService/TextBufferCloneServiceFactory.cs


        public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices)
        {
            return _singleton;


_singleton seems a misnomer here: there's one you'll get through the factory, and one directly from the export below. This could just have the [ImportingConstructor] import the type below and return it, just making this an even thinner shim.

(If the only thing still using the workspace service is that inline rename thing, then just disregard this comment and we'll delete this then.)

➡️ Covered as part of #25659

sharwell · 2018-03-22T01:25:41Z

@Pilchie and/or @MattGertz for approval

Pilchie · 2018-03-22T20:22:01Z

Have we verified that TypeScript/F#/Xaml work with these changes? Do they have an impact on VS4Mac that we should worry about?

sharwell · 2018-03-22T21:33:36Z

@Pilchie Not specifically, because I'm still not sure how to do that exactly. The listener-related items valuable in testing scenarios have type forwarders and shouldn't be a problem unless Workspaces needs some other IVT that Features already has. The other internal API changes were primarily used in remote workspace / OOP scenarios so it's unlikely to cause problems.

Pilchie · 2018-03-22T22:16:33Z

Tagging @minestarks @brettfo @mgoertz-msft - can you three take a look and chime in on whether you think this will impact your scenarios at insertion time?

mgoertz-msft · 2018-03-23T00:01:42Z

@Pilchie Shouldn't affect the XAML language service as long as you don't change any of the interfaces in XamlVisualStudio.csproj. The only other concern would be if this were to affect CustomWorkspaces, which we do use in the VS codebase.

brettfo · 2018-03-23T00:01:49Z

Looking through the changes I don't see anything that'll likely break F#, but if I could be emailed when we have a VAL build I'll run through some scenarios.

minestarks

No impact on TS that I can tell.

Pilchie · 2018-03-23T17:13:52Z

Approved - thanks for the diligence.

This addresses a code review feedback point from dotnet#25558.

Move the listener/waiter test hooks to the workspaces layer

ce44aa7

sharwell added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 17, 2018

sharwell requested a review from a team as a code owner March 17, 2018 15:56

sharwell changed the title ~~[WIP] Production code changes to aid with test stabilization~~ Production code changes to aid with test stabilization Mar 17, 2018

sharwell added 6 commits March 17, 2018 11:50

Cancel pending asynchronous operations when IdleProcessor stops

05cceb8

Extract interface ISerializer from Serializer

747ef31

Convert ISerializer to workspace service ISerializerService

d43ad9b

Add service creation hooks for HostServices implementations

f80ac92

Avoid possible MEF composition in Workspace.Dispose

805e6fa

Make AssetService.s_serializer a per-instance field

ece3115

CyrusNajmabadi reviewed Mar 17, 2018

View reviewed changes

CyrusNajmabadi approved these changes Mar 17, 2018

View reviewed changes

sharwell added 2 commits March 18, 2018 11:16

Make PrimaryWorkspace a MEF component instead of static

d07ec18

Make SolutionService.PrimaryWorkspace a per-instance property

f12bda8

jaredpar reviewed Mar 19, 2018

View reviewed changes

jasonmalinowski requested changes Mar 20, 2018

View reviewed changes

Add the ability to hook RoslynServices.HostServices during testing

37a1d2c

sharwell mentioned this pull request Mar 20, 2018

Explicitly create IMouseProcessorProvider before text views #25468

Closed

CyrusNajmabadi reviewed Mar 20, 2018

View reviewed changes

sharwell removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Mar 20, 2018

jaredpar previously requested changes Mar 20, 2018

View reviewed changes

sharwell added 3 commits March 20, 2018 13:16

Merge remote-tracking branch 'dotnet/dev15.7.x' into export-provider

ca15a20

Cleanup from code review feedback

9c28f4a

Place ITextBufferCloneService in ITextBuffer.Properties instead of se…

4924d70

…parate tracking in SnapshotSourceText

sharwell mentioned this pull request Mar 20, 2018

xUnit crash with -1073740791 #25119

Closed

jasonmalinowski approved these changes Mar 22, 2018

View reviewed changes

sharwell mentioned this pull request Mar 22, 2018

Make ITextBufferCloneService not a workspace service #25659

Closed

minestarks approved these changes Mar 23, 2018

View reviewed changes

Pilchie added the Approved to merge label Mar 23, 2018

sharwell merged commit c0bef8d into dotnet:dev15.7.x Mar 23, 2018

sharwell deleted the export-provider branch March 23, 2018 17:21

sharwell added a commit to sharwell/roslyn that referenced this pull request Mar 23, 2018

Update code comment to reflect actual behavior

c53626a

This addresses a code review feedback point from dotnet#25558.

This was referenced Mar 23, 2018

Fix failure to initialize RemoteWorkspace in the out-of-process host #25691

Merged

Preview3 rollup #25747

Merged

Isolate tests #25681

Merged

Add InternalsVisibleTo from Workspaces to Microsoft.Test.Apex.VisualStudio #25803

Merged

sharwell mentioned this pull request Apr 23, 2018

Use ITextBufferCloneService if available in AsText #26339

Merged

sharwell mentioned this pull request May 11, 2018

Relax the requirement in WithChanges that changes be ordered #26798

Merged

Conversation

sharwell commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Workarounds, if any

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Test documentation updated?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell Mar 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharwell commented Mar 17, 2018 •

edited

Loading

sharwell Mar 20, 2018 •

edited

Loading

sharwell Mar 20, 2018 •

edited

Loading

sharwell Mar 20, 2018 •

edited

Loading