Synchronize access to ContextTypeRegistry.fContextTypes

[trancexpress]

This change adjusts ContextTypeRegistry to synchronize access on the underlying map.

NOTE: Since contextTypes() returns an iterator, direct synchronization is not possible. With this change the iterator iterates over a copy of the underlying map.

I.e. removal of elements in the map is no longer possible via the iterator.

Fixes: #4020

[trancexpress]

If we are merging this, does it need a N&N entry? We are potentially breaking client code.

[merks]

Have you considered a concurrent map? (I'm a little swamped so this is knee-jerk question.)

[merks]

We should not be doing this during the RC2 week.

[trancexpress]

Have you considered a concurrent map? (I'm a little swamped so this is knee-jerk question.)

Since we have to copy the map, we'd still have to use the map for a synchronized block in the iterator method. I'm not sure we gain much, but I guess it will reduce a few lines from the diff. Should I?

[merks]

It's RC2 freeze week and @iloveeclipse mentioned M1 so I am explicitly blocking this for now.

Have you considered using java.util.concurrent.ConcurrentHashMap? The iterator can make a copy of whatever it's iterating over. But isn't this all easier if the map itself is thread safe?

[trancexpress]

Have you considered using java.util.concurrent.ConcurrentHashMap? The iterator can make a copy of whatever it's iterating over. But isn't this all easier if the map itself is thread safe?

We have a LinkedHashMap as a base, I don't think ConcurrentHashMap guarantees an order?

[github-actions]

Test Results

   864 files  ±0     864 suites  ±0   52m 12s ⏱️ - 2m 33s
 7 988 tests ±0   7 745 ✅ ±0  243 💤 ±0  0 ❌ ±0 
20 418 runs  ±0  19 763 ✅ ±0  655 💤 ±0  0 ❌ ±0 

Results for commit 8cd93af. ± Comparison against base commit a086677.

[merks]

Does anything in the API suggest there is an order specified? Is order meaningful semantically? Mostly it’s a wrapper around get and put I think. 🤔

[iloveeclipse]

Does anything in the API suggest there is an order specified? Is order meaningful semantically? Mostly it’s a wrapper around get and put I think. 🤔

Implementation uses LinkedHashMapy so order is at least desirable.

[merks]

The implementation performs quickly without synchronization. 🤪 Now you want to synchronize everything rather than use a concurrent map, you want to change the semantics of the iterator to be a copy (and reject delete?) but you want to preserve an order that was never specified and has effectively no meaning with respect to multithreaded registrations. Seems strange to me. But that’s just my opinion and I don’t really care. Someone else might.

[trancexpress]

Now you want to synchronize everything rather than use a concurrent map

What is the difference in your view? Between concurrent and synchronized?

From my POV the concurrent case will likely perform better, but I'm not sure we are on a performance critical path in this code.

[merks]

I just thought that a concurrent map had optimal performance for get, such that multiple threads doing a get did not block each other. I’m no doubt splitting hairs, but preserving unspecified meaningless order because the current internals do that, albeit not in a thread safe way, is probably splitting a different hair. I don’t really care so much and I’m pretty sure none of this is performance critical.