theraot / Theraot

Backporting .NET and more: LINQ expressions in .net 2.0 - nuget Theraot.Core available.
MIT License
158 stars 30 forks source link

ConcurrentDictionary from referencesources #139

Closed Terricide closed 3 years ago

NN--- commented 3 years ago

Reference source is deprecated The latest versions is in dotnet runtime

https://github.com/dotnet/runtime/tree/master/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs

Terricide commented 3 years ago

I can try and get that version merged. I was testing the library today and got errors when casting ToArray().

I had used this ConcurrentDictionary in a .net 2.0 project without any concurrency issues.

NN--- commented 3 years ago

It means that there is a missing test in Theraot library. Can you add a test for ToArray that your PR fixes?

Terricide commented 3 years ago

I can try, I have been maintaining https://github.com/Terricide/MonoLib this for several years. Today I tried to swap it out with this library and saw the issue deep within my code

The version of MonoLib on github is old and I wanted to get all the additional dotnet backport stuff that I've done into this library

I saw the item count inside the Dictionary go to -2 as well

NN--- commented 3 years ago

This sounds definitely as a bug. But it really needs a test to avoid such problem kn the future:)

Just curious where do you need .NET 2.0?

Terricide commented 3 years ago

I just pushed my latest changes to that Mono repo

But essentially I work on a systems management product and part of it is discovering computers on the network to manage and I want it to support the largest range of operating systems as possible and not require customers to have to install newer versions of .net on hundreds of machines. At some point I might make dotnet 4.0 the minimum or just switch to .net core, But even .net core doesn's support earlier versions of windows 10.

Once we discover the devices we can patch it up as if the newer .net runtime if it is there.

Terricide commented 3 years ago

I also think it is cool that it should still run on windows 2000 if I ever needed it to :)

NN--- commented 3 years ago

It is cool but does it worth the effort ?:)

Today even Windows 7 is not so common in enterprise.

Terricide commented 3 years ago

We have customers that purchased extended support from Microsoft and they are using our tool to patch windows 7 still.

NN--- commented 3 years ago

Windows 7 means .NET 3.5 at least. It is much nicer than .NET 2.0.

theraot commented 3 years ago

I was testing the library today and got errors when casting ToArray().

What were those errors?

Terricide commented 3 years ago

I think dotnet 3.5 was still optional.

https://i.stack.imgur.com/vKC2u.png

Terricide commented 3 years ago

The error I was getting was The array can not contain the number of elements

NN--- commented 3 years ago

Probably it is included by default but can be uninstalled. I would assume .NET 3.5 is installed on all Windows 7 machines.

https://docs.microsoft.com/en-us/dotnet/framework/install/on-windows-7#net-framework-35

Terricide commented 3 years ago

I wasnt sure what to do with Nullable parameter types it wouldn't compile for netstandard 1.0

Terricide commented 3 years ago

{"@t":"2021-01-08T23:13:35.9955242Z","@mt":"TimeoutTasks:System.ArgumentException: The array can not contain the number of elements.\r\nParameter name: array\r\n at Theraot.Collections.ThreadSafe.Bucket1.CopyTo(T[] array, Int32 arrayIndex)\r\n at System.Collections.Generic.List1.InsertRange(Int32 index, IEnumerable1 collection)\r\n at System.Collections.Concurrent.ConcurrentDictionary2.ToArray()\r\n at Verismic.PeerFile.WebRtcPeer.d__71.MoveNext()","@l":"Error"}

theraot commented 3 years ago

I hereby claim that the fix in 605ed531ab38cd2def78290ed444a433d919772a works.

Problem being that Appveyor tests against .NET less than 4.0 are not working. That problem emerged with past sdk update.

theraot commented 3 years ago

To explain how and why the problem happens:

ToArray (and CopyTo) are the only two not atomic operations on the type. ToArray works by making an array of the appropriate size and then copying the values over. However, the number of items might increase after the array was initialized but before the copy completes. In which case the code would try to copy more elements that those that fit in the array, which is an exception.

Edit: see the test introduced in 344e1b0c5a079d9471928743b32b7ac07be58c20 - it starts a few concurrent write tasks, then while those are running, it requests ToArray, only after ToArray completes the concurrent tasks are allowed to stop. This test failed with "The array can not contain the number of elements" in .NET 2.0. And didn't after the fix.

To explain how and why the fix works:

ToArray (and CopyTo) will require an exclusive lock on the dictionary. Also other write operations would also need some sort of lock to not happen concurrently with ToArray (and CopyTo). However, those other write operations have no problem happen concurrently. That means we need to kinds of locks: exclusive and not exclusive. That is what a read write lock does. So write operations take a read lock, and ToArray (and CopyTo) take a write lock. The read operations do not need a lock at all.

NN--- commented 3 years ago

I have succeeded porting ConcurrentDictionary from dotnet/runtime repository. There is only one test failing.

theraot commented 3 years ago

I had already pushed a fix and a test for this: https://github.com/theraot/Theraot/pull/139#issuecomment-757328825

theraot commented 3 years ago

The fix is on Nuget 3.2.2