Correctness issue in LazyReference
Chris Dennis <[email protected]>
| Newsgroups | gmane.comp.lang.groovy.devel |
|---|---|
| Message-ID | <CAM1ck5gZUu-y5Xs6AUWb6UC6YwA5g+27hm_+s8Dhfda=i7+Zjw@mail.gmail.com> |
Hi All, While debugging a strange Groovy method resolution failure (a spurious "groovy.lang.GroovyRuntimeException: Ambiguous method overloading..." exception) in an OpenJ9 JRE with maximally aggressive soft reference clearing I believe I've discovered a bug in LazyReference (by inspection). I'm suspicious that this might be the underlying cause of my method resolution problem but cannot be certain. The bug is that in this line: https://github.com/apache/groovy/blob/52c9ae912b8bd3b7cc27b68ec7611647952de989/src/main/java/org/codehaus/groovy/util/LazyReference.java#L37 The straight return of getLocked(false) isn't safe when racing multiple threads. If the GC clears the soft-reference placed by the "winning" thread then a contending thread can return via the `resRef != INIT` condition in getLocked(false) and mistakenly return null from the get() method. That `resRef.get()` return needs the same conditional logic as the more conventional path in get(). A crude fix can be seen in the attached patch. Let me know if this analysis makes sense and I'll file an appropriate JIRA issue. Thanks, Chris
lazy-ref.patch
(application/octet-stream, 1.1 KB)
diff --git a/src/main/java/org/codehaus/groovy/util/LazyReference.java b/src/main/java/org/codehaus/groovy/util/LazyReference.java
index bd091ddc5b..a211f26d08 100644
--- a/src/main/java/org/codehaus/groovy/util/LazyReference.java
+++ b/src/main/java/org/codehaus/groovy/util/LazyReference.java
@@ -28,7 +28,7 @@ public abstract class LazyReference<T> extends LockableObject {
private ManagedReference<T> reference = INIT;
private final ReferenceBundle bundle;
- public LazyReference(ReferenceBundle bundle) {
+ public LazyReference(ReferenceBundle bundle) {
this.bundle = bundle;
}
@@ -46,7 +46,11 @@ public abstract class LazyReference<T> extends LockableObject {
lock ();
try {
ManagedReference<T> resRef = reference;
- if (!force && resRef != INIT) return resRef.get();
+ if (!force && resRef != INIT) {
+ T res = resRef.get();
+ if (res != null)
+ return res;
+ }
T res = initValue();
if (res == null) {
reference = NULL_REFERENCE;