[3021] in BarnOwl Developers
Re: [RFC] GMainLoop branch
daemon@ATHENA.MIT.EDU (Nelson Elhage)
Sun Mar 27 13:58:55 2011
Date: Sun, 27 Mar 2011 13:58:46 -0400
From: Nelson Elhage <nelhage@nelhage.com>
To: David A Benjamin <davidben@mit.edu>
Cc: barnowl-dev@mit.edu
In-Reply-To: <alpine.DEB.1.10.1103031214500.27752@dr-wily.mit.edu>
I've now skimmed this branch, although not reviewed the code super-closely. I
was initially opposed to GMainLoop because of how batshit it seemed when I
skimmed the documentation, but I feel like we have, over time, grown most of its
complexity anyways, between pre-select actions and all that funniness. So I
think I am probably in favor of this approach, since their mainloop is almost
certainly more battle-tested than our homegrown implementation, and hopefully it
will make life easier if we grow more complexity going forward. And the ability
to (tastefully!) add threads to the C core without too much pain is probably
worth it.
So, +1 to this branch, on review. If no one else has comments, I'll find time to
do a more careful code review and we can get this merged.
- Nelson
On Thu, Mar 03, 2011 at 12:21:04PM -0500, David A Benjamin wrote:
> Continuing on the pattern of MORE GLIB THAN YOUR BARNOWL HAS ROOM
> FOR, here is a branch that backs owl_select onto glib's main loop.
> Unlike the other files of glib, this one's diffstat is not very
> good-looking. On the contrary, I'm adding near 200 lines of code.
>
> What we do gain from this main loop is (other than more Gs[1])
> thread-safety. Because what BarnOwl really needs is threads, right?
> Glib's main loop allows for threads to post messages to each other,
> which will be useful if we want to, say, put file access on a
> separate thread so we're still responsive when AFS blows up. In
> fact, Adam's been working on a patch on top of this branch to move
> logging to a worker thread which I think people have been asking
> for. This may also come in handy for a future sqlite-backed BarnOwl
> so we needn't block on the database.
>
> The documentation of glib's event loop here is:
>
> http://library.gnome.org/devel/glib/stable/glib-The-Main-Event-Loop.html
>
> As an overview (and because it's not actually well documented; I had
> to source-dive a bit), the main structure is a GMainContext which
> belongs to a particular thread and contains a number of GSources. To
> run a loop you make a GMainLoop attached to the GMainContext and run
> it. (I imagine they're separated so you can run nested event loops.
> Nevermind that this is madness and the source of no end of
> re-entrancy bugs in graphical programs.) GMainContexts are
> thread-safe and may be posted to from other threads.
>
> A GSource has three hooks (and a destructor): prepare, check, and
> dispatch and a number of fds attached. Dispatch is fairly
> straight-forward. Prepare and check return whether the source is
> ready to run. Prepare runs before select and sets a minimum timeout.
> Check runs afterwards and has access to the results of a select
> (actually a poll(2)) on its file descriptors. If either returns
> true, the main loop may call dispatch. They're analogous to
> ZQLength() and ZPending(). fds may be changed at any point (in
> particular during prepare), making the AIM hack less of a hack.
> Still ugly though.
>
> GSources also has this hard priority system. At any loop iteration,
> only the highest priority GSources are ever run. So, you aren't
> guaranteed that your dispatch will run if prepare/check return true
> or that prepare/check will run at all. The current version of this
> branch doesn't use priorities at all, but we could do so. (E.g.,
> always prefer keypresses and redraws to a pile of incoming zephyrs.
> Or separate resize/redraw and process resizes first; I think GTK
> does this.)
>
> So, in theory this branch is fairly straight-forward; add GSources
> that bridge our events and glib's. There's some muckiness with
> pre-select actions (I converted them to GSources directly; if we
> want to use the hard priority system, we want to split the check
> portions into the appropriate callbacks or lower priority sources
> will never fire.)
>
> There is one complication: UNIX signals are apparently totally
> idiotic. AFAICT, signal masks are per-thread, but signal
> dispositions are per-process. When you send a signal to process as
> opposed to a particular thread, you look at all the threads with
> this signal masked and... pick one arbitrarily. (What the hell!???)
> And the same handler runs no matter what thread gets picked. So,
> really the only sane way to deal with this is to have a dedicated
> signal thread that loops on sigwait and mask those signals on all
> threads. On the plus side, our signal handlers are now much much
> simpler because we don't have to worry about getting one during a
> malloc.
>
> On the other hand, if perl code decides to say, unmask a signal we
> care about or set a signal handler, badness may ensue. The former
> isn't so bad; threads in sigwait get priority over others, so we'll
> have a race condition. If it sets a signal handler, I don't know how
> perl reacts when its signal handler finds itself running on the
> wrong thread. Probably badly. We could mask every signal ever on the
> other threads, but I'm tempted to just say "don't do that".
>
> So... thoughts? Do we want to go down this path?
>
> David
>
> [1] In fact,
> % echo $(expr $(git diff origin/master | grep '^\+' | grep -cio 'g') \
> - $(git diff origin/master | grep '^\-' | grep -cio 'g'))
> 106
>
> The following changes since commit 9620c8d92ecfed30b4b6c4d5d6861179e2ab418d:
>
> Load Encode module for IRC. (2011-03-02 13:10:29 -0500)
>
> are available in the git repository at:
> git://github.com/davidben/barnowl.git g_main_loop
>
> David Benjamin (21):
> Start of GMainContext code
> Make the owl_window redraw hook a GSource
> Also make owl_process_messages a GSource
> Add a zephyr GSource
> First go at sigwait-based signal handling
> Punt pre-select actions
> Make owl_signal_init take a signal-thread callback
> Allow interrupts to be taken at any point
> Add a GSource for AIM events
> Remove owl_select
> Remove owl_select_aim_hack and _owl_select_timer_eq
> Just ignore SIGPIPE altogether
> Don't deliver SIGALRM through the signal thread
> Reset signal dispositions and mask, in case our parent was lame
> Move owl_process_input_char to owl.c
> Remove OwlGlobalNotifier typedef
> Cleanly destroy the signal thread on shutdown
> Add owl_select_post_task helper function
> Use sigwaitinfo instead of sigwait
> resizepending needn't be sig_atomic_t anymore
> Push stderr redirection onto a worker thread
>
> Makefile.am | 3 +-
> aim.c | 115 ++++++++++++++-
> filterproc.c | 4 -
> functions.c | 29 +----
> global.c | 83 +++++------
> owl.c | 225 +++++++++++++----------------
> owl.h | 19 +--
> select.c | 454 +++++++++++++++++++++++-----------------------------------
> signal.c | 62 ++++++++
> stderr.c | 75 ++++++++++
> window.c | 50 +++++++
> window.h | 2 +
> zephyr.c | 73 ++++++++--
> 13 files changed, 688 insertions(+), 506 deletions(-)
> create mode 100644 signal.c
> create mode 100644 stderr.c
