Created attachment 32328 [details] [review]
patch, handle SIGINTR in g_main_loop_run

OK.  I think this patch is not portable, so we may need to #ifdef it for win32.
 For systems without <signal.h>, we probably could do similar to gtk.main().

However, I think checking for stuff in timeouts is inherently evil, since it
forces the OS to wake up the process periodically, even if is completely idle,
waiting for some network event.  It makes no difference for GUI apps, but I'd
like to use pygobject in a networking app, and it is inadmissible to install
timeouts to check for signals in this application domain.

Created attachment 32335 [details] [review]
patch, updated

Updated patch.	This time it should compile on all systems, since the new code
disables itself on systems without signals.  However, on such systems we don't
handle keyboard interrupt.

Comment on attachment 32335 [details] [review]
patch, updated

Gustavo, when you have time, can you refactor this one to not use so many
ifdefs inside the functions. And frankly, I'm not convinced it's a good idea

Created attachment 36791 [details] [review]
refactored patch

Trust me, timeouts in main loops is a bad idea!  With little more work, we
could handle arbitrary signals, this way.

*** Bug 165817 has been marked as a duplicate of this bug. ***

Created attachment 36811 [details] [review]
patch from bug #165817

For completeness, adding patch similar to what's done in pygtk

Re-titling bug to include all signals, I'll attach a test case for SIGUSR1

Created attachment 36812 [details]
testcase for SIGUSR1

Created attachment 36813 [details] [review]
alternative patch

Okay, here's another way we could do this.

When GMainLoop is in poll() and python handles a signal, poll() should return
-1 and set errno to EINTR. GMainLoop then calls check() on all sources, at
which point we can check to see if python has any signals which need
dispatching. If any exceptions have been raised, we quit the mainloop.

Oh, and as regards threading - Python ensures that signals only get delivered to
the main thread, so PyErr_CheckSignals() will only cause signal handlers to be
invoked if its called from the main thread.

So if you run the main loop from another thread, the kernel delivers a signal to
that thread interrupting poll(), PyErr_CheckSignals() will do nothing and the
main thread will be able to handle the signal as normal.

Actually, on threading - if the kernel delivers the signal to anything but the
main thread, then a main loop running in the main thread won't get woken up I
don't think. See Python/Modules/signalmodule.c for some details about archs on
which the kernel won't deliver the signal to the main thread.

Created attachment 36814 [details] [review]
minor change to the custom GSource patch

Okay, to handle the corner case of the kernel delivering the signal to another
thread and the main loop in the main thread not working, I've added a 500ms
timeout to the source too.

Okay, summary of some other points that came up discussing this with jamesh on irc:

  1) On windows, it doesn't look like we can rely on the "signal interrupts
     poll()" behaviour, so we'd rely on the timeout there too. Perhaps
     100ms is more appropriate, then

  2) We need to remove the source when the main loop is finalized. Need to
     not take a ref on the main loop to allow it to be finalized.

  3) If you have recursive main loops, what we have now will only cause the
     first main loop created to quit. We should check to see if any exceptions
     are set in check() and quit the main loop rather than just checking the
     return value from PyErr_CheckSignals(). That way all main loops will
     quit if there's an exception set.

Okay, I think we decided to add a timeout only on Windows or if threads or enabled.

Created attachment 36864 [details] [review]
fixed up custom GSource patch

Okay, so this has the changes we talked about

 - have the 100ms timeout if threads are enabled or if its windows

 - remove the source when the loop is finalized

 - quit all loops when an exception occurs

Perfect!  Excellent work! :-)
Just one note.  The behaviour is a bit different from gtk+ main loop. 
gtk.main()   catches exceptions, prints them, and continues the loop.

  With your patch, the programmer has to do something like:

main = gobject.MainLoop()
while True:
  try:
    main.run()
  except:
    continue
  break

Not saying it's bad.  Perhaps better, since it gives a bit more control to the
programmer.

Yeah, I don't understand why the mainloop would continue running if there's an
uncaught exception ... doesn't seem to make much sense to me.

Can change that if you like, I've no strong opinion on that.

No, I'm fine with it this way.  In python terminology, +0 :-)
But I was raising the point so others watching the bug may comment..

So, okay to commit ?

(Next thing is to make gtkmain use this source too, but lets get this much in first)

I think it's important for the mainloops in gobject to be consistent with the
mainloop in gtk. Eg, exceptions should not quit the main loop. 
Perhaps it would make more sense to quit the mainloop,  but that's not how
gtk.main works and we cannot change it at this point.

I agree we shouldn't change the default behaviour of the gtk main loop.
Maybe we could add an optional keyword argument, like this?

  gtk.main(ignore_errors=True)

To get exceptions to quit the main loop, one would call:
gtk.main(ignore_errors=False)

Same approach could be taken for gobject.MainLoop.run...

Problem is, we are API frozen right now, so perhaps we should not quit the
mainloop on error, and add the option to quit on error in next development
cycle.  Or embrace the inconsistency with gtk.main() and make it quit on error.
 Either way is fine by me.  Let jdahlin decide ;)

Okay, so we'll stick with the way it works with gtk.main() - an exception from a
signal handler (e.g. when you hit Ctrl-C) causes the outermost main loop to quit.

I'll attach a test case which shows the same behaviour working with both
gobject.MainLoop() and gtk.main().

Your run it with either "gtk" or "gobject" as the first argument:

[markmc@blaa python]$ ./test-gtkmain.py gtk
Traceback (most recent call last):

+ Trace 55456

Created attachment 37180 [details]
test case

Created attachment 37181 [details] [review]
patch which mimic gtk.main() behaviour

Oh, an yes - I have tested with and without DISABLE_THREADING defined

gjc reviewed approved:

2005-02-09  Mark McLoughlin  <mark@skynet.ie>

        Fix for bug #154779 - Python signal handlers don't
        get executed while you're sitting in the main loop.

        * gobject/pygmainloop.c:
        (pyg_save_current_main_loop),
        (pyg_restore_current_main_loop),
        (pyg_get_current_main_loop): functions for keeping
        track of the currently running main loop. A version
        using TLS and another using a global variable, depending
        on whether DISABLE_THREADING is defined
        (pyg_signal_watch_prepare), (pyg_signal_watch_check),
        (pyg_signal_watch_dispatch), (pyg_signal_watch_new):
        a GSource which runs the python signal handlers whenever
        the mainloop is interrupted by signal delivery.
        (pyg_main_loop_new), (pyg_main_loop_dealloc): add and
        remove the source.
        (_wrap_g_main_loop_run): push/pop the currently running
        main loop.

        * gobject/pygobject-private.h: add a pointer for the
        source to PyGMainLoop.

        * gobject/Makefile.am: add -DPLATFORM_WIN32 to cflags
        if building on Windows.

This patch was reworked and is now used for gtk.main aswell, see bug 348937