Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConcurrentDictionary from referencesources #139

Closed
wants to merge 2 commits into from

Conversation

Terricide
Copy link

No description provided.

@NN---
Copy link
Collaborator

NN--- commented Jan 9, 2021

@Terricide
Copy link
Author

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---
Copy link
Collaborator

NN--- commented Jan 9, 2021

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

@Terricide
Copy link
Author

Terricide commented Jan 9, 2021

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---
Copy link
Collaborator

NN--- commented Jan 9, 2021

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
Copy link
Author

Terricide commented Jan 9, 2021

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
Copy link
Author

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

@NN---
Copy link
Collaborator

NN--- commented Jan 9, 2021

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

Today even Windows 7 is not so common in enterprise.

@Terricide
Copy link
Author

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

@NN---
Copy link
Collaborator

NN--- commented Jan 9, 2021

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

@theraot
Copy link
Owner

theraot commented Jan 9, 2021

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

What were those errors?

@Terricide
Copy link
Author

I think dotnet 3.5 was still optional.

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

@Terricide
Copy link
Author

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

@NN---
Copy link
Collaborator

NN--- commented Jan 9, 2021

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
Copy link
Author

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

@Terricide
Copy link
Author

{"@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 added a commit that referenced this pull request Jan 10, 2021
Also added .NET 2.0 back to test targets
theraot added a commit that referenced this pull request Jan 10, 2021
@@ -27,7 +27,7 @@
<AssemblyVersion>3.2.1.0</AssemblyVersion>
<FileVersion>3.2.1.0</FileVersion>
<PackageLicenseExpression>MIT</PackageLicenseExpression>
<LangVersion>8.0</LangVersion>
<LangVersion>9.0</LangVersion>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a separate PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theraot
Copy link
Owner

theraot commented Jan 11, 2021

I hereby claim that the fix in 605ed53 works.

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

@theraot
Copy link
Owner

theraot commented Jan 11, 2021

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 344e1b0 - 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---
Copy link
Collaborator

NN--- commented Jan 25, 2021

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

NN--- added a commit that referenced this pull request Jan 25, 2021
@theraot
Copy link
Owner

theraot commented Jan 25, 2021

I had already pushed a fix and a test for this: #139 (comment)

@theraot
Copy link
Owner

theraot commented Jan 25, 2021

The fix is on Nuget 3.2.2

@theraot theraot closed this Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants