[JENKINS-65821] Fix race condition by using ConcurrentHashMap

[bitwiseman]

JENKINS-65821 described a race condition resulting in high cpu usage. #153 attempted to address these using locking, but resulted in deadlocks.

This change goes the other direction and attempts to address the issue using ConcurrentHashMap instead of HashMap.

@twasyl

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[jglick]

I guess? I trust you have examined the code flow carefully and verified that there is no risk of one thread having written to just one of these maps when another thread comes along and tries to read from them. I suppose it cannot be worse than using HashMap. @dwnusbaum mentioned that the usages here were idempotent and thus safe to use locklessly.

[twasyl]

I was wondering if one possible fix would be, in each entry point of a StandardGraphLookupView instance, to create new maps from the instance ones and pass them along to all required methods. This way you only need to synchronize when creating those copies and then you ensure that even if the instance maps are updated in one thread, computations in other threads are based on the same data from their beginning to their end.

[bitwiseman]

@jglick Yes, in my opinion it has to be better than HashMap.

@twasyl
That seems like a lot of extra work being done.

[bitwiseman]

@jglick From what I can see, this is approach is safe. As you say, idempotent, the two caches do not depend on each other to update, and any read failure results in calling the "brute force" method the recalculate missing values.

[jglick]

This should be fine since the value type is just String that already exists. Just a reminder that in general you need to be careful about having one thread add an entry to a map (or whatever) that another thread might pull off without the use of any synchronization barriers: if the object were recently constructed, its constructor might not have completed by the time the other thread uses it. One of those weird theoretical problems that probably does not happen often in reality but is hard to reason about.

[bitwiseman]

if the object were recently constructed, its constructor might not have completed by the time the other thread uses it.

Wow, good to know. Thanks!