Break hangs on HashSet when a loop is formed on entries due to a concurrent operation #28225

safern · 2018-03-19T21:26:30Z

Fixes: https://github.com/dotnet/corefx/issues/28209

cc: @danmosemsft @vancem @jkotas @stephentoub

…urrent operation

stephentoub · 2018-03-19T21:39:57Z

src/System.Collections/src/System/Collections/Generic/HashSet.cs

+
+                    if (collisionCount >= _slots.Length)
+                    {
+                        // The chain of entries forms a loop; which means a concurrent update has happened.


Nit: ";" => ","

stephentoub · 2018-03-19T21:40:10Z

src/System.Collections/src/System/Collections/Generic/HashSet.cs

+
+                    if (collisionCount >= _slots.Length)
+                    {
+                        // The chain of entries forms a loop; which means a concurrent update has happened.


Ditto (for all such cases)

danmoseley · 2018-03-19T21:40:42Z

src/System.Collections/src/System/Collections/Generic/HashSet.cs

+                    if (collisionCount >= _slots.Length)
+                    {
+                        // The chain of entries forms a loop; which means a concurrent update has happened.
+                        throw new InvalidOperationException(SR.InvalidOperation_ConcurrentOperationsNotSupported);


We should probably use a ThrowHelper scheme instead of throw for these as some of these loops are particularly hot paths. There is not a ThrowHelper.cs in CoreFX common (probably should be). You can copy one of the several sprinkled around CoreFX sources.

There is not a ThrowHelper.cs in CoreFX common (probably should be)

#26636 (comment)

danmoseley · 2018-03-19T22:42:25Z

@Anipik Alpine build failed:

  Traceback (most recent call last):
    File "/mnt/j/workspace/dotnet_corefx/master/alpine-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/DumplingHelper.py", line 131, in <module>
      main(sys.argv)
    File "/mnt/j/workspace/dotnet_corefx/master/alpine-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/DumplingHelper.py", line 113, in main
      install_dumpling()
    File "/mnt/j/workspace/dotnet_corefx/master/alpine-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/DumplingHelper.py", line 34, in install_dumpling
      print("An unexpected error was encountered while installing dumpling.py: " + sys.exc_info()[0])
  TypeError: cannot concatenate 'str' and 'type' objects
/mnt/j/workspace/dotnet_corefx/master/alpine-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/Dumpling.targets(30,5): error MSB3073: The command "python /mnt/j/workspace/dotnet_corefx/master/alpine-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/Tools/DumplingHelper.py install_dumpling" exited with code 1. [/mnt/j/workspace/dotnet_corefx/master/alpine-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/tests.builds]

In a quick look, I think sys.exec_info()[0] is not stringable, or at least not always. A better way seems to be to add import traceback and do

print("An unexpected error was encountered while installing dumpling.py: " + traceback.format_exc())

Could you please fix in dumplinghelper.py.

jkotas · 2018-03-19T23:29:46Z

src/System.Collections/src/System/Collections/ThrowHelper.cs

+{
+    internal static class ThrowHelper
+    {
+        internal static void ThrowInvalidOperationException_ConcurrentOperationsNotSupported() => throw CreateInvalidOperationException_ConcurrentOperationsNotSupported();


This can be just:

internal static void ThrowInvalidOperationException_ConcurrentOperationsNotSupported() => throw new InvalidOperationException(SR.InvalidOperation_ConcurrentOperationsNotSupported);

The complex throw helper pattern with intermediate CreateInvalidOperationException is a necessary only for cases where the code needs to be compatible with old JITs that do not perform the right optimizations. It is not the case for System.Collections.

I see. Thought it was for all cases. Thanks, will update the PR.

This drop in the bucket since the rest of the Hashset is not using the ThrowHelper. Using it in one place only helps very little.

These manual throw helpers are pain to deal with for Mono folks who are trying to reuse the code. cc @marek-safar

Unless this has a measurable performance impact for something that matters, we should avoid introducing these manual throw helpers.

If we care about the performance benefits, we should build the plugin for IL linker that autogenerates them everywhere.

If we care about the performance benefits, we should build the plugin for IL linker that autogenerates them everywhere.

👍 I'd be very happy with that :)

jkotas · 2018-03-19T23:34:11Z

src/System.Collections/src/System/Collections/ThrowHelper.cs

+{
+    internal static class ThrowHelper
+    {
+        internal static void ThrowInvalidOperationException_ConcurrentOperationsNotSupported() => throw CreateInvalidOperationException_ConcurrentOperationsNotSupported();


This drop in the bucket since the rest of the Hashset is not using the ThrowHelper. Using it in one place only helps very little.

These manual throw helpers are pain to deal with for Mono folks who are trying to reuse the code. cc @marek-safar

Unless this has a measurable performance impact for something that matters, we should avoid introducing these manual throw helpers.

If we care about the performance benefits, we should build the plugin for IL linker that autogenerates them everywhere.

safern · 2018-03-20T00:02:03Z

This drop in the bucket since the rest of the Hashset is not using the ThrowHelper

So you would suggest updating HashSet to use ThrowHelper in all other places where is throwing, or measure if it is worth it, or not using ThrowHelper at all?

jkotas · 2018-03-20T00:23:27Z

I would suggest not using the manual ThrowHelper at all.

benaadams · 2018-03-20T01:28:38Z

src/System.Collections/src/System/Collections/Generic/HashSet.cs

@@ -265,6 +266,13 @@ public bool Contains(T item)
                    {
                        return true;
                    }
+
+                    if (collisionCount >= _slots.Length)


Take _slots to a local ref for its multiple uses?
Slot[] slots = _slots;

just curious and to learn, what is the benefit of doing that?

It uses the array in a register rather than a memory location; also applies extra optimisations because it cannot change between uses unlike the memory location version

Makes sense. Thanks for explaining :)

@benaadams is this something the JIT might be able to do itself in future? Always a shame to write code in an artificial way.

Possibly, though bounds checks have already regressed to become even more conservative when the array comes from memory dotnet/coreclr#15756

benaadams · 2018-03-20T01:29:47Z

src/System.Collections/src/System/Collections/Generic/HashSet.cs

@@ -293,6 +301,7 @@ public bool Remove(T item)
                int hashCode = InternalGetHashCode(item);
                int bucket = hashCode % _buckets.Length;
                int last = -1;
+                int collisionCount = 0;
                for (int i = _buckets[bucket] - 1; i >= 0; last = i, i = _slots[i].next)


Take _slots to a local ref for its multiple uses?
Slot[] slots = _slots;

safern · 2018-03-20T16:53:22Z

I would suggest not using the manual ThrowHelper at all.

Ok. Will update to throw from the class itself. Adding a private method that throws the exception would help instead of throwing it inside every instance?

jkotas · 2018-03-20T17:16:37Z

This file does not use the throw helpers at all. I would maintain the consistent style. Throw inplace without any helpers methods like vast majority of the code out there.

safern · 2018-03-21T17:34:31Z

@jkotas looks good now?

safern · 2018-03-21T17:34:45Z

@dotnet-bot test this please

jkotas

Thanks

benaadams · 2018-03-21T18:27:08Z

src/System.Collections/src/System/Collections/Generic/HashSet.cs

            }
            else
            {
-                if (_lastIndex == _slots.Length)
+                if (_lastIndex == slots.Length)
                {
                    IncreaseCapacity();


Need to grab slots again here as IncreaseCapacity() will change it. So

IncreaseCapacity(); slots = _slots;

benaadams · 2018-03-21T18:27:33Z

src/System.Collections/src/System/Collections/Generic/HashSet.cs

            }
            else
            {
-                if (_lastIndex == _slots.Length)
+                if (_lastIndex == slots.Length)
                {
                    IncreaseCapacity();


Need to grab slots again here as IncreaseCapacity() will change it. So

IncreaseCapacity(); slots = _slots;

Ah, you're right, thanks for the heads up.

safern · 2018-03-22T02:50:55Z

test this please

safern · 2018-03-22T07:16:58Z

test Windows x64 Debug Build
test OSX x64 Debug Build

safern · 2018-03-22T16:17:44Z

test Linux x64 Debug Build
test Linux x64 Release Build

safern · 2018-03-23T16:56:18Z

test Linux x64 Debug Build
test Windows x86 Release Build

safern · 2018-03-23T16:58:15Z

@mmitche seems like there is a bug in the regex job trigger. I triggered 2 specific jobs and it restarted 3 as you can see above. This happened for the last 2 times.

safern · 2018-03-26T15:36:12Z

test Linux x64 Debug Build please

safern · 2018-03-26T19:35:09Z

@danmosemsft the failures here doesn’t look related, good to merge? the Linux builds seem in a pretty flaky state.

benaadams · 2018-03-26T20:14:41Z

Not sure how to get the 1 failure

Fedora.26.Amd64.Open - x64 - Debug
[Pipeline] [Fedora.26.Amd64.Open - x64 - Debug (7dc02b32-3fe0-4f33-a236-8d33812aa46b)] httpRequest
HttpMethod: GET
URL: https://helix.dot.net/api/2017-04-14/aggregate/jobs?groupBy=job.name&maxResultSets=1&filter.name=7dc02b32-3fe0-4f33-a236-8d33812aa46b
Sending request to url: https://helix.dot.net/api/2017-04-14/aggregate/jobs?groupBy=job.name&maxResultSets=1&filter.name=7dc02b32-3fe0-4f33-a236-8d33812aa46b
Response Code: HTTP/1.1 200 OK
Response: 
[{"Key":{"job.name":"7dc02b32-3fe0-4f33-a236-8d33812aa46b"},"Data":{"Analysis":[{"Name":"xunit","Status":{"pass":301186,"fail":1,"skip":384}}],"WorkItemStatus":{"pass":232}}}]
Success code from [100‥399]
] echo
Info: Failed 1/301571 (384 skipped)

safern · 2018-03-26T20:22:20Z

https://mc.dot.net/#/user/safern/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/6c9a8e69e579acf323b57c5506be8e82913234c2

danmoseley · 2018-03-26T20:25:54Z

Please open an issue for the sockets failure you hit here.

benaadams · 2018-03-26T20:26:13Z

Fedora.26.Amd64.Open-x64-Debug

System.Net.Sockets.Tests
 System.Net.Sockets.Tests.ExecutionContextFlowTest/
  ExecutionContext_FlowsOnlyOnceAcrossAsyncOperations

Assert.InRange() Failure
Range:  (1 - 60)
Actual: 117

  at System.Net.Sockets.Tests.ExecutionContextFlowTest.<>c.<<ExecutionContext_FlowsOnlyOnceAcrossAsyncOperations>b__11_0>d.MoveNext() in /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Sockets/tests/FunctionalTests/ExecutionContextFlowTest.netcoreapp.cs:line 47
--- End of stack trace from previous location where exception was thrown ---
--- End of stack trace from previous location where exception was thrown ---

benaadams · 2018-03-26T20:57:04Z

Raised issue https://github.com/dotnet/corefx/issues/28482

safern · 2018-03-26T21:07:20Z

Thanks @benaadams

…#28225

…urrent operation (dotnet/corefx#28225) * Break hangs on HashSet when a loop is formed on entries due to a concurrent operation * PR Feedback, copy _slots to local ref * Get a copy of -slots after IncreaseCapacity changes it Commit migrated from dotnet/corefx@be48e96

Break hangs on HashSet when a loop is formed on entries due to a conc…

a7d2cb2

…urrent operation

safern added the area-System.Collections label Mar 19, 2018

safern self-assigned this Mar 19, 2018

stephentoub reviewed Mar 19, 2018

View reviewed changes

danmoseley reviewed Mar 19, 2018

View reviewed changes

danmoseley approved these changes Mar 19, 2018

View reviewed changes

danmoseley added the netfx-port-consider label Mar 19, 2018

jkotas reviewed Mar 19, 2018

View reviewed changes

Anipik mentioned this pull request Mar 19, 2018

Using traceback library to give call stack dotnet/buildtools#1969

Merged

jkotas suggested changes Mar 19, 2018

View reviewed changes

benaadams reviewed Mar 20, 2018

View reviewed changes

ahsonkhan mentioned this pull request Mar 20, 2018

API consistency for System.Memory APIs (argument names) #28160

Merged

danmoseley closed this in dotnet/buildtools#1969 Mar 20, 2018

jkotas reopened this Mar 20, 2018

PR Feedback, copy _slots to local ref

4325a73

safern force-pushed the HangsHashSet branch from 8eb58e7 to 4325a73 Compare March 20, 2018 18:06

Anipik mentioned this pull request Mar 20, 2018

using traceback to get call stack dotnet/buildtools#1972

Merged

jkotas approved these changes Mar 21, 2018

View reviewed changes

benaadams reviewed Mar 21, 2018

View reviewed changes

Get a copy of -slots after IncreaseCapacity changes it

6c9a8e6

benaadams approved these changes Mar 22, 2018

View reviewed changes

danmoseley merged commit be48e96 into dotnet:master Mar 26, 2018

safern deleted the HangsHashSet branch March 27, 2018 01:57

karelz added this to the 2.1.0 milestone Mar 27, 2018

mynkow added a commit to Elders/Cronus that referenced this pull request Mar 21, 2019

Fixes a concurrency issue when working with versions => dotnet/corefx…

38f9e29

…#28225

Break hangs on HashSet when a loop is formed on entries due to a concurrent operation #28225

Break hangs on HashSet when a loop is formed on entries due to a concurrent operation #28225

Conversation

safern commented Mar 19, 2018

Choose a reason for hiding this comment

stephentoub Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

safern commented Mar 20, 2018

jkotas commented Mar 20, 2018

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

Choose a reason for hiding this comment

benaadams Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

safern commented Mar 20, 2018

jkotas commented Mar 20, 2018

safern commented Mar 21, 2018

safern commented Mar 21, 2018

jkotas left a comment

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

safern commented Mar 22, 2018

safern commented Mar 22, 2018

safern commented Mar 22, 2018

safern commented Mar 23, 2018

safern commented Mar 23, 2018

safern commented Mar 26, 2018

safern commented Mar 26, 2018

benaadams commented Mar 26, 2018

safern commented Mar 26, 2018

danmoseley commented Mar 26, 2018

benaadams commented Mar 26, 2018

benaadams commented Mar 26, 2018

safern commented Mar 26, 2018

stephentoub Mar 19, 2018 •

edited

Loading

jkotas Mar 19, 2018 •

edited

Loading

jkotas Mar 19, 2018 •

edited

Loading

benaadams Mar 20, 2018 •

edited

Loading