async multithreading support #2

alexsku · 2016-12-21T03:34:40Z

This is my attempt to improve multithreading support for miniprofiler v4. I think the current version of the mini profiler would have issues in scenarios where the current thread spawns more than one task and waits for them.

        [Fact]
        public void Head_WhenMultipleTasksSpawned_EachSetsItsOwnHead()
        {
            MiniProfiler.Settings.ProfilerProvider = new DefaultProfilerProvider();
            var allTasks = new SemaphoreSlim(0, 1);
            var completed = new TaskCompletionSource<int>();
            using (GetRequest("http://localhost/Test.aspx", startAndStopProfiler: false))
            {
                MiniProfiler.Start();
                var sut = MiniProfiler.Current;
                var head = sut.Head;

                Task.Run(() => {
                    Assert.Equal(head, sut.Head);
                    using (sut.Step("test1"))
                    {
                        allTasks.Release();
                        completed.Task.Wait();
                    }
                });
                allTasks.Wait();
                Task.Run(() => {
                    using (sut.Step("test2"))
                    {
                        allTasks.Release();
                        completed.Task.Wait();
                    }
                });
                allTasks.Wait();

                Assert.NotNull(sut.Head);
                Assert.Equal(head, sut.Head);
                completed.SetResult(0);
            }
        }

In this scenario there would be multiple threads working with the mini profiler (since they would share it via AsyncLocal). This could lead to random failures since Timing class is not thread safe. Also this could lead to inconsistencies since the spawned tasks would replace the Head timing for the mini profiler instance, with one of the tasks's timing potentially becoming a child of another's.

alexsku · 2016-12-21T03:36:57Z

src/MiniProfiler.Shared/Helpers/Net45/AsyncLocal.cs

+    /// Unlike DbContextScope's implementation this implementation doesn't support serialization/deserialization 
+    /// ths not allowing to cross app domain barrier.
+    /// </summary>
+    internal class AsyncLocal<T>


a polyfill for AsyncLocal which is not available before .Net 4.6

alexsku · 2016-12-21T03:42:04Z

src/MiniProfiler.Shared/MiniProfiler.cs

+            get
+            {
+                Settings.EnsureProfilerProvider();
+                return Settings.ProfilerProvider.CurrentHead;


It is possible that the current thread spawns more than one task on separate threads and wait for them. As a result technically it is possible that there would be more than one separate head for a miniprofiler instance (one for each spawned thread). This code offloads managing heads to the profiler provider, so if the provider handles spawning threads (i.e. it relies on AsyncLocal or CallContext) then it should be able to manage head properly.

alexsku · 2016-12-21T03:43:21Z

src/MiniProfiler.Shared/ProfileProviders/DefaultProfilerProvider.cs

 using System.Runtime.Remoting.Messaging;
-using System.Web;
+using System.Web; 
+using StackExchange.Profiling.Helpers.Net45;


Since there is AsyncLocal polyfill we can keep the implementation of the provider the same - it will be either using the polyfill or .net version of AsyncLocal

alexsku · 2016-12-21T03:47:25Z

src/MiniProfiler.Shared/Timing.cs

        /// </summary>
        [DataMember(Order = 5)]
-        public List<Timing> Children { get; set; }
+        public List<Timing> Children


since technically there could be multiple threads working with the same Timing instance at the same time (my example above with a thread spawning two tasks and waiting for them) Children and CustomTimings need to be protected from multithreaded access. Here I added a backing field _children and protected it with _lockObject. The property always return a copy of _children so the caller can iterate through the list safely. I didn't see any code that would attempt to modify the children of a Timing via Children property so I think nothing is broken by this change

Ideally the property should return IReadOnlyList (by just casting the created List) but protobuf doesn't really work well with IReadOnlyList (due to this issue: protobuf-net/protobuf-net#90). It might be possible to use IImmutableList (backed by ImmutableArray) which is supported by protobuf. But the problem would be with CustomTimings property which returns Dictionary. IReadOnlyDictionary would not work with protobuf either, and ImmutableDictionary has significantly worse performance comparing to Dictionary which makes me reluctant to use it.

Another option is to change the property to GetChildren/SetChildren method pair to emphasize that there are not just access to the backing filed but actually copy the contents of it.

Please let me know what you prefer I can change it

alexsku · 2016-12-21T03:48:24Z

src/MiniProfiler.Shared/Timing.cs

        /// </summary>
        [DataMember(Order = 6)]
-        public Dictionary<string, List<CustomTiming>> CustomTimings { get; set; }
+        public Dictionary<string, List<CustomTiming>> CustomTimings


Similarly to Children CustomTimings is backed by a field (_customTimings) and is protected with _lockObject

alexsku · 2016-12-21T03:50:32Z

src/MiniProfiler/ProfileProviders/WebRequestProfilerProvider.cs

+        /// <summary>
+        /// Current head timing.
+        /// </summary>
+        public override Timing CurrentHead


I'm not sure what is the intended future of WebRequestProfilerProvider and why DefaultProfilerProvider cannot be used all the time. I added this property only to support the changes to the interface.

Another option is to "merge" WebRequestProfilerProvider and DefaultProfilerProvider, in this case WebRequestProfilerProvider.Current and WebRequestProfilerProvider.CurrentHead would be backed by AsyncLocals (or the polyfills). I don't see specific downside with this and the upside is that this provider would support async controller actions (that are using .ConfigureAwait(false) internally).

alexsku · 2016-12-21T03:54:22Z

tests/MiniProfiler.Tests/MiniProfilerTest.cs

+            }
+        }
+
+        [Fact]


this test is a demonstration of the scenario i'm trying to support when one thread spawns two tasks and wait for their completion. In this scenario I would expect the tasks's Timings to be children of the main thread's Timing instance

unit tests are no longer run in parallel

alexsku · 2016-12-22T01:53:41Z

src/MiniProfiler.Shared/Timing.cs

        /// </summary>
        [DataMember(Order = 5)]
-        public List<Timing> Children { get; set; }
+        public IImmutableList<Timing> Children


so it looks like IImmutableList works with the protobuf library, however i don't see a good alternative to Dictionary, there is ImmutableDictionary but it has poor performance comparing to the regular dictionary, i think the best option is to keep List and Dictionary until protobuf is fixed

alexsku · 2016-12-22T01:56:21Z

tests/MiniProfiler.Tests/App.config

    </DbProviderFactories>
  </system.data>
+  <appSettings>
+    <add key="xunit.parallelizeTestCollections" value="false"/>


it looks like xunit prefers to run unit tests in parallel, which causes random issues when static fields (e.g. ProfilerProvider) are getting changed. I think the best is to disable parallelization, i think this disables it (https://xunit.github.io/docs/configuring-with-xml.html)

alexsku · 2016-12-22T01:57:56Z

tests/MiniProfiler.Tests/BaseTest.cs

            var props = from p in typeof(T).GetProperties(BindingFlags.Public | BindingFlags.Instance)
                        where p.IsDefined(typeof(System.Runtime.Serialization.DataMemberAttribute), false)
-                        && !p.PropertyType.GetInterfaces().Any(i => i.Equals(typeof(IDictionary)) || i.Equals(typeof(IList)))
+                        && !p.PropertyType.GetInterfaces().Any(i => i.Equals(typeof(IEnumerable)))


I think most of implementations of enumerable cannot be compared using Object.Equals, so i think this is the best catch all case for all enumerable (otherwise every time i try to use IImmutableList the tests fail since is not an IList)

alexsku · 2016-12-22T01:59:24Z

tests/MiniProfiler.Tests/Storage/SqlServerStorageTest.cs


        public SqlServerStorageTest(SqlCeStorageFixture<SqlServerStorageTest> fixture)
        {
+            MiniProfiler.Settings.ProfilerProvider = new WebRequestProfilerProvider();


the test should be using WebRequestProfilerProvider the DefaultProfilerProvider doesn't save the stopped miniprofiler instances and fails these tests.

async multithreading support

5e6bfcf

alexsku commented Dec 21, 2016

View reviewed changes

alexsku added 2 commits December 21, 2016 14:14

minor improvements to support mutithreading

35ecbfb

returning IImmutableList instead of List

5805953

unit tests are no longer run in parallel

alexsku commented Dec 22, 2016

View reviewed changes

reverted back to using List as return value of Children

0000723

+                          }
+                      }
+                      [Fact]

async multithreading support #2

Are you sure you want to change the base?

async multithreading support #2

Uh oh!

Conversation

alexsku commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexsku Dec 21, 2016 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexsku commented Dec 21, 2016 •

edited

Loading

alexsku Dec 21, 2016 •

edited

Loading