Fix flaky compiler server test #23518

khyperia · 2017-12-01T16:35:29Z

MultipleSimultaneousCompiles calls RunCommandLineCompiler on multiple
threads (using Task.Run), which in turn calls CreateFiles on multiple
threads, which in turn goes through TempDirectory and TempRoot and
eventually boils down to calling List<T>.Add(), which is not
thread-safe, and so occasionally crashes.

Solution: hit the problem with a hammer by throwing a lock around CreateFiles.

Fixes #22913

ghost · 2017-12-01T16:49:20Z

src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs

-                    file.WriteAllText(pair.Value);
+                    foreach (var pair in files)
+                    {
+                        TempFile file = currentDirectory.CreateFile(pair.Key);


Since CreateFile is the not thread safe line, could you simply lock around it, and return the pointer to the file, and then do the IO to the file? (Locking inside the loop, instead of around it.)
Locking the WriteAllText seems like the lock would be held for longer than required, if the various tasks would all be writing to different files.
Main cost of locks is waiting for them to free, not actually taking/releasing them, therefore taking them often, but locking for the minimal amount of time often gives the most benefits from having parallel execution.

That's theoretically possible, yes. However, this is in a test, not in production code, and that level of optimization isn't really needed, and just complicates maintainability. (Files being written here are also probably <1KB on average)

If we really cared about performance here, a better solution would be to make TempRoot use a collection designed for concurrency and performance.

jaredpar · 2017-12-01T16:54:39Z

src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs

@@ -109,14 +109,18 @@ private static void ReferenceNetstandardDllIfCoreClr(List<string> arguments)
 #endif
        }

+        private static readonly object CreateFilesLock = new object();


Use s_createFilesLock for this variable.

MultipleSimultaneousCompiles calls RunCommandLineCompiler on multiple threads (using Task.Run), which in turn calls CreateFiles on multiple threads, which in turn goes through TempDirectory and TempRoot and eventually boils down to calling List<T>.Add(), which is not thread-safe, and so occasionally crashes. Solution: hit the problem with a hammer by throwing a lock around CreateFiles.

jasonmalinowski · 2017-12-01T17:54:59Z

src/Compilers/Server/VBCSCompilerTests/CompilerServerTests.cs

@@ -109,14 +109,18 @@ private static void ReferenceNetstandardDllIfCoreClr(List<string> arguments)
 #endif
        }

+        private static readonly object s_createFilesLock = new object();


Should this lock be inside TempDirectory so we don't get bit by this at some later point? We have plenty of other tests in the IDE layer at least that do multithreading that I now wonder if they're also broken.

khyperia · 2017-12-01T18:14:31Z

Should this lock be inside TempDirectory

I guess?? Done 😊

jaredpar · 2017-12-04T17:00:33Z

src/Test/Utilities/Portable/TempFiles/TempRoot.cs

 using System.IO;
 using System.Runtime.CompilerServices;

 namespace Microsoft.CodeAnalysis.Test.Utilities
 {
    public sealed class TempRoot : IDisposable
    {
-        private readonly List<IDisposable> _temps = new List<IDisposable>();
+        private readonly ConcurrentBag<IDisposable> _temps = new ConcurrentBag<IDisposable>();


I prefer the earlier version of this change. This type was not designed with thread safety in mind and I'm skeptical of making it more free threaded. In particular I'm always skeptical of types which are a) disposable and b) writable from multiple threads.

Okay, then you and @jasonmalinowski need to talk. Both of you are giving conflicting feedback.

My major objection comes down to Dispose. Mutation after Dispose should be an error. Enforcing that contract though adds a bit more state and complexity to the TempRoot object. Enough that I question if it's worth it to support the single case where mutate TempRoot from multiple threads vs. just adding the lock in that one case.

@jaredpar If you can prove that was the only test in our entire codebase using this in a multi-threaded way, I'll be OK with the original fix. Keep in mind this tool is used beyond just the compiler tests, so don't restrict your statements to that. Otherwise, let's just fix it for good before we create another flaky test down the road.

I don't want perfect to be the enemy of "leaving traps for the next person" here.

Okay, but what about the IDE tests Jason is talking about? Discussions about IDisposable aren't really relevant (it's simple enough to add tracking vs. finding/fixing all the IDE tests, and/or validating the IDE tests aren't broken)

Edit: Ack, didn't see that Jason submitted a comment at the same time.

jaredpar · 2017-12-04T17:02:12Z

src/Test/Utilities/Portable/TempFiles/TempRoot.cs

-        private static void DisposeAll(IEnumerable<IDisposable> temps)
-        {
-            foreach (var temp in temps)
+            while (_temps.TryTake(out var temp))


In general Dispose is a well known state transition for an object from usable to dead. This code though is allowing mutations, after dispose, to silently work.

khyperia added the Area-Infrastructure label Dec 1, 2017

khyperia requested a review from a team December 1, 2017 16:35

ghost reviewed Dec 1, 2017

View reviewed changes

jaredpar reviewed Dec 1, 2017

View reviewed changes

khyperia force-pushed the flaky_server_test branch from 57f428f to 23ca701 Compare December 1, 2017 17:05

jasonmalinowski reviewed Dec 1, 2017

View reviewed changes

Make TempRoot thread-safe

d641eaa

jaredpar reviewed Dec 4, 2017

View reviewed changes

Add disposal tracking for TempRoot

b9bc86b

jaredpar approved these changes Dec 6, 2017

View reviewed changes

khyperia merged commit 906d5cb into dotnet:master Dec 7, 2017

khyperia deleted the flaky_server_test branch December 7, 2017 18:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix flaky compiler server test #23518

Fix flaky compiler server test #23518

khyperia commented Dec 1, 2017

ghost Dec 1, 2017

khyperia Dec 1, 2017

jaredpar Dec 1, 2017

jasonmalinowski Dec 1, 2017

khyperia commented Dec 1, 2017

jaredpar Dec 4, 2017

khyperia Dec 4, 2017

jaredpar Dec 4, 2017

jasonmalinowski Dec 4, 2017

khyperia Dec 4, 2017 •

edited

Loading

jaredpar Dec 4, 2017

Fix flaky compiler server test #23518

Fix flaky compiler server test #23518

Conversation

khyperia commented Dec 1, 2017

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

khyperia commented Dec 1, 2017

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

khyperia Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khyperia Dec 4, 2017 •

edited

Loading