[libvirt] [libvirt-java] [PATCH 0/2] Ensure JNA callbacks cannot be GCed
Hi. This is a small patch set to fix a few issues I've discovered while testing Claudio's patch set. The first patch is trivial, it adds the ConnectionCloseListener interface and corresponding enum, which were missing. The second patch ensures that the JNA callbacks cannot be garbage collected whilst still registered with the libvirt C library. Durring testing, I was finding that events would stop working. Wireshark showed the remote daemon sending the event and some time spent tracing through the libvirt calls led me to discover that the JNA callback objects were being GCed. The JNA documentation states that a reference to the Callback object must be held whilst it is in use by the C layer. This patch updates the map of event listeners to also hold a reference to the callback object. Regards, Chris Chris Ellis (2): Add close listener types Ensure JNA callbacks cannot get GCed src/main/java/org/libvirt/Connect.java | 37 ++++++++++++++-------- .../org/libvirt/event/ConnectionCloseListener.java | 9 ++++++ .../org/libvirt/event/ConnectionCloseReason.java | 8 +++++ 3 files changed, 41 insertions(+), 13 deletions(-) create mode 100644 src/main/java/org/libvirt/event/ConnectionCloseListener.java create mode 100644 src/main/java/org/libvirt/event/ConnectionCloseReason.java -- 1.8.4.5
--- src/main/java/org/libvirt/event/ConnectionCloseListener.java | 9 +++++++++ src/main/java/org/libvirt/event/ConnectionCloseReason.java | 8 ++++++++ 2 files changed, 17 insertions(+) create mode 100644 src/main/java/org/libvirt/event/ConnectionCloseListener.java create mode 100644 src/main/java/org/libvirt/event/ConnectionCloseReason.java diff --git a/src/main/java/org/libvirt/event/ConnectionCloseListener.java b/src/main/java/org/libvirt/event/ConnectionCloseListener.java new file mode 100644 index 0000000..f6e8973 --- /dev/null +++ b/src/main/java/org/libvirt/event/ConnectionCloseListener.java @@ -0,0 +1,9 @@ +package org.libvirt.event; + +import org.libvirt.Connect; + +public interface ConnectionCloseListener extends EventListener { + + void onClose(Connect connection, ConnectionCloseReason reason); + +} diff --git a/src/main/java/org/libvirt/event/ConnectionCloseReason.java b/src/main/java/org/libvirt/event/ConnectionCloseReason.java new file mode 100644 index 0000000..0e94452 --- /dev/null +++ b/src/main/java/org/libvirt/event/ConnectionCloseReason.java @@ -0,0 +1,8 @@ +package org.libvirt.event; + +public enum ConnectionCloseReason { + VIR_CONNECT_CLOSE_REASON_ERROR, + VIR_CONNECT_CLOSE_REASON_EOF, + VIR_CONNECT_CLOSE_REASON_KEEPALIVE, + VIR_CONNECT_CLOSE_REASON_CLIENT; +} -- 1.8.4.5
At Wed, 26 Mar 2014 02:43:48 +0000, Chris Ellis wrote:
--- src/main/java/org/libvirt/event/ConnectionCloseListener.java | 9 +++++++++ src/main/java/org/libvirt/event/ConnectionCloseReason.java | 8 ++++++++
You're right, those files were missing because I forgot to 'git add' them before submitting my patchset. But, why didn't you just say so as you reviewed the patches? NACK, since I already have those files in my local branch including javadoc comments, with short names for the enum constants and the usual "UNKNOWN" constant. Here's what I squashed into patch 49/65 of the series: ----------- >8 ----------------- 8< ---------------------------------- diff --git a/src/main/java/org/libvirt/event/ConnectionCloseListener.java b/src/main/java/org/libvirt/event/ConnectionCloseListener.java new file mode 100644 index 0000000..4845b53 --- /dev/null +++ b/src/main/java/org/libvirt/event/ConnectionCloseListener.java @@ -0,0 +1,10 @@ +package org.libvirt.event; + +import org.libvirt.Connect; + +/** + * Interface for receiving events when a connection is closed. + */ +public interface ConnectionCloseListener { + void onClose(Connect conn, ConnectionCloseReason reason); +} diff --git a/src/main/java/org/libvirt/event/ConnectionCloseReason.java b/src/main/java/org/libvirt/event/ConnectionCloseReason.java new file mode 100644 index 0000000..0dd2924 --- /dev/null +++ b/src/main/java/org/libvirt/event/ConnectionCloseReason.java @@ -0,0 +1,17 @@ +package org.libvirt.event; + +public enum ConnectionCloseReason { + /** Misc I/O error */ + ERROR, + + /** End-of-file from server */ + EOF, + + /** Keepalive timer triggered */ + KEEPALIVE, + + /** Client requested it */ + CLIENT, + + UNKNOWN +} -- BSc (Comp) Claudio Bley - Principal Software Engineer AV-TEST GmbH, Klewitzstr. 7, 39112 Magdeburg, Germany Phone: +49 391 6075460, Fax: +49 391 6075469 Web: <http://www.av-test.org> * https://twitter.com/avtestorg * https://facebook.com/avtestorg * * https://plus.google.com/100383867141221115206/ * Eingetragen am / Registered at: Amtsgericht Stendal (HRB 114076) Geschaeftsfuehrer (CEO): Andreas Marx, Guido Habicht, Maik Morgenstern Our services shall be effected on the basis of the General Terms and Conditions of AV-TEST GmbH, which are accessible under <http://www.av-test.org/en/av-test/terms-and-conditions/> or obtainable upon request. Unsere Leistungen erfolgen auf der Grundlage der Allgemeinen Geschäftsbedingungen der AV-TEST GmbH, die unter <http://www.av-test.org/av-test/agb/> abrufbar sind oder auf Anfrage übersandt werden.
Currently nothing prevents the JNA callback objects used when registering for domain events from being garbage collected. JNA requires that callback objects are not GCed whilst they are in use by C code. To solve this we hold a reference to the callback alongside the callback id. This ensures that the JNA callback objects will retain in memory while it is registered with the C layer. We also use an IdentityHashMap rather than a HashMap to store the EventListener objects. --- src/main/java/org/libvirt/Connect.java | 37 ++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/libvirt/Connect.java b/src/main/java/org/libvirt/Connect.java index 43909f4..7a5904d 100644 --- a/src/main/java/org/libvirt/Connect.java +++ b/src/main/java/org/libvirt/Connect.java @@ -41,8 +41,21 @@ import com.sun.jna.ptr.LongByReference; public class Connect { // registered event listeners by DomainEventID - private Map<EventListener, Integer>[] eventListeners = makeHashMapArray(DomainEventID.LAST); - + private Map<EventListener, RegisteredEventListener>[] eventListeners = makeHashMapArray(DomainEventID.LAST); + + private class RegisteredEventListener { + + public final int callbackId; + + // We need to keep a reference to the callback to prevent it from being GCed + @SuppressWarnings("unused") + public final Libvirt.VirDomainEventCallback callback; + + public RegisteredEventListener(Libvirt.VirDomainEventCallback callback, int callbackId) { + this.callback = callback; + this.callbackId = callbackId; + } + } @SuppressWarnings("unchecked") private static <K, V> HashMap<K, V>[] makeHashMapArray(int size) { return new HashMap[size]; @@ -517,35 +530,33 @@ public class Connect { if (l == null) return; - Map<EventListener, Integer> handlers = eventListeners[eventID]; + Map<EventListener, RegisteredEventListener> handlers = eventListeners[eventID]; if (handlers == null) return; - Integer listenerID = handlers.remove(l); + RegisteredEventListener listenerID = handlers.remove(l); if (listenerID != null) - processError(libvirt.virConnectDomainEventDeregisterAny(VCP, listenerID)); + processError(libvirt.virConnectDomainEventDeregisterAny(VCP, listenerID.callbackId)); } private void domainEventRegister(Domain domain, int eventID, Libvirt.VirDomainEventCallback cb, EventListener l) throws LibvirtException { - Map<EventListener, Integer> handlers = eventListeners[eventID]; + Map<EventListener, RegisteredEventListener> handlers = eventListeners[eventID]; if (handlers == null) { - handlers = new HashMap<EventListener, Integer>(); + handlers = new HashMap<EventListener, RegisteredEventListener>(); eventListeners[eventID] = handlers; } else if (handlers.containsKey(l)) { return; } DomainPointer ptr = domain == null ? null : domain.VDP; - - int ret = processError(libvirt.virConnectDomainEventRegisterAny(VCP, ptr, - eventID, cb, - null, null)); - - handlers.put(l, ret); + int ret = processError(libvirt.virConnectDomainEventRegisterAny(VCP, ptr, eventID, cb, null, null)); + // track the handler + // Note: it is important that the callback does not get GCed + handlers.put(l, new RegisteredEventListener(cb, ret)); } void domainEventRegister(Domain domain, final IOErrorListener cb) throws LibvirtException { -- 1.8.4.5
At Wed, 26 Mar 2014 02:43:49 +0000, Chris Ellis wrote:
Currently nothing prevents the JNA callback objects used when registering for domain events from being garbage collected. JNA requires that callback objects are not GCed whilst they are in use by C code.
To solve this we hold a reference to the callback alongside the callback id. This ensures that the JNA callback objects will retain in memory while it is registered with the C layer.
We also use an IdentityHashMap rather than a HashMap to store the EventListener objects. --- src/main/java/org/libvirt/Connect.java | 37 ++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 13 deletions(-)
Nice catch. ACK. Should we squash this into patch 41/65 before pushing the series?
On Wed, Mar 26, 2014 at 7:24 AM, Claudio Bley <cbley@av-test.de> wrote:
At Wed, 26 Mar 2014 02:43:49 +0000, Chris Ellis wrote:
Currently nothing prevents the JNA callback objects used when registering for domain events from being garbage collected. JNA requires that
callback
objects are not GCed whilst they are in use by C code.
To solve this we hold a reference to the callback alongside the callback id. This ensures that the JNA callback objects will retain in memory while it is registered with the C layer.
We also use an IdentityHashMap rather than a HashMap to store the EventListener objects. --- src/main/java/org/libvirt/Connect.java | 37 ++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 13 deletions(-)
Nice catch. ACK.
Thanks.
Should we squash this into patch 41/65 before pushing the series?
Yes that would make sense, or you can chain my patch at the end, I'll leave that upto you. I've noticed that there are some other related errors, I'll push patches for these later today. Chris
participants (2)
-
Chris Ellis -
Claudio Bley