9:43 p.m.
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
9:43 p.m.
New subject: [libvirt] [PATCH 1/2] Add close listener types
---
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
2:17 a.m.
New subject: [libvirt] [PATCH 1/2] Add close listener types
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.
9:43 p.m.
New subject: [libvirt] [PATCH 2/2] Ensure JNA callbacks cannot get GCed
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
2:24 a.m.
New subject: [libvirt] [PATCH 2/2] Ensure JNA callbacks cannot get GCed
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?
5 a.m.
New subject: [libvirt] [PATCH 2/2] Ensure JNA callbacks cannot get GCed
On Wed, Mar 26, 2014 at 7:24 AM, Claudio Bley <cbley(a)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
3894
days inactive
3894
days old
5 comments
2 participants
participants (2)
-
Chris Ellis -
Claudio Bley