More RemoteHostClient refactoring by tmat · Pull Request #43928 · dotnet/roslyn

tmat · 2020-05-03T00:35:05Z

No description provided.

CyrusNajmabadi · 2020-05-03T00:37:56Z

src/VisualStudio/Core/Def/Implementation/Remote/GlobalNotificationRemoteDeliveryService.cs

+        /// Lock for the <see cref="_globalNotificationsTask"/> task chain.  Each time we hear 
+        /// about a global operation starting or stopping (i.e. a build) we will '.ContinueWith'
+        /// this task chain with a new notification to the OOP side.  This way all the messages
+        /// are properly serialized and appera in the right order (i.e. we don't hear about a 


Suggested change

/// are properly serialized and appera in the right order (i.e. we don't hear about a

/// are properly serialized and appear in the right order (i.e. we don't hear about a

``` #Resolved

CyrusNajmabadi · 2020-05-03T00:39:28Z

src/VisualStudio/Core/Def/Implementation/Remote/GlobalNotificationRemoteDeliveryService.cs

+        /// stop prior to hearing about the relevant start).
+        /// </summary>
+        private readonly object _globalNotificationsGate = new object();
+        private Task<GlobalNotificationState> _globalNotificationsTask = Task.FromResult(GlobalNotificationState.NotStarted);


cute :) #Resolved

CyrusNajmabadi · 2020-05-03T00:40:20Z

src/VisualStudio/Core/Def/Implementation/Remote/GlobalNotificationRemoteDeliveryService.cs

+                if (previousTask.Result != GlobalNotificationState.NotStarted)
+                {
+                    return previousTask.Result;
+                }


should we assert we're not in the 'Finished' state? can we oc what hte Finished state actually is? #Resolved

n/m. i read this wrong. #Resolved

tmat · 2020-05-03T00:41:00Z

src/VisualStudio/Core/Def/Implementation/Remote/GlobalNotificationRemoteDeliveryService.cs

+
+        /// <summary>
+        /// Lock for the <see cref="_globalNotificationsTask"/> task chain.  Each time we hear 
+        /// about a global operation starting or stopping (i.e. a build) we will '.ContinueWith'


This code moved from RemoteHostClient. #Resolved

CyrusNajmabadi · 2020-05-03T00:41:09Z

src/VisualStudio/Core/Def/Implementation/Remote/GlobalNotificationRemoteDeliveryService.cs

+        {
+            NotStarted,
+            Started,
+            Finished


i don't see the Finished value ever used. did i miss it?
#Resolved

I see, it used to be used. I deleted that code :) Will remove this value.

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

CyrusNajmabadi · 2020-05-03T00:42:16Z

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

-
-                await _endPoint.InvokeAsync(
-                    nameof(IRemoteHostService.OnGlobalOperationStarted),
-                    new object[] { "" },


previously we sent empty string, now we send nothing, is that ok? #Resolved

Yes, there probably used to be some bug in JSON-RPC. I tested this works.

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

CyrusNajmabadi · 2020-05-03T00:42:42Z

src/VisualStudio/Core/Def/Implementation/Remote/GlobalNotificationRemoteDeliveryService.cs

+                    continuation, _cancellationToken, TaskContinuationOptions.None, TaskScheduler.Default);
+            }
+
+            async Task<GlobalNotificationState> continuation(Task<GlobalNotificationState> previousTask)


Suggested change

async Task<GlobalNotificationState> continuation(Task<GlobalNotificationState> previousTask)

return;

async Task<GlobalNotificationState> continuation(Task<GlobalNotificationState> previousTask)

``` #Resolved

IDE style is to have explicit exit above the local functions that way the read knows tehre's no 'outer' code that executes beyond that point. (same for the above method plx). #Resolved

I just moved the method. But I can add it. It looks odd though, maybe it would be better if the local function was above .

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

I just moved the method. But I can add it. It looks odd though, maybe it would be better if the local function was above .

In general, IDE style is:

local functions at the end. This enables someone to immediately see the main-thread code of the function without having to go through the helpers first.

an explicit exit prior to the local functions. This ensures that you can immediately see there's no additional code that runs. And the local functions don't intersperse hte actual method code.

local functions are PascalCased at the IDE layer. We tend think of them as 'scoped sibling helper methods' versus 'a local variable of a function type'.

Optionally: sometimes we put // local functions above hte local functions. This has helped clarify things in PRs where sometimes it looks like a sibling method, and it gets confusing (esp. with captures) when you can't immediatley tell that it's a local-function. However, this is rarer now now that most people are comfortable with them.

Seems like it's better then to make it a method because all of the above is just adding extra stuff to make it clearer.

tbh, that's my general preference. but i get the desire to occasionally have a helper method that is just scoped narrowly. in practice though, if your class is well contained, just having these be helper methods is 100% fine (And how we survived for decades) :)

Especially the need to add a comment means the style is bad, imo.

tmat added 6 commits May 2, 2020 17:33

Remove direct usage of Connection

114b946

Connection

cddaae6

Refactor connection pooling

a42f225

SolutionChecksumUpdater

709f4d7

Rename

c791286

GlobalNotificationRemoteDeliveryService

d357527

CyrusNajmabadi reviewed May 3, 2020

View reviewed changes

tmat commented May 3, 2020

View reviewed changes

CyrusNajmabadi reviewed May 3, 2020

View reviewed changes

CyrusNajmabadi approved these changes May 3, 2020

View reviewed changes

tmat added 2 commits May 2, 2020 17:51

Feedback

2ecceb7

Fix

0fafd05

tmat marked this pull request as ready for review May 3, 2020 00:53

tmat requested a review from a team as a code owner May 3, 2020 00:53

Avoid local functions

75cbcee

tmat force-pushed the KeepAliveSession2 branch from 91d8c69 to 75cbcee Compare May 3, 2020 01:11

Fix

caad5f3

tmat merged commit 65722f0 into dotnet:master May 3, 2020

ghost added this to the Next milestone May 3, 2020

tmat deleted the KeepAliveSession2 branch May 3, 2020 05:15

jinujoseph added Area-IDE Concept-OOP Related to out-of-proc labels May 3, 2020

sharwell mentioned this pull request May 4, 2020

Merge release/dev16.7-preview1 to master #43922

Merged

tmat mentioned this pull request May 11, 2020

Ignore cancelled/faulted tasks in GlobalNotificationRemoteDeliveryService #44105

Merged

JoeRobich modified the milestones: Next, 16.7.P2 May 18, 2020

	/// are properly serialized and appera in the right order (i.e. we don't hear about a
	/// are properly serialized and appear in the right order (i.e. we don't hear about a
	``` #Resolved

Conversation

tmat commented May 3, 2020

Uh oh!

CyrusNajmabadi May 3, 2020 • edited by tmat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 3, 2020 • edited by tmat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 3, 2020 • edited by tmat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 3, 2020 • edited by tmat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat May 3, 2020 • 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!

CyrusNajmabadi May 3, 2020 • edited by tmat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat May 3, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 3, 2020 • edited by tmat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat May 3, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 3, 2020 • edited by tmat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 3, 2020 • edited by tmat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat May 3, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi May 3, 2020 • 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!

tmat May 3, 2020 • 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!

CyrusNajmabadi May 3, 2020

Choose a reason for hiding this comment

Uh oh!

tmat May 3, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi May 3, 2020 •

edited by tmat

Loading

CyrusNajmabadi May 3, 2020 •

edited by tmat

Loading

CyrusNajmabadi May 3, 2020 •

edited by tmat

Loading

CyrusNajmabadi May 3, 2020 •

edited by tmat

Loading

tmat May 3, 2020 •

edited

Loading

CyrusNajmabadi May 3, 2020 •

edited by tmat

Loading

CyrusNajmabadi May 3, 2020 •

edited by tmat

Loading

CyrusNajmabadi May 3, 2020 •

edited by tmat

Loading

CyrusNajmabadi May 3, 2020 •

edited by tmat

Loading

CyrusNajmabadi May 3, 2020 •

edited

Loading

tmat May 3, 2020 •

edited

Loading

tmat May 3, 2020 •

edited

Loading