Fix race condition on propertyUsageMap via AtomicReference #699
Conversation
| return ret; | ||
| } | ||
| Map<String, PropertyUsageData> map = propertyUsageMapRef.getAndSet(new ConcurrentHashMap<>()); | ||
| return Collections.unmodifiableMap(new HashMap<>(map)); |
There was a problem hiding this comment.
No need to double wrap here. If any other thread had a reference to the map they'll release fairly soon anyway. Making a copy here will lose the updates those threads were recording. If you simply wrap it in the unmodifiableMap then there's a little bit more time between here and the actual flush to give them a chance to finish.
There was a problem hiding this comment.
Ah that makes sense, updated to this
| for (Map.Entry<String, PropertyUsageData> entry : accessMonitorUtil.propertyUsageMap.entrySet()) { | ||
| propertyUsageMap.putIfAbsent(entry.getKey(), entry.getValue()); | ||
| for (Map.Entry<String, PropertyUsageData> entry : accessMonitorUtil.propertyUsageMapRef.get().entrySet()) { | ||
| propertyUsageMapRef.get().putIfAbsent(entry.getKey(), entry.getValue()); |
There was a problem hiding this comment.
Grab a reference to the destination map before entering the loop and merge the entries into that. Otherwise, if a call to getAndClear comes in while the loop is running then half of the other accessMonitorUtil's entries will be merged into the old map and half into the new, which will be confusing at best :-P
There was a problem hiding this comment.
Oops yea good point. I think in practice this should never happen, but good to safeguard against it
Address the race condition on propertyUsageMap when we are trying to flush the existing map and then empty it. We use the AtomicReference here to hot swap the existing map out for a new empty one before we then copy the old map to be flushed.
Also tacked on a small logging change here, which flips the logging to only happen when it starts up the flushing and doesn't print at all when nothing is flushing. SBN by default will setup the AccessMonitorUtil twice, so we just add confusing noise for no reason with the prior logs.