From c533dca65fe1407165e5ef32f9b7b634bd0650cd Mon Sep 17 00:00:00 2001 From: "Hyang-Ah (Hana) Kim" Date: Wed, 3 Jun 2015 06:12:57 -0400 Subject: [PATCH] bind/java: manage Java object lifetime based on reference count. The gobind framework is supposed to use reference counting to keep track of objects (e.g. pointer to a Go struct, interface values) crossing the language boundary. This change fixes two bugs: 1) no reference counting on Java object: Previously, the lifetime of a Java object was manages in the following way. a. The Java object is pinned in an internal map (javaObjs) when it's constructed. b. When Go receives the reference to the Java object, it creates a proxy object and sets a finalizer on it. The finalizer signals Java to unpin the Java object (remove from the javaObjs map). c. The javaObjs map is also used to identify the Java object when Go asks to invoke a method on it later. When the same Java object is sent to Java more than once, and the finalizer (b) runs after the first use, the second use of the Java object can cause the crash described in golang/go#10933. This change fixes the bug by reference counting the Java object. Java side pins the Java object and increments the refcount whenever it sees the object sent to Go (in Seq.writeRef). When the Go proxy object's finalizer runs, the refcount is decremented. When the refcount becomes 0, the object gets unpined. 2) race in Go object lifetime management: Pinning on a Go object has been done when the Go object is sent to Java but the Go object is not in the pinned object map yet. (bind/seq.WriteGoRef). Unpinning the object occurs when Java finds there are no proxy objects on its side. For this, Java maintains a reference count map (goObjs). When the refcount becomes zero, Java notifies Go so the object is unpinned. Here is a race case: a. Java has a proxy object for a Go object. b. Go is preparing for sending the same Go object. seq.WriteGoRef notices the corresponding entry in the pinned object map already, and returns. The remaining work for sending the object continues. c. The proxy object in Java finalizes and triggers deletion of the object from the pinned object map. d. The remaining work for (b) completes and Java creates a new proxy object. When a method is called for the Go object, the Go object is already removed from the object map on Go side and maybe already GC'd. This change fixes it by converting the pinned object map to reference counter map maintained in Go. The counter increments for each seq.WriteGoRef call. The finalizer of the proxy object in Java causes a decrement of the counter. Fixes golang/go#10933. Renables the skipped testJavaRefGC. Change-Id: I0992e002b1050b6183689e5ab821e058adbb420f Reviewed-on: https://go-review.googlesource.com/10638 Reviewed-by: David Crawshaw --- bind/java/Seq.java | 102 ++++++++++++++++++++--------------- bind/java/SeqTest.java | 45 +++++++++++----- bind/java/testpkg/testpkg.go | 14 ++++- bind/seq/buffer.go | 7 ++- bind/seq/ref.go | 28 ++++++---- 5 files changed, 127 insertions(+), 69 deletions(-) diff --git a/bind/java/Seq.java b/bind/java/Seq.java index 959ddd5..7c0b4a8 100644 --- a/bind/java/Seq.java +++ b/bind/java/Seq.java @@ -58,6 +58,7 @@ public class Seq { public native void writeByteArray(byte[] v); public void writeRef(Ref ref) { + tracker.inc(ref); writeInt32(ref.refnum); } @@ -160,20 +161,26 @@ public class Seq { // keyed by the Ref number. When the JVM calls finalize, we ask Go // to clear the entry in the map. public static final class Ref { - // ref < 0: Go object tracked by Java - // ref > 0: Java object tracked by Go + // refnum < 0: Go object tracked by Java + // refnum > 0: Java object tracked by Go int refnum; - public Seq.Object obj; + + int refcnt; // for Java obj: track how many times sent to Go. + + public Seq.Object obj; // for Java obj: pointers to the Java obj. private Ref(int refnum, Seq.Object o) { this.refnum = refnum; + this.refcnt = 0; this.obj = o; - tracker.inc(refnum); } @Override protected void finalize() throws Throwable { - tracker.dec(refnum); + if (refnum < 0) { + // Go object: signal Go to decrement the reference count. + Seq.destroyRef(refnum); + } super.finalize(); } } @@ -188,53 +195,50 @@ public class Seq { // to make debugging by reading Seq hex a little easier. private int next = 42; // next Java object ref - // TODO(crawshaw): We could cut down allocations for frequently - // sent Go objects by maintaining a map to weak references. This - // however, would require allocating two objects per reference - // instead of one. It also introduces weak references, the bane - // of any Java debugging session. - // - // When we have real code, examine the tradeoffs. - - // Number of active references to a Go object. refnum -> count - private SparseIntArray goObjs = new SparseIntArray(); - // Java objects that have been passed to Go. refnum -> Ref // The Ref obj field is non-null. // This map pins Java objects so they don't get GCed while the // only reference to them is held by Go code. private SparseArray javaObjs = new SparseArray(); - // inc increments the reference count to a Go object. - synchronized void inc(int refnum) { - if (refnum > 0) { - return; // we don't count java objects - } - int count = goObjs.get(refnum); - if (count == Integer.MAX_VALUE) { - throw new RuntimeException("refnum " + refnum + " overflow"); - } - goObjs.put(refnum, count+1); - } - - // dec decrements the reference count to a Go object. - // If the count reaches zero, the Go reference tracker is informed. - synchronized void dec(int refnum) { - if (refnum > 0) { - // Java objects are removed on request of Go. - javaObjs.remove(refnum); + // inc increments the reference count of a Java object when it + // is sent to Go. + synchronized void inc(Ref ref) { + int refnum = ref.refnum; + if (refnum <= 0) { + // We don't keep track of the Go object. return; } - int count = goObjs.get(refnum); - if (count == 0) { - throw new RuntimeException("refnum " + refnum + " underflow"); + // Count Java objects passed to Go. + if (ref.refcnt == Integer.MAX_VALUE) { + throw new RuntimeException("refnum " + refnum + " overflow"); } - count--; - if (count <= 0) { - goObjs.delete(refnum); - Seq.destroyRef(refnum); - } else { - goObjs.put(refnum, count); + ref.refcnt++; + Ref obj = javaObjs.get(refnum); + if (obj == null) { + javaObjs.put(refnum, ref); + } + } + + // dec decrements the reference count of a Java object when + // Go signals a corresponding proxy object is finalized. + // If the count reaches zero, the Java object is removed + // from the javaObjs map. + synchronized void dec(int refnum) { + if (refnum <= 0) { + // We don't keep track of the Go object. + // This must not happen. + Log.wtf("Seq", "dec request for Go object "+ refnum); + return; + } + // Java objects are removed on request of Go. + Ref obj = javaObjs.get(refnum); + if (obj == null) { + throw new RuntimeException("referenced Java object is not found: refnum="+refnum); + } + obj.refcnt--; + if (obj.refcnt <= 0) { + javaObjs.remove(refnum); } } @@ -251,6 +255,14 @@ public class Seq { // get returns an existing Ref to either a Java or Go object. // It may be the first time we have seen the Go object. + // + // TODO(crawshaw): We could cut down allocations for frequently + // sent Go objects by maintaining a map to weak references. This + // however, would require allocating two objects per reference + // instead of one. It also introduces weak references, the bane + // of any Java debugging session. + // + // When we have real code, examine the tradeoffs. synchronized Ref get(int refnum) { if (refnum > 0) { Ref ref = javaObjs.get(refnum); @@ -258,8 +270,10 @@ public class Seq { throw new RuntimeException("unknown java Ref: "+refnum); } return ref; + } else { + // Go object. + return new Ref(refnum, null); } - return new Ref(refnum, null); } } } diff --git a/bind/java/SeqTest.java b/bind/java/SeqTest.java index 1ed7fae..27bef2b 100644 --- a/bind/java/SeqTest.java +++ b/bind/java/SeqTest.java @@ -4,6 +4,7 @@ package go; +import android.util.Log; import android.test.suitebuilder.annotation.Suppress; import android.test.AndroidTestCase; import android.test.MoreAsserts; @@ -122,8 +123,6 @@ public class SeqTest extends AndroidTestCase { assertEquals("S should be collected", 1, collected); } - boolean finalizedAnI; - private class AnI extends Testpkg.I.Stub { public void E() throws Exception { throw new Exception("my exception from E"); @@ -160,12 +159,8 @@ public class SeqTest extends AndroidTestCase { return name; } - @Override - public void finalize() throws Throwable { - finalizedAnI = true; - super.finalize(); - } } + // TODO(hyangah): add tests for methods that take parameters. public void testInterfaceMethodReturnsError() { @@ -189,11 +184,19 @@ public class SeqTest extends AndroidTestCase { obj.name = "testing AnI.I"; Testpkg.I i = Testpkg.CallI(obj); assertEquals("Want AnI.I to return itself", i.String(), obj.String()); + + runGC(); + + i = Testpkg.CallI(obj); + assertEquals("Want AnI.I to return itself", i.String(), obj.String()); } public void testInterfaceMethodReturnsStructPointer() { final AnI obj = new AnI(); - Testpkg.S s = Testpkg.CallS(obj); + for (int i = 0; i < 5; i++) { + Testpkg.S s = Testpkg.CallS(obj); + runGC(); + } } public void testInterfaceMethodTakesStructPointer() { @@ -219,14 +222,22 @@ public class SeqTest extends AndroidTestCase { } } - /* Suppress this test for now; it's flaky or broken. */ - @Suppress + boolean finalizedAnI; + + private class AnI_Traced extends AnI { + @Override + public void finalize() throws Throwable { + finalizedAnI = true; + super.finalize(); + } + } + public void testJavaRefGC() { finalizedAnI = false; - AnI obj = new AnI(); - runGC(); + AnI obj = new AnI_Traced(); Testpkg.CallF(obj); assertTrue("want F to be called", obj.calledF); + Testpkg.CallF(obj); obj = null; runGC(); assertTrue("want obj to be collected", finalizedAnI); @@ -234,7 +245,15 @@ public class SeqTest extends AndroidTestCase { public void testJavaRefKeep() { finalizedAnI = false; - AnI obj = new AnI(); + AnI obj = new AnI_Traced(); + Testpkg.CallF(obj); + Testpkg.CallF(obj); + obj = null; + runGC(); + assertTrue("want obj not to be kept by Go", finalizedAnI); + + finalizedAnI = false; + obj = new AnI_Traced(); Testpkg.Keep(obj); obj = null; runGC(); diff --git a/bind/java/testpkg/testpkg.go b/bind/java/testpkg/testpkg.go index ea291b9..51313bb 100644 --- a/bind/java/testpkg/testpkg.go +++ b/bind/java/testpkg/testpkg.go @@ -76,7 +76,7 @@ func (s *S) String() string { return s.name } -func finalizeInner(*int) { +func finalizeInner(a *int) { numSCollected++ } @@ -144,3 +144,15 @@ func (a *Node) String() string { } return a.V + ":" + a.Next.String() } + +type Receiver interface { + Hello(message string) +} + +func Hello(r Receiver, name string) { + r.Hello(fmt.Sprintf("Hello, %s!\n", name)) +} + +func GarbageCollect() { + runtime.GC() +} diff --git a/bind/seq/buffer.go b/bind/seq/buffer.go index 372ecf2..45f40ca 100644 --- a/bind/seq/buffer.go +++ b/bind/seq/buffer.go @@ -244,14 +244,17 @@ func (b *Buffer) WriteString(v string) { func (b *Buffer) WriteGoRef(obj interface{}) { refs.Lock() num := refs.refs[obj] - if num == 0 { + if num != 0 { + s := refs.objs[num] + refs.objs[num] = countedObj{s.obj, s.cnt + 1} + } else { num = refs.next refs.next-- if refs.next > 0 { panic("refs.next underflow") } refs.refs[obj] = num - refs.objs[num] = obj + refs.objs[num] = countedObj{obj, 1} } refs.Unlock() diff --git a/bind/seq/ref.go b/bind/seq/ref.go index 0ebd896..2eed868 100644 --- a/bind/seq/ref.go +++ b/bind/seq/ref.go @@ -14,19 +14,24 @@ import ( "sync" ) +type countedObj struct { + obj interface{} + cnt int32 +} + // refs stores Go objects that have been passed to another language. var refs struct { sync.Mutex next int32 // next reference number to use for Go object, always negative refs map[interface{}]int32 - objs map[int32]interface{} + objs map[int32]countedObj } func init() { refs.Lock() refs.next = -24 // Go objects get negative reference numbers. Arbitrary starting point. refs.refs = make(map[interface{}]int32) - refs.objs = make(map[int32]interface{}) + refs.objs = make(map[int32]countedObj) refs.Unlock() } @@ -39,22 +44,27 @@ type Ref struct { // Get returns the underlying object. func (r *Ref) Get() interface{} { refs.Lock() - obj, ok := refs.objs[r.Num] + o, ok := refs.objs[r.Num] refs.Unlock() if !ok { panic(fmt.Sprintf("unknown ref %d", r.Num)) } - return obj + return o.obj } -// Delete remove the reference to the underlying object. +// Delete decrements the reference count and removes the pinned object +// from the object map when the reference count becomes zero. func Delete(num int32) { refs.Lock() - obj, ok := refs.objs[num] + defer refs.Unlock() + o, ok := refs.objs[num] if !ok { panic(fmt.Sprintf("seq.Delete unknown refnum: %d", num)) } - delete(refs.objs, num) - delete(refs.refs, obj) - refs.Unlock() + if o.cnt <= 1 { + delete(refs.objs, num) + delete(refs.refs, o.obj) + } else { + refs.objs[num] = countedObj{o.obj, o.cnt - 1} + } }