Use generic collections in System.ComponentModel.TypeConverter implementation

[MattKotsenas]

Fix #17217 by replacing non-generic collections classes with their generic counterparts. Of specific interest are:

• The EditorTable related collections are public, and thus can't be converted without changing the public API
• In a couple of places we have public APIs that return the Hashtable's enumerator, so we need to leave those as-is so the enumerated items are DictionaryEntry and not KeyValuePair
• In places where the Hashtables were static, I verified that every use was either already under a lock, or switched to a ConcurrentDictionary to more closely match the Hashtable semantics of allowing multiple readers and a single writer

[ghost]

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fix #17217 by replacing non-generic collections classes with their generic counterparts. Of specific interest are:

• The EditorTable related collections are public, and thus can't be converted without changing the public API
• Removal of the WeakHashtable in favor of the public ConditionalWeakTable
• A change in binary formatting for the LicenseManager

Author:	MattKotsenas
Assignees:	-
Labels:	area-System.ComponentModel
Milestone:	-

[stephentoub]

Thanks for working on this. I suggest scaling back the PR to just the replacements that are trivial to prove are correct. There are a myriad of functional and performance differences between what's being replaced and the replacements, and I'm concerned we'll find a number of regressions that slip through as a result.

[MattKotsenas]

Thanks everyone for the feedback! I've pushed a less ambitious version that converts just two cases:

• Private instance members that don't (at least appear to) have any multi-threading
• Private statics where based on usage a ConcurrentDictionary looks appropriate

Additional feedback and thoughts are very welcome. My goal is less change for change's sake, and more to address the spirit of the linked issue (and hopefully close it out). If anyone has any thoughts, questions, or concerns, please let me know. Thanks!

[teo-tsirpanis]

Static dictionaries that contain Type objects must be ConditionalWeakTables, not (Concurrent)Dictionaryies. Otherwise the types are rooted which inhibits unloadability and leads to bugs like #62050.

I have marked some places that should be changed, could you please do that? You can take advantage of the GetValue method; it works similarly to ConcurrentDictionary.GetOrAdd.

s_defaultAttributes is safe to change to CWT as well.

Edit: Oh, you had responded to a comment of mine. I thought you hadn't seen them.

[stephentoub]

Static dictionaries that contain Type objects must be ConditionalWeakTables, not (Concurrent)Dictionaryies. Otherwise the types are rooted which inhibits unloadability and leads to bugs like #62050.

As long as this PR isn't adding additional rooting, I would prefer any changes to address concerns around unloadability be done separately in a dedicated PR.

[eerhardt]

Thanks for the PR @MattKotsenas.

Most of the files look fine. But 2 of them I'm not 100% sure are better. The 2 I tagged already have locking around modification, which we aren't changing the locking in this PR. I'm not sure we can trivially prove that changing from Hashtable to ConcurrentDictionary is beneficial. Can we revert those two files? We aren't actively investing in System.ComponentModel, and changes here would introduce more risk than the value these changes bring.

Other than that I think the rest of the changes are OK.

[eerhardt]

I'm not sure I can trivially prove the changes in this file are correct. Can we revert this file's changes?

[eerhardt]

I'm not sure I can trivially prove the changes in this file are correct. Can we revert this file's changes?

[eerhardt]

@MattKotsenas - any response to my above feedback? If you aren't actively working on this PR, I can close it until it is active again.

[MattKotsenas]

Hi @eerhardt ! Yes, I'll back out those changes as well and update to resolve conflicts today or tomorrow. Thanks again for all the feedback!

[MattKotsenas]

@eerhardt updated! Please take a look when you get a chance and let me know if you have any thoughts / suggestions. Thanks!

[eerhardt]

I think this slimmed down change is now fine. And with it we should be able to close the long-standing issue.

Thanks for the contribution, @MattKotsenas!

@stephentoub - I will wait for your approval before removing the "No Merge" label. Let me know what you think.

[stephentoub]

Thanks. Seems ok. All of those GetValueOrDefault calls are extension method calls resulting in virtual dispatch via IReadOnlyDictionary; it's probably ok given the library.

[eerhardt]

Test failure is #64389. Merging.