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

Add additional methods to ConditionalWeakTable #19632

Closed
stephentoub opened this issue Dec 10, 2016 · 5 comments
Closed

Add additional methods to ConditionalWeakTable #19632

stephentoub opened this issue Dec 10, 2016 · 5 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@stephentoub
Copy link
Member

ConditionalWeakTable<T> is a specialized dictionary but it lacks several useful public members. We just added AddOrUpdate in https://github.com/dotnet/corefx/issues/8429. We should also add:

public sealed class ConditionalWeakTable<TKey,TValue> : IEnumerable<KeyValuePair<TKey,TValue>>
{
    public void Clear(); // already exists as internal, just needs to be made public
    IEnumerator<KeyValuePair<TKey,TValue>> IEnumerable<KeyValuePair<TKey,TValue>>.GetEnumerator(); // non-snapshot semantics, similar to ConcurrentDictionary
    IEnumerator IEnumerable.GetEnumerator();
}
@justinvp
Copy link
Contributor

justinvp commented Dec 10, 2016

I don't have an opinion either way, but I seem to recall a deliberate choice to not implement any of the collection/dictionary interfaces and not treat ConditionalWeakTable as a "collection" when it was originally designed. I can't remember the specific details on why... Maybe because keys aren't persisted and keys/values are automatically removed.

We could expose GetEnumerator() and a struct enumerator without implementing any collection interfaces, which would enable foreach for code that needs it, without turning it into a general-purpose collection.

@stephentoub
Copy link
Member Author

I've been thinking about how to best implement the enumerator, and all of the safe implementations I've thought of necessitate having a finalizer in case Dispose isn't called. As such, for now at least I'm removing the struct enumerator from the proposal. We can explicitly implement IEnumerable<KeyValuePair<TKey,TValue>>, such that if in the future we find a better way to implement this entirely allocation-free, we can do so without it being breaking.

@terrajobst
Copy link
Member

What's the scenario for iterating over it? There are some concerns that making ConditionalWeakTable<K,V> a general purpose collection is pushing its design in a direction we don't believe makes sense.

@stephentoub
Copy link
Member Author

stephentoub commented Dec 15, 2016

@terrajobst, why don't you believe it makes sense?

Examples:

  • Debugging, and wanting to show everything in the table, as TaskScheduler needs to elsewhere in mscorlib
  • Timing out entries, iterating through the table to find entries older than some time and forcibly removing entries from the cache;
  • It already has an internal Keys and Values property because iteration was needed elsewhere in mscorlib, so examples similar to those
  • Finding entries by value rather than by key, so scenarios similar to dictionary's ContainsValue
  • Using a different equality operation on keys, as WinRT event code does elsewhere in mscorlib
  • ...

@terrajobst
Copy link
Member

We concluded that @stephentoub will take a look whether we can actually implement these APIs without holding references for too long. If we can, the API seem fine as proposed.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

No branches or pull requests

4 participants