From 07d2571e0b4a2e1b3f4ff5213545b2524438cbc8 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Mon, 11 Dec 2023 11:57:58 +0100 Subject: [PATCH] Avoid race conditions while restructuring ConcurrentReferenceHashMap Prior to this commit, the `ConcurrentReferenceHashMap#restructure` operation would null out the entire references array before starting the restructuring operation (in case resizing is not necessary). This could cause at runtime race conditions where a lookup operation would return null, when the value is actually cached but not accesible during the restructuring phase. This commit ensures that, when resizing is not required, a new reference list is built (purged of null entries) and then assigned to the reference array. This way, concurrent reads will not return null for existing entries and there are less chances of re-calculating cache entries during the restructuring phase. Closes gh-31008 --- .../util/ConcurrentReferenceHashMap.java | 68 +++++++++++-------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java b/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java index 4e773adc011..ad0abc6844f 100644 --- a/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java +++ b/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java @@ -603,38 +603,52 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen resizing = true; } - // Either create a new table or reuse the existing one - Reference[] restructured = - (resizing ? createReferenceArray(restructureSize) : this.references); - - // Restructure int newCount = 0; - for (int i = 0; i < this.references.length; i++) { - ref = this.references[i]; - if (!resizing) { - restructured[i] = null; - } - while (ref != null) { - if (!toPurge.contains(ref)) { - Entry entry = ref.get(); - // Also filter out null references that are now null - // they should be polled the queue in a later restructure call. - if (entry != null) { - int index = getIndex(ref.getHash(), restructured); - restructured[index] = this.referenceManager.createReference( - entry, ref.getHash(), restructured[index]); - newCount++; - } - } - ref = ref.getNext(); - } - } - - // Replace volatile members + // Restructure the resized reference array if (resizing) { + Reference[] restructured = createReferenceArray(restructureSize); + for (int i = 0; i < this.references.length; i++) { + ref = this.references[i]; + while (ref != null) { + if (!toPurge.contains(ref)) { + Entry entry = ref.get(); + // Also filter out null references that are now null + // they should be polled from the queue in a later restructure call. + if (entry != null) { + int index = getIndex(ref.getHash(), restructured); + restructured[index] = this.referenceManager.createReference( + entry, ref.getHash(), restructured[index]); + newCount++; + } + } + ref = ref.getNext(); + } + } + // Replace volatile members this.references = restructured; this.resizeThreshold = (int) (this.references.length * getLoadFactor()); } + // Restructure the existing reference array "in place" + else { + for (int i = 0; i < this.references.length; i++) { + Reference purgedRef = null; + ref = this.references[i]; + while (ref != null) { + if (!toPurge.contains(ref)) { + Entry entry = ref.get(); + // Also filter out null references that are now null + // they should be polled from the queue in a later restructure call. + if (entry != null) { + purgedRef = this.referenceManager.createReference( + entry, ref.getHash(), purgedRef); + } + newCount++; + } + ref = ref.getNext(); + } + this.references[i] = purgedRef; + } + } this.count.set(Math.max(newCount, 0)); } finally {