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 12, 2018 · 23783ce · 23783ce · kares · Jan 17, 2018
1 parent 56db7a9
commit 23783ce
Showing 1 changed file with 33 additions and 1 deletion.
diff --git a/core/src/main/java/org/jruby/Ruby.java b/core/src/main/java/org/jruby/Ruby.java
@@ -198,6 +198,8 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.BiFunction;
+import java.util.function.Function;
 import java.util.regex.Pattern;
 
 import static java.lang.invoke.MethodHandles.explicitCastArguments;
@@ -4580,11 +4582,41 @@ public RubyString freezeAndDedupString(RubyString string) {
         RubyString deduped;
 
         if (dedupedRef == null || (deduped = dedupedRef.get()) == null) {
+            // Never use value as key
             deduped = string.strDup(this);
             deduped.setFrozen(true);
-            dedupMap.put(string, new WeakReference<RubyString>(deduped));
+
+            final WeakReference<RubyString> weakref = new WeakReference<>(deduped);
+
+            // try to insert new
+            dedupedRef = dedupMap.computeIfAbsent(string, new Function<RubyString, WeakReference<RubyString>>() {
+                @Override
+                public WeakReference<RubyString> apply(RubyString key) {
+                    return 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, new BiFunction<RubyString, WeakReference<RubyString>, WeakReference<RubyString>>() {
+                    @Override
+                    public WeakReference<RubyString> apply(RubyString key, WeakReference<RubyString> old) {
+                        return 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);
         }