After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 699132 - Pluggable event loop backends (epoll support)
Pluggable event loop backends (epoll support)
Status: RESOLVED OBSOLETE
Product: glib
Classification: Platform
Component: mainloop
unspecified
Other All
: Normal enhancement
: ---
Assigned To: gtkdev
gtkdev
: 156048 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-04-28 13:08 UTC by Mikhail Zabaluev
Modified: 2018-05-24 15:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gdbus: Prevent polling on a closed DBus connection (1.28 KB, patch)
2014-01-24 20:35 UTC, Mikhail Zabaluev
reviewed Details | Review
Pluggable backends for GMainLoop (78.97 KB, patch)
2014-01-24 20:35 UTC, Mikhail Zabaluev
none Details | Review
Added an epoll backend for GMainLoop (13.92 KB, patch)
2014-01-24 20:36 UTC, Mikhail Zabaluev
none Details | Review
glib/gmain.c: Some opportunistic whitespace love (4.95 KB, patch)
2014-01-24 20:36 UTC, Mikhail Zabaluev
needs-work Details | Review
A simple test for polling on a file-like descriptor (1.73 KB, patch)
2014-01-24 20:36 UTC, Mikhail Zabaluev
committed Details | Review
glib/tests/mainloop: Acquire the context while asserting its state (1.32 KB, patch)
2014-01-24 20:36 UTC, Mikhail Zabaluev
committed Details | Review
Pluggable backends for GMainLoop (78.85 KB, patch)
2014-02-06 08:46 UTC, Mikhail Zabaluev
none Details | Review
Added an epoll backend for GMainLoop (13.97 KB, patch)
2014-02-06 08:47 UTC, Mikhail Zabaluev
none Details | Review
Pluggable backends for GMainLoop (78.46 KB, patch)
2014-06-29 20:16 UTC, Mikhail Zabaluev
none Details | Review
Added an epoll backend for GMainLoop (13.94 KB, patch)
2014-06-29 20:17 UTC, Mikhail Zabaluev
none Details | Review
glib/gmain.c: Some opportunistic whitespace love (4.41 KB, patch)
2014-06-29 20:18 UTC, Mikhail Zabaluev
none Details | Review

Description Mikhail Zabaluev 2013-04-28 13:08:49 UTC
An increasing number of modern application engines, such as Node.js and Rust, feature scalable event loop implementations such as libuv. There are other libraries that have their own implementations of an event loop, which an application developer might want to use as the event loop driver in preference to a GMainLoop, without needing to maintain a thread boundary. To interoperate with such engines and libraries efficiently, as well as to support modern scalable polling methods from the OS, the GLib event loop should support pluggable backend implementations.

Unfortunately, not only does GMainContext have a single compiled-in implementation, it's really designed around the old breed of poll(), select(), and WaitForMultipleObjects() on Win32. The main incompatibility point is that GPollFDs can be added with varying priorities which are honored on each iteration between prepare and poll, so that lower priority fd's do not get polled on if there is a higher priority non-fd source ready. This does not map well to the mechanics of event-based poll APIs.

After some early attempts (bug #156048) and more consideration, I think I have a feasible plan to add a new event loop API to GLib while preserving backwards compatibility with client code using GMainLoop/GMainContext.

* Create a new class or interface GEventContext, which should become the base for event loop backends, built-in and third-party alike.
* The interface for GEventContext and its usage by GSource should largely mirror that of GMainContext, except that all GPollFDs are considered to be of default priority.
* For interoperability with old code, each GEventContext can provide an associated GMainContext, accessible as the default or thread default context parallel to the mother GEventContext's role. GMainContext is reimplemented to work in terms of the GEventContext backend, until...
* When a client adds a GPollFD with a non-default priority to the GMainContext shim, the context switches into a compatibility mode, where all sources of the context are iterated in the old way by a separate thread which sends a wakeup to the context's event loop backend whenever there are sources ready. The separate thread is needed because the event backend may interoperate with other event sources unaccounted for in the GEventContext. There might be a log message to inform about the compatibility mode and its possible performance impact.
* GLib gets built-in backends for poll, epoll, WaitForMultipleObjects, and whatever else is available on the supported platforms.
* GMainContext can be deprecated as and when appropriate.

I'm going to start developing this as a feature branch, and create a glib-uv integration library as the reference third party event loop backend. If there are works in progress already or some other related experience, I'd love to hear about it in comments.
Comment 1 Allison Karlitskaya (desrt) 2013-04-28 14:23:54 UTC
hi Mikhail,

I had almost the same idea a while ago and even began work on it.  After a while of working on it, I discovered a way to make much more compatible use of GMainContext with epoll and I do not consider it to be inconsistent with priorities.  We already did some work last cycle to prepare for this.

Once GLib is on epoll, hooking it into another mainloop will be rather simple: you can just add GLib's epollfd to that loop and call it to iterate when it polls ready.

I think the correct way forward here is to add epoll to GMainContext and not introduce new interfaces.
Comment 2 Mikhail Zabaluev 2013-04-28 19:47:58 UTC
(In reply to comment #1)
> 
> I had almost the same idea a while ago and even began work on it.  After a
> while of working on it, I discovered a way to make much more compatible use of
> GMainContext with epoll and I do not consider it to be inconsistent with
> priorities.  We already did some work last cycle to prepare for this.

I tried reimplementing GMC with epoll years ago, but I could not find a way to support the GMainContext semantics of arbitrary I/O priorities without turning it into a system call fest. Or do you plan to deprecate non-default priorities on polled sources?

> Once GLib is on epoll, hooking it into another mainloop will be rather simple:
> you can just add GLib's epollfd to that loop and call it to iterate when it
> polls ready.

Is there an equivalent on Windows? libuv documentation also says that wouldn't work with a kqueue-based implementation on BSD, but maybe nobody here cares about that...

But even if that works, it is less efficient than plugging the descriptors directly into the iteration's blocking system call. My target is being able to run GSources on the uv loop as first class citizens.
Comment 3 Mikhail Zabaluev 2013-05-03 21:18:22 UTC
Another interesting question is, what should happen when a file descriptor is added to the context more than once. The current code and its underlying poll()/select() implementation seems to allow this and update each GPollFD with respect to its event mask and priority. It may be useful to have multiple GSources dispatched when a single file descriptor is reported ready. EPOLL_CTL_ADD fails in this case, so there has to be a one-to-many mapping between epoll entries and GPollFDs. In general, I think, the backend abstraction should require multiple dispatch to be supported and it should be up to the backend to implement this.
Comment 4 Mikhail Zabaluev 2013-05-04 12:14:12 UTC
A WIP branch is maintained (and will be occasionally rebased and force-pushed) on GitHub:
https://github.com/mzabaluev/glib/commits/eventloop
Comment 5 Dan Winship 2013-05-04 19:16:31 UTC
(In reply to comment #2)
> I tried reimplementing GMC with epoll years ago, but I could not find a way to
> support the GMainContext semantics of arbitrary I/O priorities without turning
> it into a system call fest. Or do you plan to deprecate non-default priorities
> on polled sources?

We could deprecate them in the sense of "we recommend that you don't do this any more", but we still have to keep them working (well) for existing programs.

In the email thread on bug 156048, Owen suggested:

> The cost of dynamically adding and removing file descriptors could
> kill any advantage of using epoll(). I suppose you could just epoll()
> on everything all the the time... there should be little extra cost
> to having extra file descriptors in the epoll(), and no effect
> on behavior.

Is there a problem with that idea?
Comment 6 Mikhail Zabaluev 2013-05-04 20:50:00 UTC
(In reply to comment #5)
> In the email thread on bug 156048, Owen suggested:
> 
> > The cost of dynamically adding and removing file descriptors could
> > kill any advantage of using epoll(). I suppose you could just epoll()
> > on everything all the the time... there should be little extra cost
> > to having extra file descriptors in the epoll(), and no effect
> > on behavior.
> 
> Is there a problem with that idea?

If there are both idle-ish and fd sources with a higher priority than an fd source that polls ready, maintaining backwards compatibility would need to hold back on dispatching the fd source while the higher priority sources are ready. The current implementation drops lower priority sources from the poll, but it still makes a poll syscall in all cases where an "epoll on everything" implementation would need to do. So yes, Owen appears to be right about there being little extra cost. This means we can try to plug epoll and third-party backends into GMainContext instead of introducing a parallel API.
Comment 7 Mikhail Zabaluev 2013-05-05 07:12:06 UTC
(In reply to comment #6)
> This means we can try to plug epoll and third-party
> backends into GMainContext instead of introducing a parallel API.

But then other nasty bits are showing through, like g_main_context_query(), g_main_context_check(), and g_main_context_set_poll_func().

So perhaps there should be a compatibility mode in GMainContext, implemented by switching to the poll backend the first time the client calls any of the poll-specific functions. If that works well enough, hot-switching to user-specified loop backends might even become an exported feature...
Comment 8 Allison Karlitskaya (desrt) 2013-05-05 15:41:30 UTC
I don't really understand the problem here.  We plan to do fd merging for epoll (and indeed, Dan already wrote a patch to do this before we have epoll because passing the same fd to poll() is bad behaviour in certain cases as well).  When you use g_main_context_query() you would just get the 'merged' list (ie: you would see exactly what's inside of the epoll at this moment).  This is not something that would require any kind of mode-switch.
Comment 9 Mikhail Zabaluev 2013-05-05 16:52:52 UTC
(In reply to comment #8)
> I don't really understand the problem here.  We plan to do fd merging for epoll
> (and indeed, Dan already wrote a patch to do this before we have epoll because
> passing the same fd to poll() is bad behaviour in certain cases as well).  When
> you use g_main_context_query() you would just get the 'merged' list (ie: you
> would see exactly what's inside of the epoll at this moment).  This is not
> something that would require any kind of mode-switch.

It may be OK for the epoll refactoring, but it will annoyingly complicate implementation for third-party loop backends, and may even not be feasible for some main loops (libuv at least has uv_walk where you can filter out your handles by the callback). And then the array of FDs fed back to g_main_context_check is total nonsense to implement for epoll-like backends, but it has to be done if you support the possibility that the client might have messed with the FDs.

Another thing is the whole exposure of iteration stages, and maybe even the one-step iteration method. libuv has a single-step mode to try to support this, but I'd like to enable main loop backends that only have "run" and "quit".
But perhaps the built-in epoll backend can be made gnarly enough to support all that rather than falling back to poll.

All said, if you can find a portable way to get a pollable handle on the entire GMainContext, it might be an easier option for main loop integration. uv_backend_fd() cheerfully returns a -1 on Windows, so I'm not holding my breath...
Comment 10 Mikhail Zabaluev 2013-05-05 17:16:45 UTC
(In reply to comment #9)
> may even not be feasible for some main loops.

Well, it's not really a problem as any additional state can be kept in the backend.
Comment 11 Allison Karlitskaya (desrt) 2013-05-05 18:52:42 UTC
(In reply to comment #9)
> some main loops (libuv at least has uv_walk where you can filter out your
> handles by the callback).

The only way I could imagine this to be useful is if you registered a specific GSource with GMainContext and wanted to filter it out later on the libuv interface.  I have absolutely no interest in this sort of integration.  If you want to filter your GSource, you can talk to GMainContext about that.

The integration that I am concerned about consists of exactly two things:

 - GMainContext (as a whole) can be dispatched from other mainloop
   implementations when it has some work to do

 - the inverse



The first case will be handled _very_ nicely in the epoll case by returning the fd for the other loop to poll.  It will be handled somewhat less nicely in the non-epoll case using g_main_context_query() and the associated interfaces (and indeed, that part is already possible and will continue to work in the same way).

The inverse operation depends entirely on the other mainloop library, but I think there is no doubt that GMainContext is powerful enough to handle just about anything that could be reasonably expected of us.


TL;DR: I really don't see the point here...
Comment 12 Dan Winship 2013-05-05 19:58:16 UTC
(In reply to comment #8)
> I don't really understand the problem here.  We plan to do fd merging for epoll
> (and indeed, Dan already wrote a patch to do this before we have epoll because
> passing the same fd to poll() is bad behaviour in certain cases as well).

Though that patch is languishing unreviewed in bug 11059. (Hint hint)

> The first case will be handled _very_ nicely in the epoll case by returning the
> fd for the other loop to poll.  It will be handled somewhat less nicely in the
> non-epoll case using g_main_context_query() and the associated interfaces (and
> indeed, that part is already possible and will continue to work in the same
> way).

We could emulate the epoll case in the non-epoll case by running just g_poll() in another thread, and using a GWakeup fd as the "master fd" (the equivalent of the epoll fd), and having APIs (g_main_context_get_master_fd(), g_main_context_process_master_fd()) that handling setting that up and keeping it going.
Comment 13 Mikhail Zabaluev 2013-05-05 22:17:35 UTC
(In reply to comment #11)
> The integration that I am concerned about consists of exactly two things:
>  - GMainContext (as a whole) can be dispatched from other mainloop
>    implementations when it has some work to do

This is the problem discussed here, plus the details of how individual fds in the context are polled.

> The first case will be handled _very_ nicely in the epoll case by returning the
> fd for the other loop to poll.

It's nice, but it could be better. Ideally, the event loop does only two system calls per iteration: a blocking poll call and a clock_gettime() to update the "loop time". A cascade-triggered epoll adds overhead, something that the kind of scalability/speed maniacs who wrote Node.js are keen to notice. If integration of foreign event loops with master fds becomes popular among applications and libraries, epoll chains can grow even longer.

I'm in no way opposed to adding the master FD feature to GLib as long as it's portable, but I'm trying to get at the bigger thing.

> It will be handled somewhat less nicely in the
> non-epoll case using g_main_context_query() and the associated interfaces (and
> indeed, that part is already possible and will continue to work in the same
> way).

That does not help if the foreign mainloop uses an add/remove/wait API similar to epoll. I like Dan's idea of a separate thread doing the poll, though, as it is similar to the "compatibility mode" thread from my original proposal.
Comment 14 Mikhail Zabaluev 2013-05-12 08:22:52 UTC
Another poll-centric behavior is nullifying revents in GPollFDs that haven't polled ready. poll() ensures it, but with epoll you need to do it for sources that look at revents in check, and _may_ expect them to remain unchanged till dispatch. Clients may also add GPollFDs directly into GMainContext.
Comment 15 Mikhail Zabaluev 2013-05-12 09:12:16 UTC
Also this, in g_main_context_query():

      /* We need to include entries with fd->events == 0 in the array because
       * otherwise if the application changes fd->events behind our back and
       * makes it non-zero, we'll be out of sync when we check the fds[] array.
       * (Changing fd->events after adding an FD wasn't an anticipated use of
       * this API, but it occurs in practice.) */

Supporting this with epoll means every iteration, the entire set of GPollFDs needs to be rescanned and any changes to fd->events addressed with EPOLL_CTL_MOD.

This and the problem in comment #14 could be fixed by adding new functions, e.g. g_main_context_add_poll_fast()/g_source_add_poll_fast(), that will exclude the fds so added from the O(N) loops needed to support the poll-centric behaviors. The legitimate need to change fd->events could be addressed by adding _modify_poll functions, which I already had to introduce to my WIP backends.
Comment 16 Allison Karlitskaya (desrt) 2013-05-12 12:08:02 UTC
As mentioned, in comment #1, this problem was addressed last cycle.  Adding GPollFDs like this is no longer cool and we already have a new API to replace it.  Read the docs:

https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#g-source-add-poll

"""

Using this API forces the linear scanning of event sources on each main loop iteration. Newly-written event sources should try to use g_source_add_unix_fd() instead of this API. 

"""

and note as well that 'check' and 'prepare' functions are encouraged to be NULL these days.
Comment 17 Mikhail Zabaluev 2013-05-12 16:13:37 UTC
(In reply to comment #16)
> As mentioned, in comment #1, this problem was addressed last cycle.  Adding
> GPollFDs like this is no longer cool and we already have a new API to replace
> it.

My bad, I should have used the unstable docs instead of skimming through a 5000+ line file... Fantastic, no new add_poll functions then (and I guess nobody cares about non-unix). The old-school GPollFDs would still need to be queried and updated, though.

> Using this API forces the linear scanning of event sources on each main loop
iteration.

As per the current master tree, "forces" is a rather strong word, as g_main_context_check() still iterates through poll records and sources down to the iteration's dispatch priority. I'm refactoring a part of it to use a hash table, though.
Comment 18 Mikhail Zabaluev 2013-05-12 16:22:17 UTC
(In reply to comment #17)
> My bad, I should have used the unstable docs

Meaning, of course, not the docs I have in Devhelp in the latest Fedora release :)
Comment 19 Allison Karlitskaya (desrt) 2013-05-13 14:52:13 UTC
I feel that I should be very clear on this point before you spend any more time on this: we are extremely unlikely to accept any substantial new source/main-context API and particularly not any sort of pluggable event loop backend interface.
Comment 20 Mikhail Zabaluev 2013-05-17 06:58:32 UTC
(In reply to comment #19)
> I feel that I should be very clear on this point before you spend any more time
> on this: we are extremely unlikely to accept any substantial new
> source/main-context API and particularly not any sort of pluggable event loop
> backend interface.

Thank you and Dan for your timely feedback; I didn't go down as many blind alleys as I probably would have without it.
Here's how it is shaping up:

https://github.com/mzabaluev/glib/commits/pluggable-mainloop

Achievements unlocked:

1. Implements built-in backends for epoll and classic g_poll(), falling back to poll if epoll is not available.
2. Provides a streamlined interface to implement built-in backends for kqueue, IOCP and whatever other fast polling interfaces may be available on different platforms.
3. Enables integration into third-party event loops, without chained epolls (best case) or helper threads (default case) of the simpler approach discussed in comment #12 and above.
4. There is no longer a requirement on the ordering of the fd array passed to g_main_context_check() to be the same as obtained from g_main_context_query(). The latter is actually no longer used by the built-in backends.
5. A collateral fix to bug #11059.
6. Passes make check (after fixing an unrelated trivial test fail in current master).

Drawbacks:

1. Introduces three new functions and a vtable for backends. The contracts and functioning of GMainContext were preserved to the best of my knowledge, at some performance cost.
2. Semi-deprecates usage of g_main_context_set_poll_func() as having non-trivial performance cost and side effects. The new API provides more than adequate replacement.
3. There might be some regressions lurking. I saw a test failure in /gdbus/codegen-peer-to-peer once, but was not able to reproduce it ever since, after trying thousands of times in a loop.

The new API could be left internal, thus losing advantage #3.

Things to do before it can be offered for merging:

* Do something smarter with temporarily disabling fds for non-recursive sources. I think it could be done lazily, only when iteration actually recurses.
* Make the context data lock recursive. The backend methods called from a locked context are not _supposed_ to be calling back to it, but you never know. This is an admitted cost of making the backend API public.
* Prove third-party integration can work with libuv.
* Make sure the test coverage of new code is decent.
* Documentation love, especially on the thread safety as pertains to specific backend methods.
Comment 21 Mikhail Zabaluev 2013-05-17 07:07:52 UTC
(In reply to comment #16)
> Adding GPollFDs like this is no longer cool and we already have a new API to replace it.

Unfortunately, the issue described in comment #14 leaks into the new API with g_main_context_query_unix_fd(). So I'd still consider adding "Ryanair fds" that the client would have to clear explicitly to make sure that any non-zero reported events are current.
Comment 22 Mikhail Zabaluev 2013-05-17 17:07:16 UTC
Perhaps I should explain the motivation for this change.
I'm writing GObject introspection bindings for Rust:
https://github.com/mzabaluev/grust

Through the bindings, I'd like to give applications written in Rust an ability to schedule async callbacks on a GMainContext and run GLib mainloops. Trouble is, the Rust runtime uses a libuv-based scheduler for its tasks (in essence, lightweight M:N green threads). So when one task calls g_main_loop_run() through the foreign function interface, this blocks all other tasks in the same scheduler thread.
I have a test case that demonstrates the problem:
https://github.com/mzabaluev/grust/blob/master/test/grust-proof.rs#L161

It's currently possible in principle to iterate a GMainContext from an uv loop using g_main_context_query() et al., but only in a way that is exceedingly klunky. A pollable master fd would be a nice boon, but ultimately I'd like to provide Rust users with the most direct and scalable way to use GIO among other things, preferably without any chained polls or thread syncopation.
Comment 23 Mikhail Zabaluev 2013-05-18 08:43:04 UTC
(In reply to comment #20)
> * Make the context data lock recursive. The backend methods called from a
> locked context are not _supposed_ to be calling back to it, but you never know.

Nah, it does not seem feasible to support backends getting back into the context and changing random things. Perhaps it's enough to add a documentation note telling backend writers to not try anything fancy from the under-lock methods besides calling g_main_context_wakeup(), lest they meet a deadlock. Or I should make changes to ensure that the backend is always called without the context lock, such as deferring poll set modifications until the next iteration. The latter is certainly pointless for the built-in backends, and would incur a performance cost with epoll.
Comment 24 Mikhail Zabaluev 2013-05-19 16:15:04 UTC
I have force-pushed some improvements:

https://github.com/mzabaluev/glib/commits/pluggable-mainloop

(In reply to comment #20)
> 1. Introduces three new functions and a vtable for backends.

It's down to two new functions, where only one is a substantial extension.

> * Do something smarter with temporarily disabling fds for non-recursive
> sources. I think it could be done lazily, only when iteration actually
> recurses.

Done. Also no removing fds piecemeal when the whole context is going away.

(In reply to comment #14)
> Another poll-centric behavior is nullifying revents in GPollFDs that haven't
> polled ready.

In fact it is a non-issue because of how source blocking and unblocking works in dispatch. After a source dispatches (non-recursively), its GPollFDs' revents fields are cleared.
This means, in particular, that the new style unix fds can be left out of the O(N) reset loop.
Comment 25 Eduardo Lima Mitev 2013-05-24 11:02:00 UTC
adding myself to CC
Comment 26 Mikhail Zabaluev 2013-09-10 19:51:23 UTC
An 'interesting' problem is revealed by the recently added test "/closure/iochannel". Apparently, adding to the epoll set a descriptor opened on the executable file of the process fails with an EPERM, whereas plain old poll on such a descriptor works as expected. If this is not a bug in epoll, it's an unfortunate leak in the abstraction.
Comment 27 Mikhail Zabaluev 2013-09-17 06:15:50 UTC
(In reply to comment #26)
> Apparently, adding to the epoll set a descriptor opened
> on the executable file of the process fails with an EPERM, whereas plain old
> poll on such a descriptor works as expected. If this is not a bug in epoll,
> it's an unfortunate leak in the abstraction.

In fact (and I wish it was better documented), epoll does not support polling on files and other similar descriptors at all. Regardless of how marginal the case of polling file-like objects is - for all test cases in the glib tree, only a contrived closure test in gobject exercises it - backward compatibility with poll needs to be preserved. I have updated the branch with a fix, and added a regression test for this.
Comment 28 Lennart Poettering 2013-11-14 00:58:57 UTC
Not that this was directly relevant to glib, but just maybe as inspiration:

Here's systemd's "sd-event" main loop which is based on epoll() and adds priorisation to it:

http://cgit.freedesktop.org/systemd/systemd/plain/src/systemd/sd-event.h
http://cgit.freedesktop.org/systemd/systemd/tree/src/libsystemd-bus/sd-event.c

It does the priorization by always flushing out all queued events from epoll, and adding it to a prioq of pending events. Then we execute the first one, and go back to the epoll, doing the same thing again. This means after each epoll() we will have to go through all triggered events linearly, but we have to do if we want the full flexibility of epoll but apply priorisation to it.
Comment 29 Mikhail Zabaluev 2013-11-20 19:12:17 UTC
(In reply to comment #28)
> Here's systemd's "sd-event" main loop which is based on epoll() and adds
> priorisation to it:
> 
> http://cgit.freedesktop.org/systemd/systemd/plain/src/systemd/sd-event.h
> http://cgit.freedesktop.org/systemd/systemd/tree/src/libsystemd-bus/sd-event.c

Thank you. The main optimization in epoll-related changes has been to get rid of linearisms around the poll(2) array; a GSequence was similarly used for speeding up lookups in one place. I don't think much more optimization can be done within the Glib 2 API.

FYI, I'm now working on a slightly different approach, to resolve some practical issues I've seen with integrating into uv_loop. Now the idea is to have custom poll backends installed and removed dynamically, with a push-pop API similar to g_main_context_{push,pop}_thread_default().
Comment 30 Mikhail Zabaluev 2014-01-24 20:35:42 UTC
Created attachment 267152 [details] [review]
gdbus: Prevent polling on a closed DBus connection
Comment 31 Mikhail Zabaluev 2014-01-24 20:35:58 UTC
Created attachment 267153 [details] [review]
Pluggable backends for GMainLoop

A main loop can now be constructed around a custom poll backend
with g_main_loop_new_with_poller().
There are a couple of new functions to enable poller
implementation, g_main_loop_prepare_poll() and g_main_loop_process_poll().
g_main_loop_start() can be used to acquire the context and relinquish
control to an external event loop.
Comment 32 Mikhail Zabaluev 2014-01-24 20:36:03 UTC
Created attachment 267154 [details] [review]
Added an epoll backend for GMainLoop
Comment 33 Mikhail Zabaluev 2014-01-24 20:36:11 UTC
Created attachment 267155 [details] [review]
glib/gmain.c: Some opportunistic whitespace love
Comment 34 Mikhail Zabaluev 2014-01-24 20:36:17 UTC
Created attachment 267156 [details] [review]
A simple test for polling on a file-like descriptor
Comment 35 Mikhail Zabaluev 2014-01-24 20:36:22 UTC
Created attachment 267157 [details] [review]
glib/tests/mainloop: Acquire the context while asserting its state

The iteration methods presume that the context is acquired.
Comment 36 Mikhail Zabaluev 2014-01-24 20:49:40 UTC
The patches attached above offer a reworked API that satisfies my current needs, also available here:

https://github.com/mzabaluev/glib/commits/pollers

I also have a proof-of-concept library for integration into libuv event loops using the proposed API:

https://github.com/mzabaluev/glib-uv
Comment 37 Mikhail Zabaluev 2014-01-24 21:48:54 UTC
(In reply to comment #29)
> (In reply to comment #28)
> > Here's systemd's "sd-event" main loop which is based on epoll() and adds
> > priorisation to it:
> > 
> > http://cgit.freedesktop.org/systemd/systemd/plain/src/systemd/sd-event.h
> > http://cgit.freedesktop.org/systemd/systemd/tree/src/libsystemd-bus/sd-event.c
> 
> Thank you. The main optimization in epoll-related changes has been to get rid
> of linearisms around the poll(2) array; a GSequence was similarly used for
> speeding up lookups in one place. I don't think much more optimization can be
> done within the Glib 2 API.

On a better thought, separate priority queues could also be used to go through prepare and check callbacks for the sources that still have those, instead of one happy linked list that is traversed now. But that is out of scope for this bug.
Comment 38 Colin Walters 2014-01-25 15:02:03 UTC
(In reply to comment #22)
> Perhaps I should explain the motivation for this change.
> I'm writing GObject introspection bindings for Rust:
> https://github.com/mzabaluev/grust

Very cool!!  I had been thinking of trying this myself at some point, glad to see you started it.  Can you update the README with a brief blurb about how far along they are and what needs doing?

> Through the bindings, I'd like to give applications written in Rust an ability
> to schedule async callbacks on a GMainContext and run GLib mainloops.

So...I wonder if we could lean on Rust the other way and have them support dynamically plugging in say the GLib event loop.  I'd guess a lot of their native code is tied to libuv, but still.

The thing is that while GLib does play well with others to the best degree we can, some things like the UNIX signal handling we have to assume we "own" the process state.

If we can get the language bindings on top of GLib to the greatest degree possible, rather than the other way around, things work better.

> It's currently possible in principle to iterate a GMainContext from an uv loop
> using g_main_context_query() et al., but only in a way that is exceedingly
> klunky. A pollable master fd would be a nice boon, but ultimately I'd like to
> provide Rust users with the most direct and scalable way to use GIO among other
> things, preferably without any chained polls or thread syncopation.

That makes sense.
Comment 39 Colin Walters 2014-01-25 15:09:30 UTC
Review of attachment 267152 [details] [review]:

First, on mechanics - can you please replicate in the git patch the useful data you wrote in the bugzilla attachment?  The patch should have a link to this bug too.  "git-bz" will do that for you, but it's not too hard to do by hand.  See: https://wiki.gnome.org/GnomeLove/SubmittingPatches

On the content of the patch - this looks correct and it will likely fix the issue, but I think a better fix would be for GDBus to keep track of the source it added, and remove it when the connection is closed.  Since there can only be one read at a time, let's just do worker->read_source = g_socket_create_source (), and then in read_with_control_data_free(), unref it.
Comment 40 Colin Walters 2014-01-25 15:10:03 UTC
Review of attachment 267155 [details] [review]:

Sure.
Comment 41 Colin Walters 2014-01-25 15:11:21 UTC
Review of attachment 267156 [details] [review]:

Looks right.
Comment 42 Colin Walters 2014-01-25 15:12:07 UTC
Review of attachment 267157 [details] [review]:

Looks good.
Comment 43 Mikhail Zabaluev 2014-01-28 00:04:11 UTC
(In reply to comment #39)
> First, on mechanics - can you please replicate in the git patch the useful data
> you wrote in the bugzilla attachment?  The patch should have a link to this bug
> too.  "git-bz" will do that for you, but it's not too hard to do by hand.  See:
> https://wiki.gnome.org/GnomeLove/SubmittingPatches

The patch was submitted by git-bz, no extra information added. But I used -n to skip the bug link insertion :/ I will do as instructed from now on.

> On the content of the patch - this looks correct and it will likely fix the
> issue, but I think a better fix would be for GDBus to keep track of the source
> it added, and remove it when the connection is closed.  Since there can only be
> one read at a time, let's just do worker->read_source = g_socket_create_source
> (), and then in read_with_control_data_free(), unref it.

The problem is that between attaching the source and entering the next poll, the worker thread may act on a close request and start closing the socket. That does not terminate the pending read yet (only polling on the cancellation handle will), so GIOStream's async close thread may close the socket before its descriptor gets polled in the GDBus worker thread. This is probably harmless in the current master as the cancellation will win anyway, but with epoll this causes EBADF and other nastiness on EPOLL_CTL_ADD, something I wouldn't like to ignore.

I think keeping track of the read source and destroying it just before the socket async close is called would fix it for real.
Comment 44 Mikhail Zabaluev 2014-01-28 08:10:55 UTC
(In reply to comment #43)
> The problem is that between attaching the source and entering the next poll,
> the worker thread may act on a close request and start closing the socket. That
> does not terminate the pending read yet (only polling on the cancellation
> handle will), so GIOStream's async close thread may close the socket before its
> descriptor gets polled in the GDBus worker thread.

Looking further into this, perhaps g_io_stream_close_async() should punt the "real close" to an idle handler if there are pending operations on the input and output streams. This will give those operations time to get cancelled if the application took care to cancel them, like GDBus does. If any substream operations are not cancelled, It's not clear to me that GIOStream's close should ignore them, either. Anyhow, this has grown into a topic for another bug.
Comment 45 Mikhail Zabaluev 2014-02-05 21:57:34 UTC
Comment on attachment 267152 [details] [review]
gdbus: Prevent polling on a closed DBus connection

A better fix for this is proposed in bug 723719.
Comment 46 Mikhail Zabaluev 2014-02-06 08:46:59 UTC
Created attachment 268263 [details] [review]
Pluggable backends for GMainLoop

A main loop can now be constructed around a custom poll backend
with g_main_loop_new_with_poller().
There are a couple of new functions to enable poller
implementation, g_main_loop_prepare_poll() and g_main_loop_process_poll().
g_main_loop_start() can be used to acquire the context and relinquish
control to an external event loop.
Comment 47 Mikhail Zabaluev 2014-02-06 08:47:13 UTC
Created attachment 268264 [details] [review]
Added an epoll backend for GMainLoop
Comment 48 Mikhail Zabaluev 2014-02-08 06:59:01 UTC
(In reply to comment #38)
> > I'm writing GObject introspection bindings for Rust:
> > https://github.com/mzabaluev/grust
> 
> Very cool!!  I had been thinking of trying this myself at some point, glad to
> see you started it.  Can you update the README with a brief blurb about how far
> along they are and what needs doing?

Why, it's a Github project, so it has an issue list:
https://github.com/mzabaluev/grust/issues?state=open

> So...I wonder if we could lean on Rust the other way and have them support
> dynamically plugging in say the GLib event loop.  I'd guess a lot of their
> native code is tied to libuv, but still.

As a matter of fact, with Rust 0.9 my integration issues appear to be mostly obsolete since there is now a 1:1 runtime:
https://github.com/mzabaluev/grust/issues/4#issuecomment-33303817
Comment 49 Matthias Clasen 2014-06-28 17:56:47 UTC
Review of attachment 267155 [details] [review]:

this doesn't apply anymore
Comment 50 Matthias Clasen 2014-06-28 17:57:18 UTC
Attachment 267156 [details] pushed as c0e8f8a - A simple test for polling on a file-like descriptor
Attachment 267157 [details] pushed as 9f5afe3 - glib/tests/mainloop: Acquire the context while asserting its state
Comment 51 Mikhail Zabaluev 2014-06-29 20:16:58 UTC
Created attachment 279556 [details] [review]
Pluggable backends for GMainLoop

A main loop can now be constructed around a custom poll backend
with g_main_loop_new_with_poller().
There are a couple of new functions to enable poller
implementation, g_main_loop_prepare_poll() and g_main_loop_process_poll().
g_main_loop_start() can be used to acquire the context and relinquish
control to an external event loop.
Comment 52 Mikhail Zabaluev 2014-06-29 20:17:23 UTC
Created attachment 279558 [details] [review]
Added an epoll backend for GMainLoop
Comment 53 Mikhail Zabaluev 2014-06-29 20:18:08 UTC
Created attachment 279559 [details] [review]
glib/gmain.c: Some opportunistic whitespace love
Comment 54 Philip Withnall 2018-02-16 12:08:40 UTC
*** Bug 156048 has been marked as a duplicate of this bug. ***
Comment 55 Philip Withnall 2018-02-16 12:09:38 UTC
There is some relevant discussion in bug #156048.
Comment 56 GNOME Infrastructure Team 2018-05-24 15:14:03 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME'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.gnome.org/GNOME/glib/issues/693.