gtk_main() needs to be implemented to do the following:

    PyGILState_STATE gstate;
    GIOChannel* channel;

    /* Watch for input on stdin */
    channel = g_io_channel_unix_new(fileno(stdin));
    g_io_add_watch(channel, G_IO_IN, gtk_main_quit, NULL);
    g_io_channel_unref(channel);

    /* Start the main loop */
    gstate = PyGILState_Ensure();
    gtk_main();
    PyGILState_Release(gstate);


Can probably be implemented as an override in pygti as:

channel = GLib.IOChannel.unix_new(sys.stdin.fileno())
GLib.io_add_watch(channel, glib.IO_IN, Gtk.main_quit, None)

Gtk.main()

s/pygti/pygi/

Actually, we need a bit more stuff, something like this:

typedef struct {
    GSource source;
    int fds[2];
    GPollFD fd;
} PySignalWatchSource;

static gboolean
pygtk_main_watch_prepare(GSource *source,
                         int     *timeout)
{
    /* Python only invokes signal handlers from the main thread,
     * so if a thread other than the main thread receives the signal
     * from the kernel, PyErr_CheckSignals() from that thread will
     * do nothing.
     */

    /* On Windows g_poll() won't be interrupted by a signal
     * (AFAIK), so we need the timeout there too, even if there's
     * only one thread.
     */
#ifndef PLATFORM_WIN32
    if (!pyg_threads_enabled)
        return FALSE;
#endif

#ifdef HAVE_PYSIGNAL_SETWAKEUPFD
    {
        PySignalWatchSource *real_source = (PySignalWatchSource *)source;

        if (real_source->fds[0] != 0)
            return FALSE;

        /* Unfortunately we need to create a new pipe here instead of
         * reusing the pipe inside the GMainContext.  Ideally an api
         * should be added to GMainContext which allows us to reuse
         * that pipe which would suit us perfectly fine.
         */
        if (pipe(real_source->fds) < 0)
            g_error("Cannot create main loop pipe: %s\n",
                    g_strerror(errno));

        real_source->fd.fd = real_source->fds[0];
        real_source->fd.events = G_IO_IN | G_IO_HUP | G_IO_ERR;
        g_source_add_poll(source, &real_source->fd);

        PySignal_SetWakeupFd(real_source->fds[1]);
    }
#else /* !HAVE_PYSIGNAL_SETWAKEUPFD */
    /* If we're using 2.5 or an earlier version of python we
     * will default to a timeout every second, be aware,
     * this will cause unnecessary wakeups, see
     * http://bugzilla.gnome.org/show_bug.cgi?id=481569
     */
    *timeout = 1000;
#endif /* HAVE_PYSIGNAL_SETWAKEUPFD */

    return FALSE;
}

static gboolean
pygtk_main_watch_check(GSource *source)
{
    PyGILState_STATE state;

    state = pyg_gil_state_ensure();

    if (PyErr_CheckSignals() == -1 && gtk_main_level() > 0) {
        PyErr_SetNone(PyExc_KeyboardInterrupt);
        gtk_main_quit();
    }

    pyg_gil_state_release(state);

    return FALSE;
}

static gboolean
pygtk_main_watch_dispatch(GSource    *source,
                          GSourceFunc callback,
                          gpointer    user_data)
{
    /* We should never be dispatched */
    g_assert_not_reached();
    return TRUE;
}

static void
pygtk_main_watch_finalize(GSource *source)
{
    PySignalWatchSource *real_source = (PySignalWatchSource*)source;

    if (source != NULL) {
        if (real_source->fds[0] != 0)
            close(real_source->fds[0]);

        if (real_source->fds[1] != 0) {
#if HAVE_PYSIGNAL_SETWAKEUPFD
            int  wakeup_fd = PySignal_SetWakeupFd(-1);
            if (wakeup_fd != real_source->fds[1]) {
                /* Probably should never happen. */
                PySignal_SetWakeupFd(wakeup_fd);
            }
#endif

            close(real_source->fds[1]);
        }
    }
}

static GSourceFuncs pygtk_main_watch_funcs =
{
    pygtk_main_watch_prepare,
    pygtk_main_watch_check,
    pygtk_main_watch_dispatch,
    pygtk_main_watch_finalize
};

static GSource *
pygtk_main_watch_new(void)
{
    return g_source_new(&pygtk_main_watch_funcs, sizeof(PySignalWatchSource));
}


static PyObject *
_wrap_gtk_main(PyObject *self)
{
    GSource *main_watch;
    // Call enable_threads again to ensure that the thread state is recorded
    if (pyg_threads_enabled)
        pyg_enable_threads();

    main_watch = pygtk_main_watch_new();
    pyg_begin_allow_threads;
    g_source_attach(main_watch, NULL);
    g_source_unref(main_watch);
    gtk_main();
    g_source_destroy(main_watch);
    pyg_end_allow_threads;
    if (PyErr_Occurred())
        return NULL;
    Py_INCREF(Py_None);
    return Py_None;
}

It's actually not necessary to add that much logic to Gtk.main.

With PyGObject, GLib.MainLoop is already a custom wrapper class that can do the necessary handling to propagate SIGINT; it just doesn't get called by Gtk.main:

ebroder@warwick:~$ python
Python 2.7.2+ (default, Oct  4 2011, 20:06:09) 
[GCC 4.6.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from gi.repository import GLib, Gtk
>>> GLib.MainLoop().run()
^CTraceback (most recent call last):

+ Trace 228816

(In reply to comment #4)
> It's actually not necessary to add that much logic to Gtk.main.
> 
> With PyGObject, GLib.MainLoop is already a custom wrapper class that can do the
> necessary handling to propagate SIGINT; it just doesn't get called by Gtk.main:
> 
> ebroder@warwick:~$ python
> Python 2.7.2+ (default, Oct  4 2011, 20:06:09) 
> [GCC 4.6.1] on linux2
> Type "help", "copyright", "credits" or "license" for more information.
> >>> from gi.repository import GLib, Gtk
> >>> GLib.MainLoop().run()
> ^CTraceback (most recent call last):
> 

Ah, I see so the issue was that GLib.MainLoop does something more than Gtk.Main() to handle SIGINT. Why doesn't Gtk.Main() just use the GLib.MainLoop then?  Is this something we can fix in Gtk?  The issue is we can't have a dependency on Gtk so providing a static binding for Gtk.Main would be require creating a loadable module which I would like to avoid.

It seems that the gtk_main() is simple enought to be overridden with GObject.MainLoop().run(). Either that or figure out why Gtk.main() screws unix signal.

http://git.gnome.org/browse/gtk+/tree/gtk/gtkmain.c

(In reply to comment #6)
> It seems that the gtk_main() is simple enought to be overridden with
> GObject.MainLoop().run().

For me, this screws up Gtk.main_quit(). This program:

--- 8< ---
from gi.repository import Gtk

win = Gtk.Window()

win.connect('destroy', lambda w: Gtk.main_quit())
win.show()

but = Gtk.Button("Hello")
win.add(but)
but.show()

try:
    Gtk.main()
except KeyboardInterrupt:
    pass
--- >8 ---

gives this traceback when closing the window:

(foo2.5.py:24198): Gtk-CRITICAL **: gtk_main_quit: assertion `main_loops != NULL' failed

It works (without traceback) if storing the mainloop object somewhere and calling the quit() method on it in the callback for the destroy signal.

(In reply to comment #4)
> It's actually not necessary to add that much logic to Gtk.main.
> 
> With PyGObject, GLib.MainLoop is already a custom wrapper class that can do the
> necessary handling to propagate SIGINT; it just doesn't get called by Gtk.main:

That's because GLib.MainLoop has all that logic. It might be necessary to refactor that part to be usable by gtk_main.

(In reply to comment #6)
> It seems that the gtk_main() is simple enought to be overridden with
> GObject.MainLoop().run(). Either that or figure out why Gtk.main() screws unix
> signal.
> 
> http://git.gnome.org/browse/gtk+/tree/gtk/gtkmain.c

gtk_main() does more, it cannot just be replaced by loop, a few things need to happen when shutting down the loop.

In gtkmain.c, it said:
"""
 * It's OK to use the GLib main loop directly instead of gtk_main(), though it
 * involves slightly more typing. See #GMainLoop in the GLib documentation.
"""

Here is the whole gtk_main()/gtk_main_quit():

    /**
     * gtk_main:
     *
     * Runs the main loop until gtk_main_quit() is called.
     *
     * You can nest calls to gtk_main(). In that case gtk_main_quit()
     * will make the innermost invocation of the main loop return.
     */
    void
    gtk_main (void)
    {
      GMainLoop *loop;

      gtk_main_loop_level++;

      loop = g_main_loop_new (NULL, TRUE);
      main_loops = g_slist_prepend (main_loops, loop);

      if (g_main_loop_is_running (main_loops->data))
        {
          GDK_THREADS_LEAVE ();
          g_main_loop_run (loop);
          GDK_THREADS_ENTER ();
          gdk_flush ();
        }

      main_loops = g_slist_remove (main_loops, loop);

      g_main_loop_unref (loop);

      gtk_main_loop_level--;

      if (gtk_main_loop_level == 0)
        {
          /* Try storing all clipboard data we have */
          _gtk_clipboard_store_all ();

          /* Synchronize the recent manager singleton */
          _gtk_recent_manager_sync ();
        }
    }

    void
    gtk_main_quit (void)
    {
      g_return_if_fail (main_loops != NULL);

      g_main_loop_quit (main_loops->data);
    }

Except the GDK_THREADS_LEAVE/ENTER part, I see no other things that make it
different from g_main_loop_run() signal wise.

All the other codes just append the main loop to a global list so that the
gtk_main() can be nested. The clean up parts are simple too (clipboard and recent manager cleanup)

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

Any more ideas on implementing a fix for this bug? Or a work-around that doesn't break Gtk.main_quit()?

(In reply to comment #11)
> Any more ideas on implementing a fix for this bug? Or a work-around that
> doesn't break Gtk.main_quit()?

The work around is to bind SIGINT to SIG_DFL:

    if __name__ == '__main__':
        import signal
        signal.signal(signal.SIGINT, signal.SIG_DFL)
        main()

Before I forget these yet again, adding a couple of links to the
history on Johan's proposal from comment #3:

https://bugzilla.gnome.org/show_bug.cgi?id=481569
http://mail.python.org/pipermail/python-dev/2007-December/thread.html#75589
http://bugs.python.org/issue1583

As far as I understand all of the above, PySignal_SetWakeupFd was
specifically created way back to fix signal handling in PyGTK.
Grepping PyGObject sources doesn't show this being used though,
except an at the moment useless test in configure.ac...

Created attachment 230080 [details]
SIGINT test program

12:48 <dieterv> pitti: python2 test_sigint.py, you get a GtkWindow, go back to terminal and ^C -> works
12:48 <dieterv> pitti: but python2 test_sigint.py --nope, you get a GtkWindow, go back to terminal and ^C -> nothing
12:49 <dieterv> pitti: my jhbuilded python is a couple of days old though (f1a5f1b92d577c542d4258e63e595d4da9acd484) so maybe it was fixed after that commit...
12:49 <dieterv> eer, pygobject, not python
12:50 <pitti> ah, so that's GLib.MainLoop vs. Gtk.main()
12:51 <pitti> pygobject's test cases only cover GLib.MainLoop for SIGINT
12:54 <dieterv> pitti: another way for SIGINT to do nothing is to run a GLib.MainLoop and later on run a FileChooserDialog for example
12:54 <pitti> could you please send this to the bug report, so that we have these details on record?
12:55 <dieterv> 622084, 689208 or both?
12:55 <pitti> 622084 I think
12:55 <dieterv> ok
12:55 <pitti> merci
12:56 <dieterv> graag gedaan :P

This code is simple workaround that I am using:

    exchook = sys.excepthook
    def new_hook(type, value, traceback):
        if isinstance(value, KeyboardInterrupt):
            Gtk.main_quit()
            return
        return exchook(type, value, traceback)

    sys.excepthook = new_hook

    # it should raise KeyboardInterrupt
    GLib.timeout_add(500, lambda : True)

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

Created attachment 238552 [details]
Proof of concept pure python implementation

OK, it is a bit late, and I have been playing with this a bit too long ...

This test program is a pure python implementation, and results in the following behaviour:
 1. GLib mainloop will be woken up for signals
 2. Exceptions in python code will bubble up to the mainloop (ie. if an operation is running then it will be aborted, but the mainloop is not quit).
 3. Exceptions happening during the mainloop execution (ie. in the wakeup handler) will make the mainloop quit

Note that this only works if python knows how to quit the mainloop. I tried to properly use the current main context, but that failed because of binding issues (in GLib.MainContext, GLib.IOChannel, ...). So the code uses "None" (ie. default mainloop).

Note that doing this requires:
 1. Creating a pair of FDs for the notification
 2. Adding a GLib source based on the FDs
 3. Hooking into the sys.excepthook to catch the exception, store it and then try to quit the mainloop
 4. Wrapping Gtk.main (and similar) so that it will reraise the exception, and stores a function to quit the mainloop.

You can try by running the program. When pressing Ctrl+C while it is in time.sleep() the sleep will abort. If it is in the mainloop, the application will quit. You can also override the SIGINT handler from python and then the mainloop is never quit (but still woken up to handle the event right away).

Functions that call mainloops recursively should be wrapped if possible. For example for Gtk.Dialog.run() could be wrapped like this (closing the dialog with Gtk.ResponseType.NONE):

def decorate_gtk_dialog(function):
    def decorated_function(dialog):
        def close_dialog():
            dialog.response(Gtk.ResponseType.NONE)

        run_mainloop(function, close_dialog, None, dialog)

    return decorated_function
Gtk.Dialog.run = decorate_gtk_dialog(Gtk.Dialog.run)

Bug 695683 is related to this in it is the same problem but with GLib MainLoops (which works with Ctrl+C to a limited degree). Previously, GLib.MainLoop was working pretty good but there was some fallout from switching it to use unix_signal_source_new when removing the static bindings. I added a proposed patch which still suffers problems, but it contains an "InterruptibleLoopContext" context manager which could be used as the basis for what ever is needed here.

Created attachment 243561 [details] [review]
Add InterruptibleLoopContext for managing SIGINT

Add InterruptibleLoopContext context manager for wrapping main loop
calls with GLib SIGINT handling for Python.

Created attachment 243562 [details] [review]
Use InterruptibleLoopContext with Gtk.main

Wrap Gtk.main to use InterruptibleLoopContext. Add test which
verifies SIGINT exits with KeyboardInterrupt.

Created attachment 243563 [details] [review]
Use InterruptibleLoopContext with Gio.Application.run

Wrap calls to Gio.Application.run with override which handles SIGINT
and calls "release" on the app. Add tests for both
Gio.Application.run and Gtk.Application.run.

Review of attachment 243562 [details] [review]:

Rejecting on the grounds that we should not add overrides which change the default behaviour of GLib. Rather we should provide the API to accomplish what is necessary through an explicit API. In this case developers can use GLib.unix_signal_add explicitly or we could provide something like "InterruptibleLoopContext" if it is actually more convenient/portable.

Review of attachment 243563 [details] [review]:

Rejecting on the grounds that we should not add overrides which change the default behaviour of GLib. Rather we should provide the API to accomplish what is necessary through an explicit API. In this case developers can use GLib.unix_signal_add explicitly or we could provide something like "InterruptibleLoopContext" if it is actually more convenient/portable.

Leaving patch "Add InterruptibleLoopContext for managing SIGINT" for review because it has a bit of cleanup for our already overridden behaviour of GLib.MainLoop. It can also be explicitly used by developers for situations like Gtk.Main or Gio.Application.run (although it could probably use a better name).

Adding InterruptibleLoopContext to GLib seems like a good idea to me, even if it is not used by default. No sense requiring developers to create their own boilerplate for this.

I'm confused now.

What's the officially endorsed way to make my Python GTK app close on Ctrl+C?

A take on summarizing this issue:

When starting Python it installs a SIGINT signal handler by default which when
invoked sets a flag and on the next Python execution calls the signal handler
which, by default, raises KeyboardInterrupt.

Since there is no Python code executed when Gtk.main() is idling the signal is
never detected and the handler never called.

This can be fixed by the approach Benjamin has proposed:
https://bugzilla.gnome.org/show_bug.cgi?id=622084#c17

In addition to setting the flag, Python will then push same data through the
fd passed to set_wakeup_fd(), which wakes up the mainloop and executes Python
code, and as a result triggers the signal handler. (Instead of os.pipe() we
should use socket.socketpair() instead since that also works under Windows
with Python 3.5+ now)

----

The question now is if we can do what pygtk did (and what Simon implemented).
Set up a default signal handler for Gtk.main() to call Gtk.main_quit() and call
the Python handler afterwards, which raises KeyboardInterrupt.

The problem here is that (due to the complicated pipe/socket code otherwise
needed) everyone uses GLib.unix_signal_add to catch the signal and start the
application shutdown process. If we'd do the same or use a Python handler, we
would either run both or replace the glib one, breaking existing applications.

One hack would be to use "ctypes.pythonapi.PyOS_getsig(2)" and check on
import, before calling Gtk.main() and when the Python signal handler is
called, whether the C-level signal handler has changed in the meantime, and if
that's the case skip calling Gtk.main_quit() and raising any exceptions. And
by adding/removing a dummy glib unix source we could even check if
g_unix_signal_add() was called before pygobject was imported. So we could
basically only main_quit() and raise if unix_signal_add() or signal.signal()
is never used.

----

Ignoring the above hack for now, what we definitely should do is the mainloop
wakeup at signal time by default, which would make at least this code possible
(also works on Windows for Python 3.5+):

    signal.signal(signal.SIGINT, lambda *args: Gtk.main_quit())
    Gtk.main()

I'm a bit worried about the PyOS_getsig() hack, but on the other hand I think
that Gtk.main() ignoring ctrl+c by default is a big hurdle, especially for
beginners.

If there is anything I've missed please correct me.

Created attachment 363946 [details] [review]
Make Python OS signal handlers run when an event loop is idling

When Python receives a signal such as SIGINT it sets a flag and will execute
the registered signal handler on the next call to PyErr_CheckSignals().
In case the main thread is blocked by an idling event loop (say Gtk.main()
or Gtk.Dialog.run()) the check never happens and the signal handler
will not get executed.

To work around the issue use signal.set_wakeup_fd() to wake up the active
event loop when a signal is received, which will invoke a Python callback
which will lead to the signal handler being executed.

This patch enables it in overrides for Gtk.main(), Gtk.Dialog.run(),
Gio.Application.run() and GLib.MainLoop.run().

Works on Unix, and on Windows with Python 3.5+.

With this fix in place it is possible to have a cross platform way to
react to SIGINT (GLib.unix_signal_add() worked, but not on Windows),
for example:

    signal.signal(signal.SIGINT, lambda *args: Gtk.main_quit())
    Gtk.main()

Created attachment 363965 [details] [review]
Make Python OS signal handlers run when an event loop is idling

As pointed out by Benjamin on IRC, Python <3.4 does not set CLO_EXEC on the sockets. I added a ensure_socket_not_inheritable() for that.

Review of attachment 363965 [details] [review]:

I think the GLib.io_add_watch should only include IN, HUP, ERR, NVAL. It might be sensible to just throw an error instead of returning False (I have not checked, but I expect an exception will be considered "False" and the source is removed). I think an error message would be good, the most likely error I can imagine is that something closed the wrong FD.

I realised that the patch does not work with the normal GLib mainloop. One *could* do this by attaching _wakeup_fd_is_active to the relevant context. However, this does not seem very helpful overall.

Nitpicks:
 * I don't think there is a need to call get_inheritable (_ossighelper.py:33)
 * Maybe move the check for _wakeup_fd_is_active further up in wakeup_on_signal
   (_ossighelper.py:79)
 * One could share the failure handling code between the tests

(In reply to Benjamin Berg from comment #31)
> Review of attachment 363965 [details] [review] [review]:

Thanks

> I think the GLib.io_add_watch should only include IN, HUP, ERR, NVAL. It
> might be sensible to just throw an error instead of returning False (I have
> not checked, but I expect an exception will be considered "False" and the
> source is removed). I think an error message would be good, the most likely
> error I can imagine is that something closed the wrong FD.

Printing an error sounds good.

> I realised that the patch does not work with the normal GLib mainloop. One
> *could* do this by attaching _wakeup_fd_is_active to the relevant context.
> However, this does not seem very helpful overall.

Could you be more specific what wouldn't work? I now realize that in case the main context is not in the main thread (is that possible? I've never used per thread contexts) it wouldn't work. I'll test that.

> Nitpicks:
>  * I don't think there is a need to call get_inheritable (_ossighelper.py:33)

My thinking was that set_inheritable() might not be atomic, so even setting the same value might add a race. I'll look at the implementation.

>  * Maybe move the check for _wakeup_fd_is_active further up in
> wakeup_on_signal

I tried to reduce the yield cases

>    (_ossighelper.py:79)
>  * One could share the failure handling code between the tests

will do

(In reply to Christoph Reiter (lazka) from comment #32)
> > I realised that the patch does not work with the normal GLib mainloop. One
> > *could* do this by attaching _wakeup_fd_is_active to the relevant context.
> > However, this does not seem very helpful overall.
> 
> Could you be more specific what wouldn't work? I now realize that in case
> the main context is not in the main thread (is that possible? I've never
> used per thread contexts) it wouldn't work. I'll test that.

In GTK+, we always use the default main context and the mainloop is required to run in the main thread, so everything will just work.

I was contemplating the case of someone actually using the more obscure GLib MainLoop/MainContext features (extremely unlikely).

My conclusion is, that any support for GLib.MainLoop.run should simply check that it is running the default GMainContext (GLib.main_context_default). If it is not, then there are too many corner cases to get right and it is simply not worth the effort.

> > Nitpicks:
> >  * I don't think there is a need to call get_inheritable (_ossighelper.py:33)
> 
> My thinking was that set_inheritable() might not be atomic, so even setting
> the same value might add a race. I'll look at the implementation.

Ah, the cpython code looks entirely safe in this regard (sets atomically if possible and falls back to do the same check internally).

> >  * Maybe move the check for _wakeup_fd_is_active further up in
> > wakeup_on_signal
> 
> I tried to reduce the yield cases

Fine with me. It just seemed to me that the "common" case was quite hidden in the code.

> >    (_ossighelper.py:79)
> >  * One could share the failure handling code between the tests
> 
> will do

Thanks!

Created attachment 364286 [details] [review]
Make Python OS signal handlers run when an event loop is idling

Created attachment 364325 [details] [review]
Install a default SIGINT handler for functions which start an event loop

Currently ctrl+c on a program blocked on Gtk.main() will raise an exception
but not return control. While it's easy to set up the proper signal handling and
stop the event loop or execute some other application shutdown code
it's nice to have a good default behaviour for small prototypes/examples
or when testing some code in an interactive console.

This adds a context manager which registeres a SIGINT handler only in case
the default Python signal handler is active and restores the original handle
afterwards. Since signal handlers registered through g_unix_signal_add()
are not detected by Python's signal module we use PyOS_getsig() through ctypes
to detect if the signal handler is changed from outside.

In case of nested event loops, all of them will be aborted.
In case an event loop is started in a thread, nothing will happen.

The context manager is used in the overrides for Gtk.main(), Gtk.Dialog.run(),
Gio.Application.run() and GLib.MainLoop.run()

This also fixes GLib.MainLoop.run() replacing a non-default signal handler
and not restoring the default one:
    https://bugzilla.gnome.org/show_bug.cgi?id=698623

Created attachment 364818 [details] [review]
Make Python OS signal handlers run when an event loop is

Small fix to make the io watch stop in case the socket returns empty bytes, this happens when the socket gets closed (shouldn't happen here, but who knows)

Created attachment 364819 [details] [review]
Install a default SIGINT handler for functions which

rebased

Any concerns or objections?

I think both patches are good as is.

This seems to have been discussed enough, and while the solution is not perfect (e.g. exceptions?) it is practical and a big improvement. Also, my guess is that merging them won't block further improvements.

Cool, thanks Benjamin for all your help/feedback.

If anyone ends up here because this broke/changed something, please open a bug report. It should not break any existing setup.