Skip to content

ConcurrentDictionary.GetOrAdd enhancement #2003

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

Merged
merged 2 commits into from
Apr 27, 2017
Merged

ConcurrentDictionary.GetOrAdd enhancement #2003

merged 2 commits into from
Apr 27, 2017

Conversation

guardrex
Copy link
Contributor

Fixes #1940

Initial draft errs on the side of an explicit explanation favored by @epa OP at https://github.com/dotnet/corefx/issues/13308.

cc/ @stephentoub

@guardrex guardrex added Area - API Reference external Not related to content labels Apr 25, 2017
@guardrex guardrex self-assigned this Apr 25, 2017
Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

Thanks @guardrex. looks good! Left a few comments for you to address.

<returns>The value for the key. This will be either the existing value for the key if the key is already in the dictionary, or the new value for the key as returned by valueFactory if the key was not in the dictionary.</returns>
<param name="valueFactory">The function used to generate a value for the key.</param>
<summary>Adds a key/value pair to the <see cref="T:System.Collections.Concurrent.ConcurrentDictionary`2" /> by using the specified function if the key does not already exist, or returns the existing value if the key exists.</summary>
<returns>The value for the key. For more information, see the Remarks section.</returns>
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't usually need to say this in the return section. Just add the extra info in the remarks. (and it also doesn't help if you're reading this in your IntelliSense so think about what can be helpful to the customer if all they have is IntelliSense - summary, parameters, return value)

| -------- | ------------ |
| The key is already in the dictionary. | The existing value is returned. |
| The key is not in the dictionary. `valueFactory` generates a value. On rechecking for the key, no key is found. | The key/value is inserted into the dictionary, and the value is returned. |
| The key is not in the dictionary. `valueFactory` generates a value; but while `valueFactory` is generating the value, a different thread inserts a value for the key. After `valueFactory` executes and upon rechecking for the key, the key inserted by the other thread is found. | The value inserted by the other thread is returned. |
Copy link
Contributor

Choose a reason for hiding this comment

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

; but -> split the sentences?

| The key is not in the dictionary. `valueFactory` generates a value. On rechecking for the key, no key is found. | The key/value is inserted into the dictionary, and the value is returned. |
| The key is not in the dictionary. `valueFactory` generates a value; but while `valueFactory` is generating the value, a different thread inserts a value for the key. After `valueFactory` executes and upon rechecking for the key, the key inserted by the other thread is found. | The value inserted by the other thread is returned. |

Because a key/value can be inserted by another thread while `valueFactory` is generating a value, you cannot trust that just because `valueFactory` executed its produced value will be inserted into the dictionary and returned. If you call <xref:System.Collections.Concurrent.ConcurrentDictionary%602.GetOrAdd%2A> simultaneously on different threads, `valueFactory` may be called multiple times, but its key/value pair might not be added to the dictionary for every call.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing comma after executed

I feel this paragraph should be opening the remarks section.

Change the first Because to Since.

Copy link
Contributor

Choose a reason for hiding this comment

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

valueFactory may be called multiple times, but its key/value pair might not be added to the dictionary for every call

This makes it sound like it actually might be added several times. But that's not true, subsequent generated values are guaranteed to be thrown away.

I would phrase it like this:

valueFactory may be called multiple times, but only one key/value pair will be added to the dictionary

@guardrex
Copy link
Contributor Author

guardrex commented Apr 25, 2017

@mairaw @svick Ready for another look 👀

RE: "see remarks" in <returns>: It wasn't completely avoided (~10 cases), which is why I thought it would be ok. How about I put one in this afternoon for: Remove "See Remarks" from <returns>?

@svick
Copy link
Contributor

svick commented Apr 25, 2017

LGTM.

Copy link
Contributor

@mairaw mairaw left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @guardrex!

@mairaw mairaw merged commit 838c54b into dotnet:master Apr 27, 2017
@mairaw mairaw deleted the guardrex/concurrentdictionary-update branch April 27, 2017 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Not related to content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConcurrentDictionary.GetOrAdd documentation not quite correct
5 participants