event handlers from extensions are called multiple times #1899

marmarek · Apr 8, 2016

It looks like every instantiate of an extension registers all the handlers, regardless of being already registered. And because handlers are registered on VM classes, not instances, they are global.

Example:

import qubes
import qubes.ext
import qubes.vm.appvm
class TestExt(qubes.ext.Extension):
    @qubes.ext.handler('domain-create-on-disk')
    def on_domain_create(self, vm, event, source_template):
        print "Event!"

app1 = qubes.Qubes()
app2 = qubes.Qubes()

vm = app1.add_new_vm(qubes.vm.appvm.AppVM(name='test-abc', label='red'))
vm.create_on_disk()

This will print "Event!" twice.

BTW How to register an extension just for tests? Is it possible to register entry point from outside of setup.py? @woju

On Fri, Apr 08, 2016 at 06:24:01AM -0700, Marek Marczykowski-Górecki wrote:

It looks like every instantiate of an extension registers all the
handlers, regardless of being already registered. And because handlers
are registered on VM classes, not instances, they are global.

You are not supposed to instantiate qubes.Qubes() twice in one
interpreter. If you're sure you need this (and I doubt it), the
alternative would be to register the extension's event handlers in
metaclass.

BTW How to register an extension just for tests? Is it possible to
register entry point from outside of setup.py? @woju

No, it's not possible. Just create small package with extension. You can
use sudo python setup.py install instead of proper packaging.

woju · Apr 8, 2016

On Fri, Apr 08, 2016 at 06:24:01AM -0700, Marek Marczykowski-Górecki wrote:

It looks like every instantiate of an extension registers all the
handlers, regardless of being already registered. And because handlers
are registered on VM classes, not instances, they are global.

You are not supposed to instantiate qubes.Qubes() twice in one
interpreter. If you're sure you need this (and I doubt it), the
alternative would be to register the extension's event handlers in
metaclass.

BTW How to register an extension just for tests? Is it possible to
register entry point from outside of setup.py? @woju

No, it's not possible. Just create small package with extension. You can
use sudo python setup.py install instead of proper packaging.

You are not supposed to instantiate qubes.Qubes() twice in one interpreter.

Why? How for example reload qubes.xml? Leaving alone other use cases (see below).

If you're sure you need this (and I doubt it)

Have you tried running tests?

marmarek · Apr 8, 2016

You are not supposed to instantiate qubes.Qubes() twice in one interpreter.

Why? How for example reload qubes.xml? Leaving alone other use cases (see below).

If you're sure you need this (and I doubt it)

Have you tried running tests?

On Fri, Apr 08, 2016 at 06:50:12AM -0700, Marek Marczykowski-Górecki wrote:

You are not supposed to instantiate qubes.Qubes() twice in one interpreter.

Why? How for example reload qubes.xml? Leaving alone other use cases (see below).

There should be a method for that. Maybe repurpose .load().

If you're sure you need this (and I doubt it)

Have you tried running tests?

I did and I haven't found anything suspicious around this issue (maybe
because it drowned among other errors).

But maybe you're right. It should be possible, if only to get default
template. Will look into it.

woju · Apr 8, 2016

On Fri, Apr 08, 2016 at 06:50:12AM -0700, Marek Marczykowski-Górecki wrote:

You are not supposed to instantiate qubes.Qubes() twice in one interpreter.

Why? How for example reload qubes.xml? Leaving alone other use cases (see below).

There should be a method for that. Maybe repurpose .load().

If you're sure you need this (and I doubt it)

Have you tried running tests?

I did and I haven't found anything suspicious around this issue (maybe
because it drowned among other errors).

But maybe you're right. It should be possible, if only to get default
template. Will look into it.

BTW I'm not talking about running handlers just twice. When tried appmenus tests (not yet pushed), it gets called 36 times! And probably only 36 times, because I started only small subset of tests.

No, it's not possible. Just create small package with extension. You can use sudo python setup.py install instead of proper packaging.

How to reliably uninstall such package then?

marmarek · Apr 8, 2016

BTW I'm not talking about running handlers just twice. When tried appmenus tests (not yet pushed), it gets called 36 times! And probably only 36 times, because I started only small subset of tests.

No, it's not possible. Just create small package with extension. You can use sudo python setup.py install instead of proper packaging.

How to reliably uninstall such package then?

On Fri, Apr 08, 2016 at 07:17:22AM -0700, Marek Marczykowski-Górecki wrote:

BTW I'm not talking about running handlers just twice. When tried
appmenus tests (not yet pushed), it gets called 36 times! And probably
only 36 times, because I started only small subset of tests.

Ah. Probably one instantiation per vms cleanup.

No, it's not possible. Just create small package with extension. You
can use sudo python setup.py install instead of proper packaging.

How to reliably uninstall such package then?

There is python setup.py develop which has -u. Didn't use that though.
Also, there is egg_info and install_egg_info, which register the
entry_points. However those also don't support uninstall.

In any case, to manually remove package, just remove
/usr/lib/python2.7/site-packages/ and
-.egg-info, the latter containing entry_points.txt.
There are however many cases, for example some of the packages gets
installed to lib64 if they contain compiled C modules. Sometimes they
are installed to /usr/local, in debian the directory is dist-packages
not site-packages etc. Last but not least, with --user package gets
installed to $HOME/.local/lib/python/site-packages. More
complicated eggs can also install bits everywhere, from /usr/share to
/etc.

woju · Apr 8, 2016

On Fri, Apr 08, 2016 at 07:17:22AM -0700, Marek Marczykowski-Górecki wrote:

BTW I'm not talking about running handlers just twice. When tried
appmenus tests (not yet pushed), it gets called 36 times! And probably
only 36 times, because I started only small subset of tests.

Ah. Probably one instantiation per vms cleanup.

No, it's not possible. Just create small package with extension. You
can use sudo python setup.py install instead of proper packaging.

How to reliably uninstall such package then?

There is python setup.py develop which has -u. Didn't use that though.
Also, there is egg_info and install_egg_info, which register the
entry_points. However those also don't support uninstall.

In any case, to manually remove package, just remove
/usr/lib/python2.7/site-packages/ and
-.egg-info, the latter containing entry_points.txt.
There are however many cases, for example some of the packages gets
installed to lib64 if they contain compiled C modules. Sometimes they
are installed to /usr/local, in debian the directory is dist-packages
not site-packages etc. Last but not least, with --user package gets
installed to $HOME/.local/lib/python/site-packages. More
complicated eggs can also install bits everywhere, from /usr/share to
/etc.

There is python setup.py develop which has -u. Didn't use that though.
Also, there is egg_info and install_egg_info, which register the
entry_points. However those also don't support uninstall.

Is it possible to install it in some temporary directory, add it to PYTHONPATH (or something), then remove the directory entirely at cleanup?

marmarek · Apr 8, 2016

There is python setup.py develop which has -u. Didn't use that though.
Also, there is egg_info and install_egg_info, which register the
entry_points. However those also don't support uninstall.

Is it possible to install it in some temporary directory, add it to PYTHONPATH (or something), then remove the directory entirely at cleanup?

Or maybe some other solution - register extension normally, but override __init__ to do nothing outside of tests? What about such idea?

marmarek · Apr 8, 2016

Or maybe some other solution - register extension normally, but override __init__ to do nothing outside of tests? What about such idea?

On Fri, Apr 08, 2016 at 08:03:41AM -0700, Marek Marczykowski-Górecki wrote:

Or maybe some other solution - register extension normally, but
override __init__ to do nothing outside of tests? What about such
idea?

What does this extension do?

woju · Apr 8, 2016

On Fri, Apr 08, 2016 at 08:03:41AM -0700, Marek Marczykowski-Górecki wrote:

Or maybe some other solution - register extension normally, but
override __init__ to do nothing outside of tests? What about such
idea?

What does this extension do?

Will test whether extensions are working at all ;)

marmarek · Apr 8, 2016

Will test whether extensions are working at all ;)

On Fri, Apr 08, 2016 at 08:21:53AM -0700, Marek Marczykowski-Górecki wrote:

Will test whether extensions are working at all ;)

I see no point. It's hard to do it such way that built-in extensions do
work and off-package extension won't. That would be bug in pkg_resources
and not ours.

woju · Apr 8, 2016

On Fri, Apr 08, 2016 at 08:21:53AM -0700, Marek Marczykowski-Górecki wrote:

Will test whether extensions are working at all ;)

I see no point. It's hard to do it such way that built-in extensions do
work and off-package extension won't. That would be bug in pkg_resources
and not ours.

Ah, indeed it is enough to instantiate extension object, not necessarily from inside of Qubes.init.

I had other use cases for that too - like register test VM classes (so tools called in different processes, like qrexec calls will be able to use them). But for now it looks like too deep change just for tests. And probably not needed in practice.

So, EOT for this offtopic :)

Now, back to the original problem - how registering extensions in metaclass will solve this? Will it be done once?

marmarek · Apr 8, 2016

Ah, indeed it is enough to instantiate extension object, not necessarily from inside of Qubes.init.

I had other use cases for that too - like register test VM classes (so tools called in different processes, like qrexec calls will be able to use them). But for now it looks like too deep change just for tests. And probably not needed in practice.

So, EOT for this offtopic :)

Now, back to the original problem - how registering extensions in metaclass will solve this? Will it be done once?

On Fri, Apr 08, 2016 at 04:36:39PM -0700, Marek Marczykowski-Górecki wrote:

Ah, indeed it is enough to instantiate extension object, not necessarily from inside of Qubes.init.

I had other use cases for that too - like register test VM classes (so tools called in different processes, like qrexec calls will be able to use them). But for now it looks like too deep change just for tests. And probably not needed in practice.

So, EOT for this offtopic :)

Now, back to the original problem - how registering extensions in metaclass will solve this? Will it be done once?

Yes. The metaclass is called when the class itself is defined, not when
it is instantiated. That would be hard for two reasons:

There will be cyclic imports. If done now, any extension which would
dare to "import qubes" would not work, because the definition of the
"qubes" module contains the definition of qubes.Qubes, which would
require importing extension.
It will be hack nonetheless, because the metaclass will also be
called for classes that derive from qubes.Qubes. There are none at the
moment, however it would be natural for someone who would like to extend
Qubes a little more than extensions allow.

woju · Apr 9, 2016

On Fri, Apr 08, 2016 at 04:36:39PM -0700, Marek Marczykowski-Górecki wrote:

Ah, indeed it is enough to instantiate extension object, not necessarily from inside of Qubes.init.

I had other use cases for that too - like register test VM classes (so tools called in different processes, like qrexec calls will be able to use them). But for now it looks like too deep change just for tests. And probably not needed in practice.

So, EOT for this offtopic :)

Now, back to the original problem - how registering extensions in metaclass will solve this? Will it be done once?

Yes. The metaclass is called when the class itself is defined, not when
it is instantiated. That would be hard for two reasons:

There will be cyclic imports. If done now, any extension which would
dare to "import qubes" would not work, because the definition of the
"qubes" module contains the definition of qubes.Qubes, which would
require importing extension.
It will be hack nonetheless, because the metaclass will also be
called for classes that derive from qubes.Qubes. There are none at the
moment, however it would be natural for someone who would like to extend
Qubes a little more than extensions allow.

So, maybe better register handlers on objects, not classes? Or keep all the handlers from extensions in Qubes() object (as a dict VM class -> handlers). This will be more consistent with how global handlers are registered: https://github.com/woju/qubes-core-admin/blob/core3-devel/qubes/ext/__init__.py#L49-L55

marmarek · Apr 9, 2016

So, maybe better register handlers on objects, not classes? Or keep all the handlers from extensions in Qubes() object (as a dict VM class -> handlers). This will be more consistent with how global handlers are registered: https://github.com/woju/qubes-core-admin/blob/core3-devel/qubes/ext/__init__.py#L49-L55

On Sat, Apr 09, 2016 at 03:15:12AM -0700, Marek Marczykowski-Górecki wrote:

So, maybe better register handlers on objects, not classes? Or keep
all the handlers from extensions in Qubes() object (as a dict VM
class -> handlers). This will be more consistent with how global
handlers are registered:
https://github.com/woju/qubes-core-admin/blob/core3-devel/qubes/ext/__init__.py#L49-L55

Don't want to do that, because you'd have to manage handlers on vms,
which may come and go. I think it would be better to have extensions as
singletons, without direct access to app object. You could still access
it temporarily in handlers, because you get the vm, which has vm.app
attribute.

woju · Apr 9, 2016

On Sat, Apr 09, 2016 at 03:15:12AM -0700, Marek Marczykowski-Górecki wrote:

So, maybe better register handlers on objects, not classes? Or keep
all the handlers from extensions in Qubes() object (as a dict VM
class -> handlers). This will be more consistent with how global
handlers are registered:
https://github.com/woju/qubes-core-admin/blob/core3-devel/qubes/ext/__init__.py#L49-L55

Don't want to do that, because you'd have to manage handlers on vms,
which may come and go. I think it would be better to have extensions as
singletons, without direct access to app object. You could still access
it temporarily in handlers, because you get the vm, which has vm.app
attribute.

This means extension shouldn't keep any state internally. Currently none of them does, so maybe not a problem. But it should be clearly noted in documentation. Keeping state in VM objects (for example as features) is ok.

How about global handlers then? qubes.Qubes.add_handler?

marmarek · Apr 9, 2016

This means extension shouldn't keep any state internally. Currently none of them does, so maybe not a problem. But it should be clearly noted in documentation. Keeping state in VM objects (for example as features) is ok.

How about global handlers then? qubes.Qubes.add_handler?

On Sat, Apr 09, 2016 at 03:26:02AM -0700, Marek Marczykowski-Górecki wrote:

This means extension shouldn't keep any state internally. Currently
none of them does, so maybe not a problem. But it should be clearly
noted in documentation. Keeping state in VM objects (for example as
features) is ok.

In principle there is nothing that stops you from keeping some
"superglobal" state in extension. You only have to know that it you may
encounter vms from different apps. That surely has to be documented. And
yes, features would be the preferred way to store state.

How about global handlers then? qubes.Qubes.add_handler?

This would cease to be an issue. I would be be done once, during the
first and only instantiation of the extension.

woju · Apr 9, 2016

On Sat, Apr 09, 2016 at 03:26:02AM -0700, Marek Marczykowski-Górecki wrote:

This means extension shouldn't keep any state internally. Currently
none of them does, so maybe not a problem. But it should be clearly
noted in documentation. Keeping state in VM objects (for example as
features) is ok.

In principle there is nothing that stops you from keeping some
"superglobal" state in extension. You only have to know that it you may
encounter vms from different apps. That surely has to be documented. And
yes, features would be the preferred way to store state.

How about global handlers then? qubes.Qubes.add_handler?

This would cease to be an issue. I would be be done once, during the
first and only instantiation of the extension.

Ok.

marmarek · Apr 9, 2016

Ok.

woju/qubes-core-admin@540942d

marmarek · May 4, 2016

woju/qubes-core-admin@540942d

marmarek assigned woju Apr 8, 2016

marmarek added bug C: core P: critical task labels Apr 8, 2016

marmarek added this to the Release 4.0 milestone Apr 8, 2016

marmarek closed this May 4, 2016

QubesOS/qubes-issues

Join GitHub today

event handlers from extensions are called multiple times #1899

Comments

marmarek commented Apr 8, 2016

marmarek assigned woju Apr 8, 2016

marmarek added bug C: core P: critical task labels Apr 8, 2016

marmarek added this to the Release 4.0 milestone Apr 8, 2016

This comment has been minimized.

woju commented Apr 8, 2016

This comment has been minimized.

marmarek commented Apr 8, 2016

This comment has been minimized.

woju commented Apr 8, 2016

This comment has been minimized.

marmarek commented Apr 8, 2016

This comment has been minimized.

woju commented Apr 8, 2016

This comment has been minimized.

marmarek commented Apr 8, 2016

This comment has been minimized.

marmarek commented Apr 8, 2016

This comment has been minimized.

woju commented Apr 8, 2016

This comment has been minimized.

marmarek commented Apr 8, 2016

This comment has been minimized.

woju commented Apr 8, 2016

This comment has been minimized.

marmarek commented Apr 8, 2016

This comment has been minimized.

woju commented Apr 9, 2016

This comment has been minimized.

marmarek commented Apr 9, 2016

This comment has been minimized.

woju commented Apr 9, 2016

This comment has been minimized.

marmarek commented Apr 9, 2016

This comment has been minimized.

woju commented Apr 9, 2016

This comment has been minimized.

marmarek commented Apr 9, 2016

This comment has been minimized.

marmarek commented May 4, 2016

marmarek closed this May 4, 2016