[Core] Fix remote project builder deadlock

mrward commented 4 years ago

UI hang can occur when the UI thread and a background thread are updating the remote project builders at the same time due to two locks being accessed in a different order.

Background thread:

RemoteBuildEngine.GetRemoteProjectBuilder - locks remoteProjectBuilders RemoteProjectBuilder.AddReference - locks usageLock

UI thread:

RemoteProjectBuilder.Shutdown - locks usageLock RemoteProjectBuilder.Dispose RemoteBuildEngine.UnloadProject RemoteBuildEngine.RemoveBuilder - locks remoteProjectBuilders

To avoid this the RemoteBuildEngine.UnloadProject is not done synchronously in the RemoteProjectBuilder's Dispose method.

Testing this the UnloadProject now generally seems to fail since the engine's connection has been closed before the UnloadProject is called. So not sure if this is the correct fix here. Same thing happens with the original code when you close the solution.

Fixes VSTS #1030374 - [FATAL] SigTerm signal in MonoDevelop.Core.dll! MonoDevelop.Projects.MSBuild.RemoteBuildEngine::RemoveBuilder+9

slluis commented 4 years ago

@monojenkins backport release-8.4

mrward commented 4 years ago

BuildManager tests are failing I assume because the builder shutdown is no longer synchronous now.

2019-12-04T16:02:41.5675150Z 1) RecycleBuildersIsGraceful (MonoDevelop.Projects.BuilderManagerTests.RecycleBuildersIsGraceful)
2019-12-04T16:02:41.5677070Z      Expected: 0
2019-12-04T16:02:41.5677610Z   But was:  1
2019-12-04T16:02:41.5677720Z 
2019-12-04T16:02:41.5678130Z   at MonoDevelop.Projects.BuilderManagerTests.RecycleBuildersIsGraceful () [0x00502] in /Users/runner/runners/2.161.1/work/1/s/monodevelop/main/tests/MonoDevelop.Core.Tests/MonoDevelop.Projects/BuilderManagerTests.cs:624 
2019-12-04T16:02:41.5678310Z 
2019-12-04T16:02:41.5678400Z 
2019-12-04T16:02:41.5678550Z 2) ReloadProject_ProjectDisposedWhilstTargetRunning_AnotherTargetRun (MonoDevelop.Projects.BuilderManagerTests.ReloadProject_ProjectDisposedWhilstTargetRunning_AnotherTargetRun)
2019-12-04T16:02:41.5679500Z      Expected: 1
2019-12-04T16:02:41.5680100Z   But was:  2
2019-12-04T16:02:41.5680550Z 
2019-12-04T16:02:41.5681050Z   at MonoDevelop.Projects.BuilderManagerTests.ReloadProject_ProjectDisposedWhilstTargetRunning_AnotherTargetRun () [0x00427] in /Users/runner/runners/2.161.1/work/1/s/monodevelop/main/tests/MonoDevelop.Core.Tests/MonoDevelop.Projects/BuilderManagerTests.cs:408

mrward commented 4 years ago

@monojenkins backport release-8.4

mrward commented 4 years ago

Changed the code so the build manager tests now work - ensure the RemoteBuildEngineManager.ReleaseProjectBuilder method is called with the current engine.

mono / monodevelop

[Core] Fix remote project builder deadlock #9427