Created attachment 61007 [details] [review]
patch that adds support for python-based main loops

* fixed base MainLoop (use NotImplementedError exception instead of NotImplemented)

* add "flags" argument to remove_watch because watchers can refer to the same fd, but with different flags; we need to know what we should remove - readable, writable or both

* add twisted main loop (dbus.mainloop.python.twisted)

Comment on attachment 61007 [details] [review]
patch that adds support for python-based main loops

Review of attachment 61007 [details] [review]:
-----------------------------------------------------------------

Generally looks good, with some design work needed around the relationship between MainLoop and NativeMainLoop, and a few minor details.

::: Makefile.am
@@ +32,4 @@
>      dbus/lowlevel.py \
>      dbus/mainloop/__init__.py \
>      dbus/mainloop/glib.py \
> +    dbus/mainloop/python/__init__.py \

I don't think the extra level of hierarchy is worthwhile. I'd just do:

dbus/mainloop/__init__.py     # includes your "base"
dbus/mainloop/simple.py       # your select and/or poll
dbus/mainloop/twisted.py

@@ +34,5 @@
>      dbus/mainloop/glib.py \
> +    dbus/mainloop/python/__init__.py \
> +    dbus/mainloop/python/base.py \
> +    dbus/mainloop/python/select.py \
> +    dbus/mainloop/python/poll.py \

I don't think distinguishing between select and poll makes sense.

If you're integrating with a third-party main loop (GLib or Twisted), it can matter which one you use, because everything else in your application has to integrate with that same main loop.

However, if you're using a main loop supplied by dbus-python, you're effectively saying "I only care about D-Bus, and anything else that explicitly registers with the dbus-python main loop" - so apart from performance, it doesn't really matter whether, behind the scenes, dbus-python is using select or poll or epoll or (...).

So, I'd prefer to merge select and poll into a dbus.mainloop.simple which uses whatever it can find (poll, or select if poll is unavailable). It could be extended to support epoll too in a later version, if anyone cares enough.

::: _dbus_bindings/dbus_bindings-internal.h
@@ +224,5 @@
>  
> +/* util.c */
> +extern int dbus_py_socketpair(int fds[2]);
> +extern ssize_t dbus_py_send(int sockfd, const void *buf, size_t len, int flags);
> +extern ssize_t dbus_py_recv(int sockfd, void *buf, size_t len, int flags);

ssize_t isn't fully portable, please use Py_ssize_t.

::: _dbus_bindings/mainloop-python.c
@@ +1,1 @@
> +#include "config.h"

Please add a copyright/license notice at the top of each file (as in, for instance, bus.c) with yourself or your employer, whichever is applicable, as the copyright holder.

@@ +5,5 @@
> +typedef struct {
> +    PyObject_HEAD
> +    PyObject * (*callback)(PyObject *, PyObject *, PyObject *);
> +    void *data;
> +} Callback;

I can't help thinking you shouldn't have to reinvent C closures... but I can't find anything like this in the Python documentation, so never mind.

@@ +39,5 @@
> +    0,                                  /*tp_doc*/
> +};
> +
> +static PyObject *
> +DBusPyCallback_New2(PyObject * (*callback)(PyObject *, PyObject *, PyObject *),

Because this is static, it doesn't really need a namespaced name - it could just be Callback_New. The DBusPy thing is just for things that are part of the C API for dbus-python (e.g. are used by the dbus-glib and Qt main loop glue).

@@ +58,5 @@
> +    int socketpair[2];
> +} ConnectionSetup;
> +
> +static ConnectionSetup *
> +ConnectionSetup_init(DBusConnection *conn, PyObject *loop)

This returns the ConnectionSetup, so I'd prefer it called ConnectionSetup_new. "init" should initialize a preallocated thing.

@@ +66,5 @@
> +    int socketpair_result = -1;
> +
> +    cs = dbus_new0(ConnectionSetup, 1);
> +    if (!cs) {
> +        PyErr_SetString(PyExc_RuntimeError, "Failed to allocate memory for ConnectionSetup");

MemoryError, surely?

@@ +121,5 @@
> +ConnectionSetup_free(ConnectionSetup *cs)
> +{
> +    if (cs->connection) {
> +        /* only DBusConnection has allocated
> +         * socketpair so clean it up

Thanks for documenting the assumption, but I'd prefer to compare socketpair[n] with -1 instead.

@@ +170,5 @@
> +        result = FALSE;
> +        goto out;
> +    }
> +
> +    if (!PyObject_CallMethod(cs->mainloop, "add_watch", "iIN", unix_fd, flags, callback)) {

You leak whatever PyObject_CallMethod returns on success (usually None, but you never know). Instead, use something like this:

    tmp = PyObject_CallMethod(cs->mainloop, "add_watch", "iIO", unix_fd, flags, callback);
    result = (tmp != NULL);
    Py_CLEAR(tmp);

It should also PyErr_Warn and PyErr_Clear (to do something with the exception and then clear the global exception indicator), I think.

This should use the format letter "O" instead of "N" for callback so that callback gets reffed...

@@ +177,5 @@
> +    }
> +
> +out:
> +    if (!result) {
> +        Py_CLEAR(callback);

... and then this could just "Py_CLEAR(callback)" unconditionally.

This would avoid a memory leak that you currently have: you don't free callback if DBusPyCallback_New2 succeeds but then PyObject_CallMethod fails.

@@ +192,5 @@
> +    ConnectionSetup *cs = data;
> +
> +    int unix_fd = dbus_watch_get_unix_fd(watch);
> +    unsigned int flags = dbus_watch_get_flags(watch);
> +    PyObject_CallMethod(cs->mainloop, "remove_watch", "iI", unix_fd, flags);

Again, this leaks the result of PyObject_CallMethod. To ignore errors entirely, you'd use:

    /* ignore any error */
    Py_XDECREF (PyObject_CallMethod(...));
    PyErr_Clear();

but I'd prefer a warning (via PyErr_Warn() or something) if the method raises an exception.

@@ +243,5 @@
> +        result = FALSE;
> +        goto out;
> +    }
> +
> +    id = PyObject_CallMethod(cs->mainloop, "add_timeout", "iN", interval, callback);

Again, use "O" and make the Py_CLEAR unconditional, so that it's non-leaky.

@@ +246,5 @@
> +
> +    id = PyObject_CallMethod(cs->mainloop, "add_timeout", "iN", interval, callback);
> +    if (!id) {
> +        result = FALSE;
> +        goto out;

Again, this deserves a PyErr_Warn() and PyErr_Clear(), I think.

@@ +269,5 @@
> +
> +    ConnectionSetup *cs = data;
> +    PyObject *id = dbus_timeout_get_data(timeout);
> +
> +    PyObject_CallMethod(cs->mainloop, "remove_timeout", "O", id);

Also leaks the returned value, and ignores any exception.

@@ +304,5 @@
> +    ConnectionSetup *cs = callback->data;
> +    char flag;
> +
> +    Py_BEGIN_ALLOW_THREADS
> +    while (dbus_py_recv(cs->socketpair[0], &flag, 1, 0) == 1);

I'm not a fan of this subtle use of semicolons, I'd prefer:

while (...) {
    /* do nothing */
}

(and the same further down).

@@ +334,5 @@
> +
> +    dbus_connection_allocate_data_slot(&dbus_py_mainloop_connection_data_slot);
> +    if (dbus_py_mainloop_connection_data_slot < 0) {
> +        PyErr_SetString(PyExc_RuntimeError,
> +                        "Failed to allocate connection data slot");

MemoryError (the only reason that function can fail is documented to be OOM)

@@ +344,5 @@
> +                                  cs,
> +                                  (DBusFreeFunction)ConnectionSetup_free))
> +    {
> +        PyErr_SetString(PyExc_RuntimeError,
> +                        "Failed to set connection data slot");

MemoryError

@@ +380,5 @@
> +    if (!callback) {
> +        goto fail;
> +    }
> +
> +    if (!PyObject_CallMethod(data, "add_watch", "iIN", cs->socketpair[0], DBUS_WATCH_READABLE, callback)) {

Should use "O" for callback, and the result is leaked.

@@ +386,5 @@
> +    }
> +
> +    /* force dbus connection to change its status to COMPLETE */
> +    Py_BEGIN_ALLOW_THREADS
> +    while (dbus_connection_dispatch(connection) == DBUS_DISPATCH_DATA_REMAINS);

Again, {} would be clearer.

@@ +406,5 @@
> +                                         dbus_py_mainloop_remove_watch,
> +                                         dbus_py_mainloop_toggle_watch,
> +                                         data,
> +                                         NULL)) {
> +        PyErr_SetString(PyExc_RuntimeError,

MemoryError

@@ +417,5 @@
> +                                           dbus_py_mainloop_remove_timeout,
> +                                           dbus_py_mainloop_toggle_timeout,
> +                                           data,
> +                                           NULL)) {
> +        PyErr_SetString(PyExc_RuntimeError,

MemoryError

@@ +432,5 @@
> +    Py_CLEAR(data);
> +}
> +
> +static PyObject *
> +dbus_py_mainloop_init(PyObject *self, PyObject *args, PyObject *kw)

I'd call this _native_main_loop_from_python or something. (It's a module-level function, really, right?)

@@ +444,5 @@
> +
> +    native_mainloop = DBusPyNativeMainLoop_New4(dbus_py_mainloop_set_up_connection,
> +                                                 dbus_py_mainloop_set_up_server,
> +                                                 dbus_py_mainloop_free,
> +                                                 python_mainloop);

Interesting... I didn't think NativeMainLoop would be suitable for use from Python like this.

I can see the appeal of having NativeMainLoop and PythonMainLoop be the same thing, but I think you may actually be making life harder for yourself by doing that. I think it might be clearer if PythonMainLoop was an independent base class, with subclasses in Python. Then dbus_py_set_up_connection() and dbus_py_set_up_server() could just look like this:

    if (NativeMainLoop_Check(...)) {
        ...
    }
    else if (PythonMainLoop_Check(...)) {
        ...
    }
    else {
        PyErr_SetString (..., "A dbus.mainloop.MainLoop or dbus.mainloop.NativeMainLoop instance is required);
        ...
    }

Having this class hierarchy:

    _dbus_bindings.PythonMainLoop
        dbus.mainloop.MainLoop
            dbus.mainloop.Twisted.TwistedMainLoop etc.

would still let you introduce the NotImplementedError-raising virtual methods in MainLoop.

Alternatively, if d.m.M can be a subclass of NativeMainLoop, that'd work too.

@@ +470,5 @@
> +{
> +    /* the factory function that wraps python main loop into a native one */
> +    PyObject *PythonMainLoop = NULL;
> +
> +    PythonMainLoop = DBusPyCallback_New2(dbus_py_mainloop_init,

This seems pretty weird - if you want it to be a function, just make it a function (via a PyMethodDef like in _dbus_bindings/module.c).

I'd call it something like _dbus_bindings._native_main_loop_from_python.

::: _dbus_bindings/util.c
@@ +20,5 @@
> +}
> +
> +/*
> + * the idea and some part of this code
> + * has been taken from Perl's util.c

Under what copyright and license?

If you're only sending things one-way, on Unix, you don't need a socket pair anyway - just use a pipe.

If you're aiming to be portable to Windows, then yes this is the only thing that can work.

::: dbus/mainloop/__init__.py
@@ +27,4 @@
>  import _dbus_bindings
>  
>  NativeMainLoop = _dbus_bindings.NativeMainLoop
> +PythonMainLoop = _dbus_bindings.PythonMainLoop

Is it actually useful for this to be present here? I don't think it is - no user code should ever be using it, since library users should go via dbus.mainloop(.python.base).MainLoop anyway.

::: dbus/mainloop/python/base.py
@@ +1,1 @@
> +# dbus

Copyright and license, again.

I'd be inclined to put MainLoop directly in dbus.mainloop rather than having a "base" module.

@@ +2,5 @@
> +import _dbus_bindings
> +
> +class MainLoop(object):
> +    @classmethod
> +    def install(klas, *args, **kwargs):

The dbus-glib and QtDBus main loops don't have this method. Instead, they have this factory function:

    SomeMainLoop(set_as_default=False) -> NativeMainLoop

Please stay consistent with those.

One way to do this would be for MainLoop to be a subclass of NativeMainLoop, which calls a factory function from _dbus_bindings in its __new__ instead of chaining up, something like this:

    class MainLoop(NativeMainLoop):
        def __new__(cls, set_as_default=False):
            self = cls._new_for_python()
            if set_as_default:
                _dbus_bindings.set_default_main_loop(self)

        # ... add_watch, etc. as before

In that version, your PythonMainLoop factory-callable would become _new_for_python, a class-method somewhat similar to DBusPyConnection_NewForBus.

Another (which is what I had intended would happen) would be for MainLoop to be an entirely separate class, implemented in C but subclassable in Python. At the moment you're jumping through quite a few hoops to make the Python main loop conform to the expected API for the C NativeMainLoop.

Finally, you could do something like this:

    class MainLoop(object):
        def __init__(self, set_as_default=False):
            self._mainloop = _dbus_bindings._native_main_loop_from_python(self)
            if set_as_default:
                _dbus_bindings.set_default_main_loop(self._mainloop)

        # ... add_watch, etc. as before

and special-case dbus_py_set_up_connection and dbus_py_set_up_server so that if passed a dbus.mainloop.MainLoop, they get its _mainloop member and use that. That seems pretty awkward, though.

@@ +20,5 @@
> +
> +    def remove_timeout(self, id):
> +        raise NotImplementedError
> +
> +    def start(self):

I'd prefer this to be called run() like in GLib, since the name "start" is pretty misleading - I'd expect a "start" method to do something quickly and then return. (For instance, a "run the main loop in another thread" class could have a "start" method, but this "run the main loop in this thread" method shouldn't.)

@@ +23,5 @@
> +
> +    def start(self):
> +        raise NotImplementedError
> +
> +    def stop(self):

I'd prefer this to be called quit() like in GLib.

::: dbus/mainloop/python/select.py
@@ +46,5 @@
> +
> +    def stop(self):
> +        self.running = False
> +
> +    def tick(self):

I think this method should be private (_tick).

::: dbus/mainloop/python/twisted.py
@@ +1,1 @@
> +# future (to load real twisted first)

(Not reviewed yet)

(In reply to comment #0)
> import dbus
> from dbus.mainloop import PythonMainLoop
> from dbus.mainloop.python.select import SelectMainLoop
> 
> mainloop = SelectMainLoop()
> wrapper  = PythonMainLoop(mainloop)
> 
> bus = dbus.SystemBus(mainloop=wrapper)

I don't think this is the right API - you should just be able to instantiate a SelectMainLoop and pass it to the dbus.SystemBus constructor.

Created attachment 61314 [details] [review]
patch that adds support for python-based main loops

* fix memory leaks

* fix coding style

* add copyright and license headers

* get rid of socketpair in favor of pipe2 (we don't want to focus on Windows at the moment)

* rename Callback to CClosure (thanks to Simon, i like this name more than older one)

* move everything from dbus.mainloop.python.* to dbus.mainloop.*

* get rid of PythonMainLoop wrapper and turn it into the base class for dbus.mainloop.base.MainLoop

* remove MainLoop.install and use "set_as_default" constructor's (__init__) argument instead (the default value is False)

* dbus.mainloop.twisted: import the reactor if it's not yet (that can save you few lines of code if you're too lazy to import it on your own)

- i still want to keep every main loop implementation separately (as well as the base class) because this way is much clearer and easier to maintain based on my experience

- i still import PythonMainLoop into the dbus.mainloop package namespace even if it seems useless just because there is NativeMainLoop and NULL_MAIN_LOOP imports already that are useless too. the point is either we remove all of them or we keep the code looks the same

Comment on attachment 61314 [details] [review]
patch that adds support for python-based main loops

Review of attachment 61314 [details] [review]:
-----------------------------------------------------------------

This is looking pretty good. I'll try to find time to make some of these changes myself.

::: _dbus_bindings/mainloop-python.c
@@ +34,5 @@
> +
> +typedef struct {
> +    PyObject_HEAD
> +    PyObject * (*function)(PyObject *, PyObject *, PyObject *);
> +    void *data;

The way you're using this void* (fishing it out of self in the callbacks) is pretty unusual.

I think I'd prefer function() to have this more-conventional signature:

    PyObject *(*function)(PyObject *args, PyObject *kwargs, void *data);

@@ +99,5 @@
> +
> +    cs = dbus_new0(ConnectionSetup, 1);
> +    if (!cs) {
> +        PyErr_SetString(PyExc_MemoryError, "Failed to allocate memory for ConnectionSetup");
> +        return NULL;

You can replace these two lines with:

    return PyErr_NoMemory();

@@ +115,5 @@
> +        return cs;
> +    }
> +
> +    Py_BEGIN_ALLOW_THREADS
> +    pipe2_result = pipe2(cs->pipefd, O_NONBLOCK | O_CLOEXEC);

This is Linux-specific; there should be a generic fallback using pipe() and fcntl().

I'd be tempted to factor it out into a dbus_py_pipe function, something like this:

static int
dbus_py_pipe(int fds[2])
{
    int res;

#ifdef HAVE_PIPE2
    res = pipe2(fds, O_NONBLOCK | O_CLOEXEC);

    if (res == 0)
        return 0;
#endif
    /* either libc doesn't have pipe2() or the kernel didn't support it */

    if (pipe(fds) != 0)
        return -1;

    if (fcntl (fds[0], F_SETFL, fcntl (fds[0], F_GETFL) | O_NONBLOCK) != 0 ||
        fcntl (fds[1], F_SETFL, fcntl (fds[1], F_GETFL) | O_NONBLOCK) != 0)
        goto close_them;

    if (fcntl (fds[0], F_SETFD, fcntl (fds[0], F_GETFD) | FD_CLOEXEC) != 0 ||
        fcntl (fds[1], F_SETFD, fcntl (fds[1], F_GETFD) | FD_CLOEXEC) != 0)
        goto close_them;

    return 0;

close_them:
    close(fds[0]);
    close(fds[1]);
    return -1;
}

@@ +119,5 @@
> +    pipe2_result = pipe2(cs->pipefd, O_NONBLOCK | O_CLOEXEC);
> +    Py_END_ALLOW_THREADS
> +
> +    if (pipe2_result == -1) {
> +        PyErr_SetFromErrno(PyExc_RuntimeError);

PyExc_OSError, surely?

@@ +156,5 @@
> +static void
> +ConnectionSetup_free(ConnectionSetup *cs)
> +{
> +    PyGILState_STATE gil = PyGILState_Ensure();
> +    {

Nice use of {} for a scope.

@@ +162,5 @@
> +
> +        if (cs->connection) {
> +            tmp = PyObject_CallMethod(cs->mainloop, "remove_watch", "iI", cs->pipefd[0], DBUS_WATCH_READABLE);
> +
> +            /* ignore any error */

Shouldn't this at least complain to stderr, rather than swallowing the error silently? That's what other native <-> Python bits do if they can't do anything more useful.

if (PyErr_Occurred()) {
    PyErr_Print();
}
Py_CLEAR(tmp);

@@ +246,5 @@
> +    PyObject *tmp = NULL;
> +
> +    tmp = PyObject_CallMethod(cs->mainloop, "remove_watch", "iI", unix_fd, flags);
> +
> +    /* ignore any error */

As above

@@ +304,5 @@
> +        result = FALSE;
> +        goto out;
> +    }
> +
> +    dbus_timeout_set_data(timeout, id, NULL);

I think this leaks the id. You probably want "take GIL and unref" as a free function (I think I already implemented that somewhere else in dbus-python).

@@ +324,5 @@
> +    PyObject *tmp = NULL;
> +
> +    tmp = PyObject_CallMethod(cs->mainloop, "remove_timeout", "O", id);
> +
> +    /* ignore any error */

As above

@@ +351,5 @@
> +    ConnectionSetup *cs = data;
> +    int count = -1;
> +
> +    if (status == DBUS_DISPATCH_DATA_REMAINS) {
> +        count = write(cs->pipefd[1], "\0", 1);

This will trigger gcc warnings ("variable set but not used") in some versions.

This would be better:

    if (write(cs->pipefd[1], "\0", 1) < 1) {
        /* ignore: if the pipe buffer is full, the dispatch function
         * will run soon anyway */
    }

@@ +363,5 @@
> +    ConnectionSetup *cs = cclosure->data;
> +    char flag;
> +
> +    Py_BEGIN_ALLOW_THREADS
> +    while (read(cs->pipefd[0], &flag, 1) == 1) {

This would make fewer syscalls if it drained the buffer faster:

    char buf[4096];

    while (read(cs->pipefd[0], buf, sizeof (buf)) > 0) {
        /* do nothing */
    }

@@ +396,5 @@
> +
> +    dbus_connection_allocate_data_slot(&dbus_py_mainloop_connection_data_slot);
> +    if (dbus_py_mainloop_connection_data_slot < 0) {
> +        PyErr_SetString(PyExc_MemoryError,
> +                        "Failed to allocate connection data slot");

PyErr_NoMemory()

@@ +406,5 @@
> +                                  cs,
> +                                  (DBusFreeFunction)ConnectionSetup_free))
> +    {
> +        PyErr_SetString(PyExc_MemoryError,
> +                        "Failed to set connection data slot");

PyErr_NoMemory()

@@ +488,5 @@
> +                                         dbus_py_mainloop_toggle_watch,
> +                                         cs,
> +                                         NULL)) {
> +        PyErr_SetString(PyExc_RuntimeError,
> +                        "Failed to set up server watch functions");

PyErr_NoMemory()

@@ +519,5 @@
> +    if (PyType_Ready(&CClosureType) < 0) {
> +        return 0;
> +    }
> +
> +    return 1;

return TRUE;

@@ +525,5 @@
> +
> +dbus_bool_t
> +dbus_py_insert_mainloop_python_types(PyObject *this_module)
> +{
> +    return 1;

return TRUE;

::: _dbus_bindings/mainloop.c
@@ +183,5 @@
>          return (nml->set_up_connection_cb)(dbc, nml->data);
>      }
> +
> +    if (PythonMainLoop_Check(mainloop)) {
> +        /* Native mainloops are allowed to do arbitrary strange things */

This comment is not applicable.

@@ +214,2 @@
>      PyErr_SetString(PyExc_TypeError,
> +                    "A dbus.mainloop.NativeMainLoop or dbus.mainloop.PythonMainLoop instance is required");

I feel as though this ought to be claiming that we require a dbus.mainloop.MainLoop - if we also accept a PythonMainLoop then that's an implementation detail, nobody should be using PythonMainLoop directly.

::: dbus/mainloop/base.py
@@ +24,5 @@
> +# dbus
> +import dbus
> +import _dbus_bindings
> +
> +class MainLoop(_dbus_bindings.PythonMainLoop):

Wouldn't this be better placed directly in dbus.mainloop?

@@ +31,5 @@
> +
> +        if set_as_default:
> +            dbus.set_default_main_loop(self)
> +
> +    def add_watch(self, fd, flags, callback):

This API assumes that the same fd will never be watched twice with the same flags. I'm not sure whether that's actually guaranteed :-( Perhaps it should be in terms of a Watch object with get_flags(), get_socket(), get_unix_fd() and handle() methods?

It also assumes that the distinction between "unix fd" and "socket" is never relevant, which isn't true on Windows. Even if dbus-python doesn't actually work on Windows right now, I feel that the API shouldn't rule it out.

@@ +37,5 @@
> +
> +    def remove_watch(self, fd, flags):
> +        raise NotImplementedError
> +
> +    def add_timeout(self, interval, callback):

Unlike the watch API, this one is OK for multiple "identical" timeouts, so I don't think we need a Timeout object. (The CClosure takes on that role, sort of...)

::: dbus/mainloop/poll.py
@@ +31,5 @@
> +# dbus
> +from dbus.mainloop import WATCH_READABLE, WATCH_WRITABLE
> +from dbus.mainloop.select import SelectMainLoop
> +
> +class PollMainLoop(SelectMainLoop):

I still think these two should be the same class - dbus.mainloop.simple.SimpleMainLoop or something - with whether it uses poll(), select() or even epoll() being an implementation detail.

::: examples/example-mainloop-python.py
@@ +9,5 @@
> +    if owner:
> +        print 'name is owned: %s' % owner
> +        mainloop.stop()
> +
> +bus = dbus.SystemBus()

I'd prefer new examples to use the session bus, so they can be used unprivileged.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus-python/issues/24.