ReaderWriterLock in PropertyDictionary #4374

rainersigwald · 2019-05-14T12:29:08Z

Fix #4368 by using a ReaderWriterLockSlim instead of a full lock. This
will allow the case where a.Equals(b) and b.Equals(a) run concurrently,
since they both only want a reader lock on their _properties: to
enumerate their own properties, and on the other to index in.

@tmeschter suggested this approach offline, and it seems obviously correct (in hindsight!).

Fix dotnet#4368 by using a ReaderWriterLockSlim instead of a full lock. This will allow the case where a.Equals(b) and b.Equals(a) run concurrently, since they both only want a reader lock on their _properties: to enumerate their own properties, and on the other to index in.

The previous implementation attempted to lock inside ToDictionary(), and then again inside GetEnumerator() from the foreach. Instead of moving to a recursive lock, which is discouraged in the ReaderWriterLockSlim docs, access _properties.Values directly in ToDictionary().

tmeschter · 2019-05-14T16:30:15Z

On a related note, is there any possibility that one of the PropertyDictionarys could be modified during an Equals call?

Clearly this won't happen in the particular case where a.Equals(b) and b.Equals(a) run concurrently, as each will hold a read lock on their own data. But if just a.Equals(b) is running then currently nothing prevents concurrent modifications to b. Is that a problem?

tmeschter · 2019-05-14T16:30:42Z

src/Build/Collections/PropertyDictionary.cs

 using System.Diagnostics;
 using Microsoft.Build.Shared;
 using Microsoft.Build.Evaluation;
+using System.Threading;


Nit: ordering.

danmoseley · 2019-05-21T01:27:31Z

src/Build/Collections/PropertyDictionary.cs

            get
            {
-                lock (_properties)
+                _lock.EnterReadLock();


Maybe worth creating a struct that implements IDisposable so you can use a using statement instead of adding a try/finally with new indent.

danmoseley · 2019-05-21T01:29:28Z

src/Build/Collections/PropertyDictionary.cs

+        /// </summary>
+        ~PropertyDictionary()
+        {
+            _lock?.Dispose();


Is this needed? If the finalizer thread is used to run dispose on the lock, why not just cut out the middleman and let the finalizer thread finalize the lock.

Does the slim version even require disposing?

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

It's not clear to me whether it does or not. I was aping the ReaderWriterLockSlim doc example. But I see several current examples in CoreFX that don't dispose it, like CodePagesEncodingProvider and Privilege. So I guess it's ok to skip it.

@stephentoub any reason we're inconsistent?

any reason we're inconsistent?

In Privilege, it's static.

And CodePagesEncodingProvider exposes a static singleton that contains the instance, so it's essentially static there as well.

Does the slim version even require disposing?

ReaderWriterLockSlim wraps several event wait handles. If the RWLS isn't disposed, they'll be left for finalization.

rainersigwald · 2019-07-10T16:43:11Z

/azp run

azure-pipelines · 2019-07-10T16:45:29Z

Azure Pipelines successfully started running 1 pipeline(s).

Using the formula given in https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/lock-statement

…rable it sometimes uses

danmoseley · 2019-07-10T20:46:04Z

src/Build/Collections/CopyOnReadEnumerable.cs

        /// <summary>
        /// The object used to synchronize access for copying.
        /// </summary>
        private readonly object _syncRoot;


You can eliminate this field now and use the RW lock? It doesn't matter what the object is so long as it's readonly and private. That will claw back the size increase in this object form adding the RW lock field.

Disregard, I didn't view the full diff. You still use that syncroot in another path.

cdmihai · 2019-07-10T21:16:36Z

src/Build/Collections/PropertyDictionary.cs

+        /// <summary>
+        /// Lock object to guard access to backing collection.
+        /// </summary>
+        private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();


Would be nice to reuse the DisposableReaderWriterLockSlim, and maybe make it its own file.

In https://github.com/cdmihai/msbuild/blob/c8660645471a185287148f9e0897a3ec9c1ec3c8/src/Build/Definition/ProjectCollection.cs#L87

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

…ry-readerwriterlockslim

…y-readerwriterlockslim

This reverts commit 7b931cf.

This is necessary for the GetCopyOnReadEnumerable case; it gets a read lock in GetCopyOnReadEnumerable.GetEnumerator and then calls PropertyDictionary.GetEnumerator within that.

stephentoub · 2019-07-11T20:42:46Z

src/Build/Collections/PropertyDictionary.cs

        IEnumerator<KeyValuePair<string, T>> IEnumerable<KeyValuePair<string, T>>.GetEnumerator()
        {
-            lock (_properties)
+            _lock.EnterReadLock();


This method was and continues to be highly suspect. Maybe it's fine because you control all the callers as well, but the lock is going to be held while this method yields. In other words:

PropertyDictionary pd = ...; var e = pd.GetEnumerator(); e.MoveNext(); ... // lock is held here!

rainersigwald force-pushed the propertydictionary-readerwriterlockslim branch from 7122e2c to 9194da0 Compare May 14, 2019 12:45

rainersigwald added 2 commits May 14, 2019 06:19

rainersigwald force-pushed the propertydictionary-readerwriterlockslim branch from 9194da0 to ddf872e Compare May 14, 2019 13:21

tmeschter reviewed May 14, 2019

View reviewed changes

danmoseley reviewed May 21, 2019

View reviewed changes

rainersigwald added 3 commits July 10, 2019 14:21

fixup! ReaderWriterLock in PropertyDictionary

0de9fc2

Expand lock in CopyOnReadEnumerable

4bf72b6

Using the formula given in https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/lock-statement

Pass PropertyDictionary's ReaderWriterLockSlim to the CopyOnReadEnume…

7c65b97

…rable it sometimes uses

danmoseley reviewed Jul 10, 2019

View reviewed changes

cdmihai reviewed Jul 10, 2019

View reviewed changes

Merge remote-tracking branch 'Microsoft/master' into propertydictiona…

15c5052

…ry-readerwriterlockslim

This was referenced Jul 11, 2019

Disable intermittently-hanging tests #4501

Merged

Copy sources from Versions.props to NuGet.config #4500

Merged

rainersigwald added 3 commits July 11, 2019 12:32

Merge remote-tracking branch 'upstream/master' into propertydictionar…

f9025d1

…y-readerwriterlockslim

Revert "Disable GraphBuild tests to avoid dotnet#4368 (dotnet#4501)"

51d0f5e

This reverts commit 7b931cf.

Allow recursion

ad7be43

This is necessary for the GetCopyOnReadEnumerable case; it gets a read lock in GetCopyOnReadEnumerable.GetEnumerator and then calls PropertyDictionary.GetEnumerator within that.

stephentoub reviewed Jul 11, 2019

View reviewed changes

rainersigwald added this to the MSBuild 16.4 milestone Aug 20, 2019

livarcocc assigned rainersigwald Oct 8, 2019

livarcocc modified the milestones: MSBuild 16.4, MSBuild 16.5 Oct 8, 2019

rainersigwald modified the milestones: MSBuild 16.5, MSBuild 16.6 Feb 24, 2020

rainersigwald modified the milestones: MSBuild 16.6, MSBuild 16.7 Mar 3, 2020

rainersigwald mentioned this pull request Mar 17, 2020

Deadlock in PropertyDictionary equality #4368

Closed

rainersigwald closed this Mar 17, 2020

ReaderWriterLock in PropertyDictionary #4374

ReaderWriterLock in PropertyDictionary #4374

Uh oh!

Conversation

rainersigwald commented May 14, 2019

Uh oh!

tmeschter commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

rainersigwald Jul 10, 2019 • 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!

rainersigwald commented Jul 10, 2019

Uh oh!

azure-pipelines bot commented Jul 10, 2019

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!

stephentoub Jul 11, 2019 • 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!

6 participants

tmeschter commented May 14, 2019 •

edited

Loading

rainersigwald Jul 10, 2019 •

edited

Loading

stephentoub Jul 11, 2019 •

edited

Loading