Strong detection for List/Dictionary concurrent use in Debug #17076

benaadams · 2018-03-20T21:33:08Z

Bit ugly, but should do the trick?

Resolves: https://github.com/dotnet/coreclr/issues/17070

benaadams · 2018-03-20T21:34:31Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

sharwell · 2018-03-20T22:06:47Z

src/mscorlib/shared/System/Collections/Generic/Dictionary.cs

@@ -1041,6 +1191,9 @@ ICollection IDictionary.Values
                {
                    ThrowHelper.ThrowWrongKeyTypeArgumentException(key, typeof(TKey));
                }
+#if DEBUG


❓ Why bother with #if DEBUG here if the target method is marked [Conditional("DEBUG")]?

If you do that it might be worth naming something like as DebugOnlyConcurrentAccessCheck so it's obvious it's debug only

Wasn't sure the Jit would elide all the int version = _version; calls (or C# delete them and not produce an error about not using it)

Will change an check diffs as it reduces the number of #ifs considerably

danmoseley · 2018-03-20T22:23:51Z

src/mscorlib/shared/System/Collections/Generic/Dictionary.cs

@@ -214,14 +215,26 @@ public ValueCollection Values
        {
            get
            {
+#if DEBUG
+                int version = _version;


if you're using the existing _version field doesn't it have to be volatile or incremented with Interlocked?

@stephentoub

In DEBUG its incremented in ConcurrentAccessWriteCheck with

Interlocked.CompareExchange(ref _version, version + 1, version) != version

Changed to

if (Interlocked.Exchange(ref _version, version + 1) != version) { ThrowHelper.ThrowInvalidOperationException_ConcurrentOperationsNotSupported(); }

As don't care if it stops version being updated; just that it throws if the version wasn't correct

benaadams · 2018-03-21T00:33:56Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

jkotas · 2018-03-21T03:27:57Z

The barriers from these interlocked operations can hide some types of the races as well.

Do we have any examples of bugs in CoreFX that were caused by invalid multi-threaded Dictionary use and that were in the product for a long time because of they were hard to hit and find? It would be nice to validate the effectiveness of this on some real world examples.

benaadams · 2018-03-21T04:11:01Z

Its generating the asm for the [Conditional("DEBUG")] in Release; though not calling it, also not eliding the _version use :sad: back to #if DEBUG

Total bytes of diff: 1604 (0.05% of base)
    diff is a regression.

Total byte diff includes 61 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    2 unique methods,       61 unique bytes

Top file regressions by size (bytes):
        1604 : System.Private.CoreLib.dasm (0.05% of base)

1 total files with size differences (0 improved, 1 regressed), 0 unchanged.

Top method regessions by size (bytes):
          88 : System.Private.CoreLib.dasm - Dictionary`2:.ctor(ref,ref):this (58 methods)
          87 : System.Private.CoreLib.dasm - Dictionary`2:CopyTo(ref,int):this (29 methods)
          87 : System.Private.CoreLib.dasm - Dictionary`2:System.Collections.IDictionary.get_Item(ref):ref:this (29 methods)
          75 : System.Private.CoreLib.dasm - Dictionary`2:TrimExcess(int):this (25 methods)
          58 : System.Private.CoreLib.dasm - Dictionary`2:Resize(int,bool):this (29 methods)

Top method improvements by size (bytes):
        -203 : System.Private.CoreLib.dasm - Dictionary`2:Clear():this (29 methods)

164 total methods with size differences (1 improved, 163 regressed), 17109 unchanged.

benaadams · 2018-03-21T04:29:44Z

Total bytes of diff: -66 (0.00% of base)
    diff is an improvement.

Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes

Top file improvements by size (bytes):
         -66 : System.Private.CoreLib.dasm (0.00% of base)

1 total files with size differences (1 improved, 0 regressed), 0 unchanged.

Top method regessions by size (bytes):
          75 : System.Private.CoreLib.dasm - Dictionary`2:TrimExcess(int):this (25 methods)
          58 : System.Private.CoreLib.dasm - Dictionary`2:Resize(int,bool):this (29 methods)
          32 : System.Private.CoreLib.dasm - PseudoCustomAttribute:GetCustomAttributes(ref,ref,byref):ref (9 methods)
          32 : System.Private.CoreLib.dasm - PseudoCustomAttribute:IsDefined(ref,ref):bool (9 methods)
           8 : System.Private.CoreLib.dasm - Attribute:CopyToArrayList(ref,ref,ref)
           8 : System.Private.CoreLib.dasm - MemberInfoCache`1:PopulateInterfaces(struct):ref:this
           8 : System.Private.CoreLib.dasm - MemberInfoCache`1:PopulateEvents(struct,ref,ref,byref):this
           8 : System.Private.CoreLib.dasm - EventSource:DebugCheckEvent(byref,ref,ref,ref,ref,int)
           8 : System.Private.CoreLib.dasm - ManifestBuilder:AddEventParameter(ref,ref):this
           8 : System.Private.CoreLib.dasm - SerializationInfo:AddValueInternal(ref,ref,ref):this
           8 : System.Private.CoreLib.dasm - EventRegistrationTokenTable`1:AddEventHandlerNoLock(ref):struct:this

Top method improvements by size (bytes):
        -319 : System.Private.CoreLib.dasm - Dictionary`2:Clear():this (29 methods)

12 total methods with size differences (1 improved, 11 regressed), 17261 unchanged.

Regression in Dictionary is from incrementing _version in TrimExcess and Resize

Other regressions are from ContainsKey(TKey key) deciding to inline now that it gets the bool to a local and returns it; rather than returning the bool directly - which seems a bit weird?

danmoseley · 2018-03-21T05:29:20Z

@jkotas this was motivated by https://github.com/dotnet/corefx/issues/28198. I assume there have been other cases periodically?

benaadams · 2018-03-21T06:15:48Z

Is there a way to run coreclr (checked or debug) against corefx outerloop?

benaadams · 2018-03-21T06:40:50Z

Triggered some tests that were checking version didn't change

System.Collections.Tests.Dictionary_Generic_Tests_string_string.EnsureCapacity_Generic_RequestingLargerCapacity_DoesNotInvalidateEnumeration(count: 1) [FAIL]
   System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
   Stack Trace:
         at System.Collections.Generic.Dictionary`2.KeyCollection.Enumerator.MoveNext()
      D:\j\workspace\x64_checked_w---d7295605\_\fx\src\System.Collections\tests\Generic\Dictionary\Dictionary.Generic.Tests.netcoreapp.cs(104,0): at System.Collections.Tests.Dictionary_Generic_Tests`2.EnsureCapacity_Generic_RequestingLargerCapacity_DoesNotInvalidateEnumeration(Int32 count)
System.Collections.Tests.Dictionary_Generic_Tests_string_string.EnsureCapacity_Generic_RequestingLargerCapacity_DoesNotInvalidateEnumeration(count: 75) [FAIL]
   System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
   Stack Trace:
         at System.Collections.Generic.Dictionary`2.KeyCollection.Enumerator.MoveNext()
      D:\j\workspace\x64_checked_w---d7295605\_\fx\src\System.Collections\tests\Generic\Dictionary\Dictionary.Generic.Tests.netcoreapp.cs(104,0): at System.Collections.Tests.Dictionary_Generic_Tests`2.EnsureCapacity_Generic_RequestingLargerCapacity_DoesNotInvalidateEnumeration(Int32 count)

benaadams · 2018-03-21T06:44:49Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

jkotas · 2018-03-21T07:03:12Z

@jkotas this was motivated by dotnet/corefx#28198

https://github.com/dotnet/corefx/issues/28198#issuecomment-374676611 says that we do not have any tests for the buggy code. If it is the case, this would not help us to find this issue faster.

benaadams · 2018-03-21T14:21:35Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

benaadams · 2018-03-21T15:43:23Z

Linux - CoreFx issues unrelated

Test Result (4 failures / ±0)
System.IO.Tests.FileInfo_GetSetTimes.TimesIncludeMillisecondPart_Linux
System.IO.Tests.File_GetSetTimes.TimesIncludeMillisecondPart_Linux
System.IO.Tests.DirectoryInfo_GetSetTimes.TimesIncludeMillisecondPart_Linux
System.IO.Tests.Directory_GetSetTimes.TimesIncludeMillisecondPart_Linux

## TimesIncludeMillisecondPart got a file time of 2018-03-21T14:57:32.0000000Z on ext2
## TimesIncludeMillisecondPart got a file time of 2018-03-21T14:57:33.0000000Z on ext2
## TimesIncludeMillisecondPart got a file time of 2018-03-21T14:57:34.0000000Z on ext2
## TimesIncludeMillisecondPart got a file time of 2018-03-21T14:57:36.0000000Z on ext2
## TimesIncludeMillisecondPart got a file time of 2018-03-21T14:57:37.0000000Z on ext2
 System.IO.Tests.FileInfo_GetSetTimes.TimesIncludeMillisecondPart_Linux [FAIL]
  Assert.All() Failure: 6 out of 6 items in the collection did not pass.
  [5]: Xunit.Sdk.NotEqualException: Assert.NotEqual() Failure
   Expected: Not 0
   Actual:   0
    at Xunit.Assert.NotEqual[T](T expected, T actual, IEqualityComparer`1 comparer)
    at System.IO.Tests.BaseGetSetTimes`1.<>c__DisplayClass9_0.<TimesIncludeMillisecondPart_Linux>b__0(TimeFunction function) in /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs:line 108
    at Xunit.Assert.All[T](IEnumerable`1 collection, Action`1 action)
  [4]: Xunit.Sdk.NotEqualException: Assert.NotEqual() Failure
   Expected: Not 0
   Actual:   0
    at Xunit.Assert.NotEqual[T](T expected, T actual, IEqualityComparer`1 comparer)
    at System.IO.Tests.BaseGetSetTimes`1.<>c__DisplayClass9_0.<TimesIncludeMillisecondPart_Linux>b__0(TimeFunction function) in /mnt/j/workspace/dotnet_coreclr/master/jitstress/x64_checked_ubuntu_corefx_baseline_prtest/_/fx/src/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs:line 108
    at Xunit.Assert.All[T](IEnumerable`1 collection, Action`1 action)

danmoseley · 2018-03-21T15:46:04Z

@stephentoub thoughts about existence of real world examples?

benaadams · 2018-03-21T16:41:24Z

Windows_NT x64 Checked Build and Test (Jit - CoreFx) failure

System.ArgumentException: ComposablePartDefinition of type 'System.ComponentModel.Composition.AttributedModel.ConcreteCPD' cannot be used in this context.
 Only part definitions produced by the ReflectionModelServices.CreatePartDefinition are supported.
Parameter name: partDefinition
 at System.ComponentModel.Composition.AttributedModelServices.CreatePart(ComposablePartDefinition partDefinition, Object attributedPart) in D:\j\workspace\x64_checked_w---d7295605\_\fx\src\System.ComponentModel.Composition\src\System\ComponentModel\Composition\AttributedModelServices.cs:line 49
 at System.ComponentModel.Composition.AttributedModel.AttributedModelServicesTests.<>c.<CreatePart_From_InvalidPartDefiniton_ShouldThrowArgumentException>b__2_0() in D:\j\workspace\x64_checked_w---d7295605\_\fx\src\System.ComponentModel.Composition\tests\System\ComponentModel\Composition\AttributedModelServicesTests.cs:line 65

benaadams · 2018-03-21T16:42:02Z

@danmosemsft it didn't catch anything 😢

Is this change worth it?

danmoseley · 2018-03-21T16:43:56Z

I figured stress runs would be more likely to catch something. As to whether it's worth it, I'm open minded, I'm curious whether @stephentoub has seen real cases.

Do you understand this gain - is it worth keeping?

Top method improvements by size (bytes):
        -319 : System.Private.CoreLib.dasm - Dictionary`2:Clear():this (29 methods)

benaadams · 2018-03-21T16:48:22Z

Do you understand this gain - is it worth keeping?

Added PR for the gains :) #17096

stephentoub · 2018-03-22T14:41:39Z

I'm curious whether @stephentoub has seen real cases.

I've seen real cases, e.g. the SQL one I fixed earlier in the week, but as Jan restated this wouldn't have caught that as we didn't have any tests that exercised those code paths, nevermind ones that would have exercised it in parallel.

I just did a search for "static Dictionary", "static readonly Dictionary", "readonly static Dictionary" in corefx (thankfully there's only one of the latter, as that's not the desired ordering), and did a cursory review of each's usage. A few things I noticed:

This is buggy: https://github.com/dotnet/corefx/blob/619dbe6503a2de2afa27122cb34716d9ff6906ff/src/System.Runtime.Serialization.Xml/tests/SerializationTestTypes/DataContractResolverLibrary.cs#L16-L34. But it's in tests, and it would only manifest if tests that were using this type were to run concurrently; it looks like all tests that use it are in the same xunit class, so they won't run concurrently. Still, it's probably worth fixing just to avoid issues in the future should usage here be extended.
Just based on brief code inspection, these look problematic: https://github.com/dotnet/corefx/blob/619dbe6503a2de2afa27122cb34716d9ff6906ff/src/Common/tests/System/Runtime/Serialization/Utils.cs#L38-L39. Those dictionaries are read/written without locks from various static helper methods. These could be in the same camp as above, though, where they're "safe" only because all tests that use them end up getting serialized.
In the same boat: https://github.com/dotnet/corefx/blob/619dbe6503a2de2afa27122cb34716d9ff6906ff/src/System.ComponentModel.Composition/tests/System/ComponentModel/Composition/DynamicMetadata.cs#L144. It's accessed in an unsafe way, but it appears to be ok because the tests that use it are serialized by xunit. If we were to ever refactor these tests to live in different test classes, this could become a test bug.
There's a potential product issue here, though not one these changes would likely catch: https://github.com/dotnet/corefx/blob/619dbe6503a2de2afa27122cb34716d9ff6906ff/src/System.Data.SqlClient/src/System/Data/SqlClient/SqlConnectionString.cs#L488-L492. cc: @saurabh500. There's no synchronization whatsoever (no locks, no interlockeds, no volatiles, etc.) used in lazily initializing this dictionary. It's possible that could be an issue on some hardware.
This looks dangerous: https://github.com/dotnet/corefx/blob/bb399884797d73f5401c637ed861745b97f0fd92/src/System.Private.DataContractSerialization/src/System/Runtime/Serialization/DataContract.cs#L30-L36. But I also can't find any references to GetDataContracts() elsewhere in corefx, so usage isn't clear. And enough care was taken with other shared dictionaries elsewhere in the file that presumably the author here knew what they were doing.
This looks like a product bug, and one that this change could potentially catch (cc: @shmao). https://github.com/dotnet/corefx/blob/bb399884797d73f5401c637ed861745b97f0fd92/src/System.Private.DataContractSerialization/src/System/Runtime/Serialization/ExtensionDataReader.cs#L487-L496. But again I don't have enough context as to usage, and I don't know if we have sufficient tests in place to help this change catch it. Similarly it's suspicious that there are multiple static methods that access another dictionary on this type, writing https://github.com/dotnet/corefx/blob/bb399884797d73f5401c637ed861745b97f0fd92/src/System.Private.DataContractSerialization/src/System/Runtime/Serialization/ExtensionDataReader.cs#L501-L505 and reading https://github.com/dotnet/corefx/blob/bb399884797d73f5401c637ed861745b97f0fd92/src/System.Private.DataContractSerialization/src/System/Runtime/Serialization/ExtensionDataReader.cs#L209 and without explicit synchronization, at least at this level.

In short, across the large codebase that is corefx, it's possible this might catch something, but I wouldn't expect significant wins.

jkotas · 2018-06-25T19:20:03Z

I have opened https://github.com/dotnet/corefx/issues/30651 and https://github.com/dotnet/corefx/issues/30650 on the issues that Stephen identified by code inspection.

jkotas · 2018-06-25T19:22:51Z

Otherwise, I do not think that this is worth the extra DEBUG #ifdefs. I think that these ifdefs have about the same chance of catching problems when running debug CoreCLR build as introducing new ones.

@benaadams Thanks for the suggested change anyway.

sharwell reviewed Mar 20, 2018

View reviewed changes

danmoseley reviewed Mar 20, 2018

View reviewed changes

benaadams force-pushed the dict-debig-concurrency branch from 1914ca9 to c9646e4 Compare March 21, 2018 00:16

benaadams force-pushed the dict-debig-concurrency branch from b6f9740 to 3363111 Compare March 21, 2018 05:52

Strong detection for Dictionary concurrent use in Debug

c34bfe4

benaadams force-pushed the dict-debig-concurrency branch 2 times, most recently from 3b28f71 to 22fb6c0 Compare March 21, 2018 13:04

benaadams changed the title ~~Strong detection for Dictionary concurrent use in Debug~~ Strong detection for List/Dictionary concurrent use in Debug Mar 21, 2018

Strong detection for List concurrent use in Debug

956fdd2

benaadams force-pushed the dict-debig-concurrency branch from 22fb6c0 to 956fdd2 Compare March 21, 2018 13:34

fix

fe55440

benaadams mentioned this pull request Mar 21, 2018

Small tweaks to Dict asm size #17096

Merged

RussKeldorph added the area-System.Collections label Apr 13, 2018

jkotas closed this Jun 25, 2018

benaadams deleted the dict-debig-concurrency branch June 25, 2018 19:23

This was referenced Jan 31, 2020

Potential race condition in SqlConnectionString.cs dotnet/runtime#26606

Closed

Potential race conditions in DataContractSerialization dotnet/runtime#26607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strong detection for List/Dictionary concurrent use in Debug #17076

Strong detection for List/Dictionary concurrent use in Debug #17076

benaadams commented Mar 20, 2018

benaadams commented Mar 20, 2018

sharwell Mar 20, 2018

danmoseley Mar 20, 2018

benaadams Mar 20, 2018

benaadams Mar 20, 2018

danmoseley Mar 20, 2018

benaadams Mar 20, 2018

benaadams Mar 21, 2018

benaadams commented Mar 21, 2018

jkotas commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018

danmoseley commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018

jkotas commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018 •

edited

Loading

danmoseley commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018

danmoseley commented Mar 21, 2018

benaadams commented Mar 21, 2018

stephentoub commented Mar 22, 2018

jkotas commented Jun 25, 2018

jkotas commented Jun 25, 2018

Strong detection for List/Dictionary concurrent use in Debug #17076

Strong detection for List/Dictionary concurrent use in Debug #17076

Conversation

benaadams commented Mar 20, 2018

benaadams commented Mar 20, 2018

sharwell Mar 20, 2018

Choose a reason for hiding this comment

danmoseley Mar 20, 2018

Choose a reason for hiding this comment

benaadams Mar 20, 2018

Choose a reason for hiding this comment

benaadams Mar 20, 2018

Choose a reason for hiding this comment

danmoseley Mar 20, 2018

Choose a reason for hiding this comment

benaadams Mar 20, 2018

Choose a reason for hiding this comment

benaadams Mar 21, 2018

Choose a reason for hiding this comment

benaadams commented Mar 21, 2018

jkotas commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018

danmoseley commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018

jkotas commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018 • edited Loading

danmoseley commented Mar 21, 2018

benaadams commented Mar 21, 2018

benaadams commented Mar 21, 2018

danmoseley commented Mar 21, 2018

benaadams commented Mar 21, 2018

stephentoub commented Mar 22, 2018

jkotas commented Jun 25, 2018

jkotas commented Jun 25, 2018

benaadams commented Mar 21, 2018 •

edited

Loading