Merge lp:~spiv/bzr/i-hate-signals into lp:bzr

[Andrew Bennetts]

Python resets the siginterrupt flag when it invokes a pure-python signal handler. So this patch sets it back again.

This seems to be a bug in upstream Python (in their docs if nothing else), I'll submit a bug report soon.

[Martin Packman (gz)]

Abstaining so I get CCed until I have a spare moment to look at this properly.

Of somewhat related interest on python-dev today:
<http://mail.python.org/pipermail/python-dev/2010-April/099165.html>
<http://bugs.python.org/issue7978>

review: Abstain

[Andrew Bennetts (spiv)]

I filed <http://bugs.python.org/issue8354> about this.

I should clarify that this patch doesn't fully workaround that bug, but it helps in the case that there's some IO between delivery of signals (so that my signal handler gets a chance to redo the siginterrupt call that Python undoes). The only relevant signal for current bzr is SIGWINCH, so IO just needs to happen more often than a window resize event, which is plausible — it did fix the immediate issue that I ran into as a user.

[Martin Packman (gz)]

Okay, been through this, the upstream bug, and the signal module C code.
Diagnosis looks fine, as does the proposed patch to the C signal handler in the Python source, would be good to get that landed.

One thing I'm not clear on is how effective the workaround in this merge will be. As the Python handler won't run till the IO function returns, and we've prevented it returning early, there can (and in common cases will be) whole seconds elapsed before say, `sock.read(65536)` completes. Do window managers fire multiple SIGWINCHes if a terminal emulator is dragged wider, or do they tend to wait for the release?

review: Needs Information

[Andrew Bennetts (spiv)]

It probably depends on the window manager: some have you drag an outline (or static snapshot) of the window, some do "live" resizing (and some can be configured either way). For me the most common way I resize a terminal is by maximising or unmaximising a GNOME Terminal, which thankfully only seems to trigger one SIGWINCH per (un)maximise.

There can be other ways to trigger a resize too, like opening a second tab in a maximised GNOME Terminal window: the creation of the tab widget consumes some screen real estate, so that's a terminal resize too. Opening a second tab and (un)maximising are relatively frequent operations for me.

I'm sure there are many real-world cases this workaround can't fix, sadly. But I can say that it definitely improved behaviour for me in some real-world cases, e.g. a 'bzr pull' over plain HTTP.

[Martin Pool (mbp)]

Tangentially, we do have the option to say that sigwinch is just too
much trouble and instead poll the window size when we need it. But
polling every time we update progress may be too often, and doing it
infrequently may cause mess when the window is resized.

Alternatively we could hook that signal from C, and make it just
update a global accessible from python.

--
Martin <http://launchpad.net/~mbp/>

[Martin Packman (gz)]

Okay, so there are common situations this workaround-for-a-workaround will help with then. Can't test here, but diff looks fine to me. Oh, and I sympathise with the branch name.

review: Approve

[Andrew Bennetts (spiv)]

Martin P: I think avoiding SIGWINCH, or at least a Python handler for it, is worth exploring. Although there's a reasonable-looking fix attached to the upstream bug, which would make the existing Python SIGWINCH handler just fine.

In the meantime, I'll merge this partial workaround because it is better than the status quo.

=== modified file 'NEWS'
--- NEWS	2010-04-14 04:35:47 +0000
+++ NEWS	2010-04-14 06:49:37 +0000
@@ -40,6 +40,12 @@
   which is not installed any more" error.
   (Martin Pool, James Westby, #528114)
 
+* Reset ``siginterrupt`` flag to False every time we handle a signal
+  installed with ``set_signal_handler(..., restart_syscall=True)`` (from
+  ``bzrlib.osutils``.  Reduces the likelihood of "Interrupted System Call"
+  errors after two window resizes.
+  (Andrew Bennetts)
+
 * When invoked with a range revision, ``bzr log`` doesn't show revisions
   that are not part of the ancestry anymore.
   (Vincent Ladeuil, #474807)

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-03-26 04:47:45 +0000
+++ bzrlib/osutils.py	2010-04-14 06:49:37 +0000
@@ -1365,15 +1365,25 @@
         False)`).  May be ignored if the feature is not available on this
         platform or Python version.
     """
-    old_handler = signal.signal(signum, handler)
+    try:
+        siginterrupt = signal.siginterrupt
+    except AttributeError:
+        # siginterrupt doesn't exist on this platform, or for this version
+        # of Python.
+        siginterrupt = lambda signum, flag: None
     if restart_syscall:
-        try:
-            siginterrupt = signal.siginterrupt
-        except AttributeError: # siginterrupt doesn't exist on this platform, or for this version of
-            # Python.
-            pass
-        else:
+        def sig_handler(*args):
+            # Python resets the siginterrupt flag when a signal is
+            # received.  <http://bugs.python.org/issue8354>
+            # As a workaround for some cases, set it back the way we want it.
             siginterrupt(signum, False)
+            # Now run the handler function passed to set_signal_handler.
+            handler(*args)
+    else:
+        sig_handler = handler
+    old_handler = signal.signal(signum, sig_handler)
+    if restart_syscall:
+        siginterrupt(signum, False)
     return old_handler