[libvirt] [PATCH v2 0/6] libvirt-python/libvirtaio patches for 3.8.0
Hi libvirt-list, I'd like to submit a second iteration of libvirtaio series for 3.8.0. There are some improvements to have better control over closing connections, which are important in test suites which repeatedly open and close connections. Also there are some minor bugfixes and better logging. Since v1 (31.08.2017) there are changes: - dropped implementation re-registering - added current implementation getter instead - added unrelated bugfixes Wojtek Porczyk (6): libvirtaio: add more debug logging libvirtaio: cache the list of callbacks when calling libvirtaio: do not double-add callbacks libvirtaio: fix closing of the objects libvirtaio: keep track of the current implementation libvirtaio: add .drain() coroutine libvirtaio.py | 101 +++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 78 insertions(+), 23 deletions(-) -- 2.9.4
This was a harmless bug, without any impact, but it is wrong to manage the collection of callbacks from it's members. Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/libvirtaio.py b/libvirtaio.py index d962e64..239561d 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -63,11 +63,6 @@ class Callback(object): self.cb = cb self.opaque = opaque - assert self.iden not in self.impl.callbacks, \ - 'found {} callback: {!r}'.format( - self.iden, self.impl.callbacks[self.iden]) - self.impl.callbacks[self.iden] = self - def __repr__(self): return '<{} iden={}>'.format(self.__class__.__name__, self.iden) @@ -324,6 +319,8 @@ class virEventAsyncIOImpl(object): ''' callback = FDCallback(self, cb, opaque, descriptor=self.descriptors[fd], event=event) + assert callback.iden not in self.callbacks + self.log.debug('add_handle(fd=%d, event=%d, cb=..., opaque=...) = %d', fd, event, callback.iden) self.callbacks[callback.iden] = callback @@ -376,6 +373,8 @@ class virEventAsyncIOImpl(object): https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddTimeoutFunc ''' callback = TimeoutCallback(self, cb, opaque) + assert callback.iden not in self.callbacks + self.log.debug('add_timeout(timeout=%d, cb=..., opaque=...) = %d', timeout, callback.iden) self.callbacks[callback.iden] = callback -- 2.9.4
On Thu, Sep 14, 2017 at 02:24:29AM +0200, Wojtek Porczyk wrote:
This was a harmless bug, without any impact, but it is wrong to manage the collection of callbacks from it's members.
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/libvirtaio.py b/libvirtaio.py index d962e64..239561d 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -63,11 +63,6 @@ class Callback(object): self.cb = cb self.opaque = opaque
- assert self.iden not in self.impl.callbacks, \ - 'found {} callback: {!r}'.format( - self.iden, self.impl.callbacks[self.iden]) - self.impl.callbacks[self.iden] = self - def __repr__(self): return '<{} iden={}>'.format(self.__class__.__name__, self.iden)
@@ -324,6 +319,8 @@ class virEventAsyncIOImpl(object): ''' callback = FDCallback(self, cb, opaque, descriptor=self.descriptors[fd], event=event) + assert callback.iden not in self.callbacks + self.log.debug('add_handle(fd=%d, event=%d, cb=..., opaque=...) = %d', fd, event, callback.iden) self.callbacks[callback.iden] = callback @@ -376,6 +373,8 @@ class virEventAsyncIOImpl(object): https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddTimeoutFunc ''' callback = TimeoutCallback(self, cb, opaque) + assert callback.iden not in self.callbacks + self.log.debug('add_timeout(timeout=%d, cb=..., opaque=...) = %d', timeout, callback.iden) self.callbacks[callback.iden] = callback
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
- Descriptor.close() was a dead code, never used. - TimeoutCallback.close(), as a cleanup function, should have called super() as last statement, not first Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/libvirtaio.py b/libvirtaio.py index 239561d..a7ede41 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -154,11 +154,6 @@ class Descriptor(object): self.update() return callback - def close(self): - '''''' - self.callbacks.clear() - self.update() - class DescriptorDict(dict): '''Descriptors collection @@ -249,8 +244,8 @@ class TimeoutCallback(Callback): def close(self): '''Stop the timer and call ff callback''' - super(TimeoutCallback, self).close() self.update(timeout=-1) + super(TimeoutCallback, self).close() # # main implementation -- 2.9.4
Since 7534c19 it is not possible to register event implementation twice. Instead, allow for retrieving the current one, should it be needed afterwards. Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/libvirtaio.py b/libvirtaio.py index a7ede41..97a7f6c 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -30,7 +30,11 @@ Register the implementation of default loop: __author__ = 'Wojtek Porczyk <woju@invisiblethingslab.com>' __license__ = 'LGPL-2.1+' -__all__ = ['virEventAsyncIOImpl', 'virEventRegisterAsyncIOImpl'] +__all__ = [ + 'getCurrentImpl', + 'virEventAsyncIOImpl', + 'virEventRegisterAsyncIOImpl', +] import asyncio import itertools @@ -401,10 +405,18 @@ class virEventAsyncIOImpl(object): callback = self.callbacks.pop(timer) callback.close() + +_current_impl = None +def getCurrentImpl(): + '''Return the current implementation, or None if not yet registered''' + return _current_impl + def virEventRegisterAsyncIOImpl(loop=None): '''Arrange for libvirt's callbacks to be dispatched via asyncio event loop The implementation object is returned, but in normal usage it can safely be discarded. ''' - return virEventAsyncIOImpl(loop=loop).register() + global _current_impl + _current_impl = virEventAsyncIOImpl(loop=loop).register() + return _current_impl -- 2.9.4
The intended use is to ensure that the implementation is empty, which is one way to ensure that all connections were properly closed and file descriptors reclaimed. Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/libvirtaio.py b/libvirtaio.py index 97a7f6c..1c432dd 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -269,10 +269,27 @@ class virEventAsyncIOImpl(object): self.descriptors = DescriptorDict(self) self.log = logging.getLogger(self.__class__.__name__) + # NOTE invariant: _finished.is_set() iff _pending == 0 + self._pending = 0 + self._finished = asyncio.Event(loop=loop) + self._finished.set() + def __repr__(self): return '<{} callbacks={} descriptors={}>'.format( type(self).__name__, self.callbacks, self.descriptors) + def _pending_inc(self): + '''Increase the count of pending affairs. Do not use directly.''' + self._pending += 1 + self._finished.clear() + + def _pending_dec(self): + '''Decrease the count of pending affairs. Do not use directly.''' + assert self._pending > 0 + self._pending -= 1 + if self._pending == 0: + self._finished.set() + def register(self): '''Register this instance as event loop implementation''' # pylint: disable=bad-whitespace @@ -293,7 +310,20 @@ class virEventAsyncIOImpl(object): This is a coroutine. ''' self.log.debug('ff_callback(iden=%d, opaque=...)', iden) - return libvirt.virEventInvokeFreeCallback(opaque) + ret = libvirt.virEventInvokeFreeCallback(opaque) + self._pending_dec() + return ret + + @asyncio.coroutine + def drain(self): + '''Wait for the implementation to become idle. + + This is a coroutine. + ''' + self.log.debug('drain()') + if self._pending: + yield from self._finished.wait() + self.log.debug('drain ended') def is_idle(self): '''Returns False if there are leftovers from a connection @@ -301,7 +331,7 @@ class virEventAsyncIOImpl(object): Those may happen if there are sematical problems while closing a connection. For example, not deregistered events before .close(). ''' - return not self.callbacks + return not self.callbacks and not self._pending def _add_handle(self, fd, event, cb, opaque): '''Register a callback for monitoring file handle events @@ -324,6 +354,7 @@ class virEventAsyncIOImpl(object): fd, event, callback.iden) self.callbacks[callback.iden] = callback self.descriptors[fd].add_handle(callback) + self._pending_inc() return callback.iden def _update_handle(self, watch, event): @@ -378,6 +409,7 @@ class virEventAsyncIOImpl(object): timeout, callback.iden) self.callbacks[callback.iden] = callback callback.update(timeout=timeout) + self._pending_inc() return callback.iden def _update_timeout(self, timer, timeout): -- 2.9.4
On Thu, Sep 14, 2017 at 02:41:12AM +0200, Wojtek Porczyk wrote:
The intended use is to ensure that the implementation is empty, which is one way to ensure that all connections were properly closed and file descriptors reclaimed.
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/libvirtaio.py b/libvirtaio.py index 97a7f6c..1c432dd 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -269,10 +269,27 @@ class virEventAsyncIOImpl(object): self.descriptors = DescriptorDict(self) self.log = logging.getLogger(self.__class__.__name__)
+ # NOTE invariant: _finished.is_set() iff _pending == 0 + self._pending = 0 + self._finished = asyncio.Event(loop=loop) + self._finished.set() + def __repr__(self): return '<{} callbacks={} descriptors={}>'.format( type(self).__name__, self.callbacks, self.descriptors)
+ def _pending_inc(self): + '''Increase the count of pending affairs. Do not use directly.''' + self._pending += 1 + self._finished.clear() + + def _pending_dec(self): + '''Decrease the count of pending affairs. Do not use directly.''' + assert self._pending > 0 + self._pending -= 1 + if self._pending == 0: + self._finished.set() + def register(self): '''Register this instance as event loop implementation''' # pylint: disable=bad-whitespace @@ -293,7 +310,20 @@ class virEventAsyncIOImpl(object): This is a coroutine. ''' self.log.debug('ff_callback(iden=%d, opaque=...)', iden) - return libvirt.virEventInvokeFreeCallback(opaque) + ret = libvirt.virEventInvokeFreeCallback(opaque) + self._pending_dec() + return ret + + @asyncio.coroutine + def drain(self): + '''Wait for the implementation to become idle. + + This is a coroutine. + ''' + self.log.debug('drain()') + if self._pending: + yield from self._finished.wait() + self.log.debug('drain ended')
What is responsible for calling 'drain' ?
def is_idle(self): '''Returns False if there are leftovers from a connection @@ -301,7 +331,7 @@ class virEventAsyncIOImpl(object): Those may happen if there are sematical problems while closing a connection. For example, not deregistered events before .close(). ''' - return not self.callbacks + return not self.callbacks and not self._pending
def _add_handle(self, fd, event, cb, opaque): '''Register a callback for monitoring file handle events @@ -324,6 +354,7 @@ class virEventAsyncIOImpl(object): fd, event, callback.iden) self.callbacks[callback.iden] = callback self.descriptors[fd].add_handle(callback) + self._pending_inc() return callback.iden
def _update_handle(self, watch, event): @@ -378,6 +409,7 @@ class virEventAsyncIOImpl(object): timeout, callback.iden) self.callbacks[callback.iden] = callback callback.update(timeout=timeout) + self._pending_inc() return callback.iden
def _update_timeout(self, timer, timeout):
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Sep 25, 2017 at 02:53:01PM +0100, Daniel P. Berrange wrote:
On Thu, Sep 14, 2017 at 02:41:12AM +0200, Wojtek Porczyk wrote:
The intended use is to ensure that the implementation is empty, which is one way to ensure that all connections were properly closed and file descriptors reclaimed.
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- (snip) + @asyncio.coroutine + def drain(self): + '''Wait for the implementation to become idle. + + This is a coroutine. + ''' + self.log.debug('drain()') + if self._pending: + yield from self._finished.wait() + self.log.debug('drain ended')
What is responsible for calling 'drain' ?
Users of the library, and they do it at their pleasure. This is to allow the loop to actually run the scheduled tasks. After calling virConnect.close() the handles/timeouts aren't actually closed until the loop run the scheduled callbacks. In practice a simple `yield` in a coroutine would be also sufficient since respective Tasks are in the _ready queue and all run during next loop cycle, but that's not guaranteed in asyncio specification. -- pozdrawiam / best regards _.-._ Wojtek Porczyk .-^' '^-. Invisible Things Lab |'-.-^-.-'| | | | | I do not fear computers, | '-.-' | I fear lack of them. '-._ : ,-' -- Isaac Asimov `^-^-_>
On Tue, Sep 26, 2017 at 12:47:00AM +0200, Wojtek Porczyk wrote:
On Mon, Sep 25, 2017 at 02:53:01PM +0100, Daniel P. Berrange wrote:
On Thu, Sep 14, 2017 at 02:41:12AM +0200, Wojtek Porczyk wrote:
The intended use is to ensure that the implementation is empty, which is one way to ensure that all connections were properly closed and file descriptors reclaimed.
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- (snip) + @asyncio.coroutine + def drain(self): + '''Wait for the implementation to become idle. + + This is a coroutine. + ''' + self.log.debug('drain()') + if self._pending: + yield from self._finished.wait() + self.log.debug('drain ended')
What is responsible for calling 'drain' ?
Users of the library, and they do it at their pleasure. This is to allow the loop to actually run the scheduled tasks. After calling virConnect.close() the handles/timeouts aren't actually closed until the loop run the scheduled callbacks. In practice a simple `yield` in a coroutine would be also sufficient since respective Tasks are in the _ready queue and all run during next loop cycle, but that's not guaranteed in asyncio specification.
Ah I see. Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
When the callback causes something that results in changes wrt registered handles, python aborts iteration. Relevant error message: Exception in callback None() handle: <Handle cancelled> Traceback (most recent call last): File "/usr/lib64/python3.5/asyncio/events.py", line 126, in _run self._callback(*self._args) File "/usr/lib64/python3.5/site-packages/libvirtaio.py", line 99, in _handle for callback in self.callbacks.values(): RuntimeError: dictionary changed size during iteration QubesOS/qubes-issues#2805 Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libvirtaio.py b/libvirtaio.py index 46de9ab..d962e64 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -96,7 +96,7 @@ class Descriptor(object): :param int event: The event (from libvirt's constants) being dispatched ''' - for callback in self.callbacks.values(): + for callback in list(self.callbacks.values()): if callback.event is not None and callback.event & event: callback.cb(callback.iden, self.fd, event, callback.opaque) -- 2.9.4
On Thu, Aug 31, 2017 at 09:20:40PM +0200, Wojtek Porczyk wrote:
When the callback causes something that results in changes wrt registered handles, python aborts iteration.
Relevant error message:
Exception in callback None() handle: <Handle cancelled> Traceback (most recent call last): File "/usr/lib64/python3.5/asyncio/events.py", line 126, in _run self._callback(*self._args) File "/usr/lib64/python3.5/site-packages/libvirtaio.py", line 99, in _handle for callback in self.callbacks.values(): RuntimeError: dictionary changed size during iteration
QubesOS/qubes-issues#2805 Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libvirtaio.py b/libvirtaio.py index 46de9ab..d962e64 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -96,7 +96,7 @@ class Descriptor(object):
:param int event: The event (from libvirt's constants) being dispatched ''' - for callback in self.callbacks.values(): + for callback in list(self.callbacks.values()): if callback.event is not None and callback.event & event: callback.cb(callback.iden, self.fd, event, callback.opaque)
Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
This logging is helpful for tracing problems with unclosed connections and leaking file descriptors. Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/libvirtaio.py b/libvirtaio.py index 7c8c396..46de9ab 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -74,7 +74,7 @@ class Callback(object): def close(self): '''Schedule *ff* callback''' self.impl.log.debug('callback %d close(), scheduling ff', self.iden) - self.impl.schedule_ff_callback(self.opaque) + self.impl.schedule_ff_callback(self.iden, self.opaque) # # file descriptors @@ -275,6 +275,10 @@ class virEventAsyncIOImpl(object): self.descriptors = DescriptorDict(self) self.log = logging.getLogger(self.__class__.__name__) + def __repr__(self): + return '<{} callbacks={} descriptors={}>'.format( + type(self).__name__, self.callbacks, self.descriptors) + def register(self): '''Register this instance as event loop implementation''' # pylint: disable=bad-whitespace @@ -284,9 +288,18 @@ class virEventAsyncIOImpl(object): self._add_timeout, self._update_timeout, self._remove_timeout) return self - def schedule_ff_callback(self, opaque): + def schedule_ff_callback(self, iden, opaque): '''Schedule a ff callback from one of the handles or timers''' - self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque) + ensure_future(self._ff_callback(iden, opaque), loop=self.loop) + + @asyncio.coroutine + def _ff_callback(self, iden, opaque): + '''Directly free the opaque object + + This is a coroutine. + ''' + self.log.debug('ff_callback(iden=%d, opaque=...)', iden) + return libvirt.virEventInvokeFreeCallback(opaque) def is_idle(self): '''Returns False if there are leftovers from a connection @@ -309,10 +322,10 @@ class virEventAsyncIOImpl(object): .. seealso:: https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddHandleFuncFun... ''' - self.log.debug('add_handle(fd=%d, event=%d, cb=%r, opaque=%r)', - fd, event, cb, opaque) callback = FDCallback(self, cb, opaque, descriptor=self.descriptors[fd], event=event) + self.log.debug('add_handle(fd=%d, event=%d, cb=..., opaque=...) = %d', + fd, event, callback.iden) self.callbacks[callback.iden] = callback self.descriptors[fd].add_handle(callback) return callback.iden @@ -339,7 +352,11 @@ class virEventAsyncIOImpl(object): https://libvirt.org/html/libvirt-libvirt-event.html#virEventRemoveHandleFunc ''' self.log.debug('remove_handle(watch=%d)', watch) - callback = self.callbacks.pop(watch) + try: + callback = self.callbacks.pop(watch) + except KeyError as err: + self.log.warning('remove_handle(): no such handle: %r', err.args[0]) + raise fd = callback.descriptor.fd assert callback is self.descriptors[fd].remove_handle(watch) if len(self.descriptors[fd].callbacks) == 0: @@ -358,9 +375,9 @@ class virEventAsyncIOImpl(object): .. seealso:: https://libvirt.org/html/libvirt-libvirt-event.html#virEventAddTimeoutFunc ''' - self.log.debug('add_timeout(timeout=%d, cb=%r, opaque=%r)', - timeout, cb, opaque) callback = TimeoutCallback(self, cb, opaque) + self.log.debug('add_timeout(timeout=%d, cb=..., opaque=...) = %d', + timeout, callback.iden) self.callbacks[callback.iden] = callback callback.update(timeout=timeout) return callback.iden -- 2.9.4
On Thu, Aug 24, 2017 at 09:22:49PM +0200, Wojtek Porczyk wrote:
This logging is helpful for tracing problems with unclosed connections and leaking file descriptors.
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/libvirtaio.py b/libvirtaio.py index 7c8c396..46de9ab 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -74,7 +74,7 @@ class Callback(object): def close(self): '''Schedule *ff* callback''' self.impl.log.debug('callback %d close(), scheduling ff', self.iden) - self.impl.schedule_ff_callback(self.opaque) + self.impl.schedule_ff_callback(self.iden, self.opaque)
# # file descriptors @@ -275,6 +275,10 @@ class virEventAsyncIOImpl(object): self.descriptors = DescriptorDict(self) self.log = logging.getLogger(self.__class__.__name__)
+ def __repr__(self): + return '<{} callbacks={} descriptors={}>'.format( + type(self).__name__, self.callbacks, self.descriptors) + def register(self): '''Register this instance as event loop implementation''' # pylint: disable=bad-whitespace @@ -284,9 +288,18 @@ class virEventAsyncIOImpl(object): self._add_timeout, self._update_timeout, self._remove_timeout) return self
- def schedule_ff_callback(self, opaque): + def schedule_ff_callback(self, iden, opaque): '''Schedule a ff callback from one of the handles or timers''' - self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque) + ensure_future(self._ff_callback(iden, opaque), loop=self.loop)
This use of ensure_future puts a min python version of 3.4 on the code. Is there a strong reason to prefer that over the previous call_soon code ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Sep 25, 2017 at 02:43:56PM +0100, Daniel P. Berrange wrote:
On Thu, Aug 24, 2017 at 09:22:49PM +0200, Wojtek Porczyk wrote:
This logging is helpful for tracing problems with unclosed connections and leaking file descriptors.
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/libvirtaio.py b/libvirtaio.py index 7c8c396..46de9ab 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -74,7 +74,7 @@ class Callback(object): def close(self): '''Schedule *ff* callback''' self.impl.log.debug('callback %d close(), scheduling ff', self.iden) - self.impl.schedule_ff_callback(self.opaque) + self.impl.schedule_ff_callback(self.iden, self.opaque)
# # file descriptors @@ -275,6 +275,10 @@ class virEventAsyncIOImpl(object): self.descriptors = DescriptorDict(self) self.log = logging.getLogger(self.__class__.__name__)
+ def __repr__(self): + return '<{} callbacks={} descriptors={}>'.format( + type(self).__name__, self.callbacks, self.descriptors) + def register(self): '''Register this instance as event loop implementation''' # pylint: disable=bad-whitespace @@ -284,9 +288,18 @@ class virEventAsyncIOImpl(object): self._add_timeout, self._update_timeout, self._remove_timeout) return self
- def schedule_ff_callback(self, opaque): + def schedule_ff_callback(self, iden, opaque): '''Schedule a ff callback from one of the handles or timers''' - self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque) + ensure_future(self._ff_callback(iden, opaque), loop=self.loop)
This use of ensure_future puts a min python version of 3.4 on the code. Is there a strong reason to prefer that over the previous call_soon code ?
1) There is no technical reason whatsoever. This is an artifact from the previous iteration. 2) In fact ensure_future() does not make it incompatible with python<3.4 because of the try/except import at the beginning of the file (in python<3.4.3 the function is called asyncio.async, imported as ensure_future). -- pozdrawiam / best regards _.-._ Wojtek Porczyk .-^' '^-. Invisible Things Lab |'-.-^-.-'| | | | | I do not fear computers, | '-.-' | I fear lack of them. '-._ : ,-' -- Isaac Asimov `^-^-_>
On Tue, Sep 26, 2017 at 12:46:02AM +0200, Wojtek Porczyk wrote:
On Mon, Sep 25, 2017 at 02:43:56PM +0100, Daniel P. Berrange wrote:
On Thu, Aug 24, 2017 at 09:22:49PM +0200, Wojtek Porczyk wrote:
This logging is helpful for tracing problems with unclosed connections and leaking file descriptors.
Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com> --- libvirtaio.py | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/libvirtaio.py b/libvirtaio.py index 7c8c396..46de9ab 100644 --- a/libvirtaio.py +++ b/libvirtaio.py @@ -74,7 +74,7 @@ class Callback(object): def close(self): '''Schedule *ff* callback''' self.impl.log.debug('callback %d close(), scheduling ff', self.iden) - self.impl.schedule_ff_callback(self.opaque) + self.impl.schedule_ff_callback(self.iden, self.opaque)
# # file descriptors @@ -275,6 +275,10 @@ class virEventAsyncIOImpl(object): self.descriptors = DescriptorDict(self) self.log = logging.getLogger(self.__class__.__name__)
+ def __repr__(self): + return '<{} callbacks={} descriptors={}>'.format( + type(self).__name__, self.callbacks, self.descriptors) + def register(self): '''Register this instance as event loop implementation''' # pylint: disable=bad-whitespace @@ -284,9 +288,18 @@ class virEventAsyncIOImpl(object): self._add_timeout, self._update_timeout, self._remove_timeout) return self
- def schedule_ff_callback(self, opaque): + def schedule_ff_callback(self, iden, opaque): '''Schedule a ff callback from one of the handles or timers''' - self.loop.call_soon(libvirt.virEventInvokeFreeCallback, opaque) + ensure_future(self._ff_callback(iden, opaque), loop=self.loop)
This use of ensure_future puts a min python version of 3.4 on the code. Is there a strong reason to prefer that over the previous call_soon code ?
1) There is no technical reason whatsoever. This is an artifact from the previous iteration.
2) In fact ensure_future() does not make it incompatible with python<3.4 because of the try/except import at the beginning of the file (in python<3.4.3 the function is called asyncio.async, imported as ensure_future).
Ah ok, I missed that bit of existing code. That's fine then: Reviewed-by: Daniel P. Berrange <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Sep 22, 2017 at 05:54:04PM +0200, Wojtek Porczyk wrote:
Hi libvirt-list,
I'd like to submit a second iteration of libvirtaio series for 3.8.0. There are some improvements to have better control over closing connections, which are important in test suites which repeatedly open and close connections. Also there are some minor bugfixes and better logging.
There's something strange in this posting - the patches I received are all missing 'libvir-list@redhat.com' in the To: field - its almost like this was Bcc'd to the list which means anyone replying doesn't copy the list address on their reply. Even more odd though, I got a second copy of patch 3 which does have the To field set. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Sep 25, 2017 at 12:14:24PM +0100, Daniel P. Berrange wrote:
On Fri, Sep 22, 2017 at 05:54:04PM +0200, Wojtek Porczyk wrote:
Hi libvirt-list,
I'd like to submit a second iteration of libvirtaio series for 3.8.0. There are some improvements to have better control over closing connections, which are important in test suites which repeatedly open and close connections. Also there are some minor bugfixes and better logging.
There's something strange in this posting - the patches I received are all missing 'libvir-list@redhat.com' in the To: field - its almost like this was Bcc'd to the list which means anyone replying doesn't copy the list address on their reply. Even more odd though, I got a second copy of patch 3 which does have the To field set.
I'm sorry, this was my fault. I sent the whole series by [b]ouncing this in mutt, because it otherwise mangles Message-Id and In-Reply-To: fields. The previous posting of this series has this problem. I did it two times, since there was a delay in delivery (I don't know why, maybe greylisting) and I worried that the first batch was rejected for the lack of To: field, which I forgot at the time of git format-patch. Sorry again for the noise. -- pozdrawiam / best regards _.-._ Wojtek Porczyk .-^' '^-. Invisible Things Lab |'-.-^-.-'| | | | | I do not fear computers, | '-.-' | I fear lack of them. '-._ : ,-' -- Isaac Asimov `^-^-_>
On Tue, Sep 26, 2017 at 12:45:16AM +0200, Wojtek Porczyk wrote:
On Mon, Sep 25, 2017 at 12:14:24PM +0100, Daniel P. Berrange wrote:
On Fri, Sep 22, 2017 at 05:54:04PM +0200, Wojtek Porczyk wrote:
Hi libvirt-list,
I'd like to submit a second iteration of libvirtaio series for 3.8.0. There are some improvements to have better control over closing connections, which are important in test suites which repeatedly open and close connections. Also there are some minor bugfixes and better logging.
There's something strange in this posting - the patches I received are all missing 'libvir-list@redhat.com' in the To: field - its almost like this was Bcc'd to the list which means anyone replying doesn't copy the list address on their reply. Even more odd though, I got a second copy of patch 3 which does have the To field set.
I'm sorry, this was my fault. I sent the whole series by [b]ouncing this in mutt, because it otherwise mangles Message-Id and In-Reply-To: fields. The previous posting of this series has this problem. I did it two times, since there was a delay in delivery (I don't know why, maybe greylisting) and I worried that the first batch was rejected for the lack of To: field, which I forgot at the time of git format-patch.
The delay wasn't anything you did - the mail server was having significant delays processing mail at the latter half of last week, and thus randomly delaying stuff for people. FWIW, if possible with your local network/mail setup, for sending patches we recommend using 'git send-email' rather than doing it from a mail client since it gets threading right automagically. Anyway I've pushed this series to git master, so thanks for your continued contributions :-) Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (2)
-
Daniel P. Berrange -
Wojtek Porczyk