Avoid race when deduplicating frozen strings. Fixes #4970.

headius · headius · commit 239ef50bc14c · 2018-01-22T14:39:13.000-06:00
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.
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);
         }
