Re: [libvirt] [PATCH 2/2] Java bindings for domain events
On Tue, 2008-11-18 at 17:05 +0000, Daniel P. Berrange wrote:
On Tue, Nov 18, 2008 at 11:18:38AM -0500, David Lively wrote:
> This patch allows the remote driver to work with an asynchronous
> EventImpl (it's the only one using an externally-supplied one), assuming
> libvirt is compiled with pthread support. (Without pthreads, this code
> is harmless in a single-threaded environment.)
I don't like this patch, since it is only making one tiny part of the
API thread-safe, for one tiny use case. eg, if someone uses events from
the Xen driver in Java with threads, they'll crash & burn, since only
the remote driver is being protected.
Currently ONLY the remote driver (and yes, soon - the xen driver, which
would also need thread-safety changes) use an EventImpl supplied
externally. All others use the libvirtd-provided synchronous impl.
Why doesn't the Java code simply synchronize on the virConnect
object
before invoking the FD event callbacks. That will ensure another
thread has finished whatever API call it was doing
Note that the Java code uses (Java's builtin) Connect-level lock to
avoid concurrent calls using the same virConnect. This even applies to
domain event delivery - note that Connect.fireDomainEventCallbacks is
(in the Java patch) a synchronized method. A FD event callback isn't
associated with a particular virConnect object, and EventImpls aren't
virConnect-specific, so I can't lock "the virConnect" (see below).
When we exposed virEventRegisterImpl, we said that externally-supplied
event impls must not make callbacks when a virConnect is being used. If
the Java EventImpl were following this rule, we wouldn't need this
patch. BUT because the EventImpl can't know which virConnect (if any)
is involved in a particular callback, the only way to satisfy this rule
is to lock ALL Connect objects before making an EventImpl callback.
This is (IMO) both way too restrictive, and (I'm a little less sure of
this) not restrictive enough. (I suspect we'd find that making two
callbacks concurrently can break things, even if no virConnects are in
use at the time.)
I think we have to allow externally supplied EventImpls to make
callbacks without regard to which virConnect, etc. objects are in use,
since EventImpls don't know anything about virConnect, etc. This
doesn't require all of libvirt to be made thread-safe.
So I view this fix (and the one necessary for the xen driver once it
starts using an externally-supplied EventImpl) as providing just enough
concurrency control to allow an asynchronous EventImpl, while still
making the libvirt user (the Java bindings, in my case) responsible for
avoiding concurrent virConnect usage.