RAR as a Service: Transfer and execution #5699

ostorc · 2020-09-02T06:31:41Z

This PR closes #5556 and #5557.

When this PR is closed it will provide functionality to delegate work to RAR node (implemented by #5555).

In this PR I provide necessary functionality to delegate work of RAR task to RAR node.

I have added code mostly into src\Tasks\ResolveAssemblyReferences\:
Client
Contains all logic for client to construct connection and to delegate work.
Contract
DTOs and interface which client expects server to have.
Formatters
Custom written formatters, so we don't wait for them to be constructed dynamically, which adds extra cost to 1st request.
Server
Server side logic for handling connection
Services
Logic used during execution of RAR task. In future, there should be added caching layer, etc.

TODO: Logs are serialized in not optimal way. Find better solution

Forgind

I think this works, but it's a bigger change than I'd expected.

src/Framework/BuildErrorEventArgs.cs

ref/Microsoft.Build.Tasks.Core/netstandard/Microsoft.Build.Tasks.Core.cs

src/Tasks.UnitTests/AssemblyDependency/BaseComparer.cs

src/Tasks.UnitTests/AssemblyDependency/RARRequestComparer.cs

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

src/Tasks/ResolveAssemblyReferences/Contract/ReadOnlyTaskItem.cs

src/Tasks/ResolveAssemblyReferences/LazyFromattedBuildEventArgsFormatter.cs

src/Tasks/ResolveAssemblyReferences/Services/ResolveAssemblyReferenceBuildEngine.cs

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

ostorc · 2020-09-10T17:41:29Z

/azp run

azure-pipelines · 2020-09-10T17:41:38Z

Azure Pipelines successfully started running 1 pipeline(s).

ladipro

Thank you, I've done a first pass over the changes and have left comments and questions inline. Overall:

Would it be appropriate to remove the WIP label from the PR now?
Can you add a good description explaining the code structure, which files were generated by which tool and so on?
Is this PR also making it possible to run tasks concurrently in the RAR service? The change introducing getRootedPath would suggest so.
What is the caching strategy? Do entries ever expire? What guarantees that when a dependent assembly changes, all relevant entries are invalidated?

src/Build/BackEnd/BuildManager/BuildManager.cs

src/Framework/MSBuildEventSource.cs

ladipro · 2020-09-11T12:15:26Z

src/MSBuild/app.amd64.config

+        <dependentAssembly>
+          <assemblyIdentity name="Newtonsoft.Json" culture="neutral" publicKeyToken="30ad4fe6b2a6aeed" />
+          <bindingRedirect oldVersion="0.0.0.0-12.0.0.0" newVersion="12.0.0.0" />
+        </dependentAssembly>


Can you please explain why this change is needed? Where does the request to load Newtonsoft.Json version <12 come from?

I believe @ostorc mentioned in our PR reviews meeting this week that StreamJsonRPC relies on it, but he doesn't actually depend on the part of StreamJsonRPC that depends on Newtonsoft.Json, so we were wondering if we could make an exception in ngen'ing it so that we don't have to.

In this PR microsoft/vs-streamjsonrpc#556 was the unnecessary load of Newtonsoft.Json removed, so when the version with this change is out, we will be able to remove this dangerous lines.

Unfortunately, that didn't solve the issue. Issue #5752 will solve that.

src/Shared/UnitTests/MockEngine.cs

src/Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceAsAService_Tests.cs

src/Tasks/Microsoft.Build.Tasks.csproj

src/Tasks/ResolveAssemblyReferences/Services/ResolveAssemblyReferenceComparer.cs

src/Tasks/Microsoft.Build.Tasks.csproj

Forgind

I didn't really review the MessagePack code, but the rest looks reasonable to me. It would be nice to avoid some of the long repetitive sections, but I know that isn't always possible.

src/Build/BackEnd/BuildManager/BuildManager.cs

src/Build/BackEnd/Node/RarNode.cs

src/Framework/MSBuildEventSource.cs

Forgind · 2020-09-18T21:19:07Z

src/MSBuild/app.amd64.config

+        <dependentAssembly>
+          <assemblyIdentity name="Newtonsoft.Json" culture="neutral" publicKeyToken="30ad4fe6b2a6aeed" />
+          <bindingRedirect oldVersion="0.0.0.0-12.0.0.0" newVersion="12.0.0.0" />
+        </dependentAssembly>


I believe @ostorc mentioned in our PR reviews meeting this week that StreamJsonRPC relies on it, but he doesn't actually depend on the part of StreamJsonRPC that depends on Newtonsoft.Json, so we were wondering if we could make an exception in ngen'ing it so that we don't have to.

src/Tasks.UnitTests/AssemblyDependency/Miscellaneous.cs

src/Tasks/ResolveAssemblyReferences/Contract/ReadOnlyTaskItem.cs

Forgind · 2020-09-18T22:52:35Z

src/Tasks/ResolveAssemblyReferences/Formatters/BuildEventArgsFormatter.cs

+            {
+                switch (key)
+                {
+                    case 0:


Can you explain why does this structure makes sense?

Fixed it little, not it should be more understandable. I was adopting format from MessagePack code generators

Forgind · 2020-09-18T22:54:57Z

src/Tasks/ResolveAssemblyReferences/Formatters/BuildEventArgsFormatter.cs

+
+        private BuildEventArgsFormatter() { }
+
+        #region BuildWarningEventArgs


Having not used MessagePack before, I can't say that I really understand what you're doing here.

I need this custom formatter, so I don't have to use dynamic one (which is slower on first request). The implementation follows how it is generated on runtime (with some minor tweaks).

Is there no way to specify a message in some format and autogenerate the serializer code at build time? That's what I expect from protobuf.

There is, but unfortunately it doesn't support delegating of version of nuget packages to other files, so it not reallly useful right now.

src/Tasks/ResolveAssemblyReferences/Server/RarController.cs

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

src/Framework/MSBuildEventSource.cs

src/Tasks/AssemblyDependency/ResolveAssemblyReference.cs

rainersigwald · 2020-09-22T16:58:40Z

src/Tasks/ResolveAssemblyReferences/Client/RarClient.cs

+            catch (ConnectionLostException e)
+            {
+                throw new InternalErrorException("Request failed", e);
+            }


Do you have a test to exercise this case?

No not direct one. But if we run test in RARaaS mode we can check for example that in AssemblyFoldersFromConfigFileNotFoundTest

src/Tasks/ResolveAssemblyReferences/Contract/ReadOnlyTaskItem.cs

rainersigwald · 2020-09-22T17:06:48Z

src/Tasks/ResolveAssemblyReferences/Formatters/BuildEventArgsFormatter.cs

+
+        private BuildEventArgsFormatter() { }
+
+        #region BuildWarningEventArgs


Is there no way to specify a message in some format and autogenerate the serializer code at build time? That's what I expect from protobuf.

rainersigwald · 2020-09-22T17:08:36Z

src/Tasks/ResolveAssemblyReferences/Server/RarController.cs

+
+            if (!(serverStream is NamedPipeServerStream pipeServerStream))
+            {
+                return serverStream;


Can you comment this please? I don't understand the reasoning for the early return.

This is for testing purposes. In testing scenarios I want to pass streams which I have created. And normal streams doesn't support rest of the method, so for this reason I am making early exit.

src/Build/BackEnd/Node/RarNode.cs

benvillalobos · 2020-09-29T21:42:11Z

src/Build/BackEnd/Node/RarNode.cs

            {
-                shutdownException = e;
-                return NodeEngineShutdownReason.Error;
+                return NodeEngineShutdownReason.BuildComplete;


Why are we assuming the build was completed when catching an OperationCanceledException

benvillalobos · 2020-09-29T21:45:03Z

src/Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceTestFixture.cs

-using System.Runtime.Versioning;
+using System.Threading;
+using System.Threading.Tasks;
+


Nit: Empty line

ladipro · 2021-01-08T12:07:54Z

We'll submit a fresh set of PRs as we reboot the RAR service effort. We'll be looking at this PR and the reverted #5613 for inspiration but it can be closed now.

Ondřej Štorc added 15 commits August 17, 2020 10:15

RAR Input

ea48eaa

Contract finalization

f3f4fa1

TODO: Logs are serialized in not optimal way. Find better solution

Node shutdown from comandline via cancelation token

30650da

Generilize api from NamedPipeClientStream to Stream

b4689a7

Merge branch 'rarAsService/node' into rarAsService/transfer

adfbb86

Refactoring

f70b26d

Basic tests for RAR transfer

6752388

WIP

d878818

Merge branch 'rarAsService/node'

9bb7883

WIP

ade41d8

Merge branch 'rarAsService/node' into rarAsService/transfer

c56d61a

Add node expiration timeout

7022a2f

Merge branch 'rarAsService/node' into rarAsService/transfer

ce11fa4

Merge branch 'master' into rarAsService/transfer

8675b69

WIP

ba70b3e

ostorc added the WIP label Sep 2, 2020

Forgind reviewed Sep 2, 2020

View reviewed changes

Ondřej Štorc and others added 12 commits September 10, 2020 11:19

Tests update

995b9d3

Formatters

75cd9f8

Relative path to absolute path delegate

03ace9c

WIP

578c71a

Apply suggestions from code review

c660352

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

Ref and other things

3505f72

Merge

04ae1d1

Build fix

ca44b6a

Fix test

d678b4c

Remove get rooted path from unnecessary files

dd64b8e

Build fix

70328bf

Comments fix

9f52aba

ostorc force-pushed the rarAsService/transfer branch from e40ebcf to 7f7d7c3 Compare September 10, 2020 16:18

Fix build

40d1d51

ostorc force-pushed the rarAsService/transfer branch from 7f7d7c3 to 40d1d51 Compare September 10, 2020 16:25

Build fix dotnet#2

72b6e8a

ladipro reviewed Sep 11, 2020

View reviewed changes

Ondřej Štorc added 5 commits September 11, 2020 17:27

PR comments

a3bb1f9

Merge remote-tracking branch 'ms/master' into rarAsService/transfer

49341d9

Merge remote-tracking branch 'ms/master' into rarAsService/transfer

30f52af

PR comments and refacttoring

3732920

Remove unnecessary reference

5f501a6

ostorc removed the WIP label Sep 17, 2020

ostorc changed the title ~~[WIP] RAR as a Service: Transfer and execution~~ RAR as a Service: Transfer and execution Sep 17, 2020

Forgind reviewed Sep 18, 2020

View reviewed changes

ostorc and others added 2 commits September 21, 2020 19:06

Apply suggestions from code review

92a4965

Co-authored-by: Forgind <Forgind@users.noreply.github.com>

PR comments

414616b

rainersigwald reviewed Sep 22, 2020

View reviewed changes

Formatter redo and some PR comments changes

0185da0

ostorc force-pushed the rarAsService/transfer branch from a3d2e74 to 0185da0 Compare September 23, 2020 20:12

Simplify name

72b498d

This was referenced Sep 23, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

benvillalobos reviewed Sep 29, 2020

View reviewed changes

ladipro closed this Jan 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RAR as a Service: Transfer and execution #5699

RAR as a Service: Transfer and execution #5699

ostorc commented Sep 2, 2020 •

edited

Loading

Forgind left a comment

ostorc commented Sep 10, 2020

azure-pipelines bot commented Sep 10, 2020

ladipro left a comment

ladipro Sep 11, 2020

Forgind Sep 18, 2020

ostorc Sep 22, 2020 •

edited

Loading

ostorc Sep 23, 2020

Forgind left a comment

Forgind Sep 18, 2020

Forgind Sep 18, 2020

ostorc Sep 23, 2020

Forgind Sep 18, 2020

ostorc Sep 21, 2020

rainersigwald Sep 22, 2020

ostorc Sep 23, 2020

rainersigwald Sep 22, 2020

ostorc Sep 23, 2020 •

edited

Loading

rainersigwald Sep 22, 2020

rainersigwald Sep 22, 2020

ostorc Sep 23, 2020

benvillalobos Sep 29, 2020

benvillalobos Sep 29, 2020

ladipro commented Jan 8, 2021


		private BuildEventArgsFormatter() { }

		#region BuildWarningEventArgs

RAR as a Service: Transfer and execution #5699

RAR as a Service: Transfer and execution #5699

Conversation

ostorc commented Sep 2, 2020 • edited Loading

Forgind left a comment

Choose a reason for hiding this comment

ostorc commented Sep 10, 2020

azure-pipelines bot commented Sep 10, 2020

ladipro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ostorc Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Forgind left a 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

ostorc Sep 23, 2020 • 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

ladipro commented Jan 8, 2021

ostorc commented Sep 2, 2020 •

edited

Loading

ostorc Sep 22, 2020 •

edited

Loading

ostorc Sep 23, 2020 •

edited

Loading