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. IMHO, if we want to allow multi-threaded access to a single virConnectPtr object then we need to do the full job and make the whole thing safe, including adding thread-local error objects, to replace the inherantly non-thread safe global & per-virConnect error objects. The other alternative that we've discussed in the past is to add the ability to 'clone' an existing virConnectPtr object. The idea being that a 2nd thread could be given a clone, and safely use that. Internally we'd share & synchronize on relevant state information, so we didn't actually need to setup a separate network connection & re-auth for each. As it stands, this patch adding semi-thread safe access will make it harder for us to pursue this 2nd option of cloning virConnect, without breaking the Java bindings in future.
Basically it uses a mutex to protect reads from the RPC socket in such a way that message reads (in their entirety) are done atomically (otherwise the remoteDomainEventFired() can race the call() code that reads replies & events).
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
In addition, I update the EventImpl handle to prevent remoteDomainEventFired() from being called everytime a reply is sent. (This helps us dispatch events in a timely manner, though it's not strictly necessary. Without it, any events coming in during a call() won't be dispatched until the call drops the socket lock (because remoteDomainEventFired() will be stuck awaiting the lock).
I don't really like the idea of applying any of this patch, since I think it'll cause us unexpected complications when doing proper thread support in the next release of libvirt, and risk us breaking the java bindings. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|