Avoid race when deduplicating frozen strings. Fixes #4970.

The logic here goes like this: * Attempt to lookup the dedup'ed string. * If found, return it. * Try to insert new dedup'ed string. * If nobody beat us to it, return it. * If someone beat us to it and the reference is still good, return it. * If someone beat us to it and the reference is vacated, atomically replace it or retrieve any updates that beat us. * Keep trying until we succeed in updating or get back a populated reference. Previously, there could be a race if two threads try to dedup the same string at the same time. Only one would get in the cache, but the n+1 threads might not return the already-cached value.
jruby · Jan 22, 2018 · 239ef50 · 239ef50
1 parent cce752c
commit 239ef50
Showing 1 changed file with 21 additions and 1 deletion.
diff --git a/core/src/main/java/org/jruby/Ruby.java b/core/src/main/java/org/jruby/Ruby.java
@@ -4679,11 +4679,31 @@ public RubyString freezeAndDedupString(RubyString string) {
         RubyString deduped;
 
         if (dedupedRef == null || (deduped = dedupedRef.get()) == null) {
+            // Never use incoming value as key
             deduped = string.strDup(this);
             deduped.setFrozen(true);
-            dedupMap.putIfAbsent(string, new WeakReference<RubyString>(deduped));
+
+            final WeakReference<RubyString> weakref = new WeakReference<>(deduped);
+
+            // try to insert new
+            dedupedRef = dedupMap.computeIfAbsent(deduped, key -> weakref);
+            if (dedupedRef == null) return deduped;
+
+            // entry exists, return result if not vacated
+            RubyString unduped = dedupedRef.get();
+            if (unduped != null) return unduped;
+
+            // ref is there but vacated, try to replace it until we have a result
+            while (true) {
+                dedupedRef = dedupMap.computeIfPresent(string, (key, old) -> old.get() == null ? weakref : old);
+
+                // return result if not vacated
+                unduped = dedupedRef.get();
+                if (unduped != null) return unduped;
+            }
         } else if (deduped.getEncoding() != string.getEncoding()) {
             // if encodings don't match, new string loses; can't dedup
+            // FIXME: This may never happen, if we are properly considering encoding in RubyString.hashCode
             deduped = string.strDup(this);
             deduped.setFrozen(true);
         }