Merge lp:~spiv/bzr/no-sigwinch-583941 into lp:bzr/2.1

[Andrew Bennetts]

Remove the SIGWINCH handler to avoid EINTR problems. Instead poll the terminal size as needed. (#583941)

Final, simple fix for EINTR?

This simply removes the SIGWINCH handler introduced in 2.1.0rc1, and adjusts the osutils.terminal_width function accordingly.

I'm a little concerned that the logic inside terminal_width looks somewhat different now, because it tries _terminal_size before it tries $COLUMNS — but, strangely, the SIGWINCH handler had actually been setting $COLUMNS, so I don't think this is actually so different. And I do think the new code is clearer. I'd welcome second opinions though.

Finally, here's the diffstat to whet your appetite:

$ bzr diff | diffstat
 NEWS | 8 +++++++-
 bzrlib/osutils.py | 42 +++++++++---------------------------------
 bzrlib/ui/text.py | 4 ----
 3 files changed, 16 insertions(+), 38 deletions(-)

As with the previous proposal for this, I've tested interactively that e.g. bzr log -n0 from a large HTTP branch continues successfully while the terminal is rapidly resized, unlike before.

[Robert Collins (lifeless)]

DoIt - love to get this behind us

[Vincent Ladeuil (vila)]

>>>>> Andrew Bennetts <email address hidden> writes:

    > Andrew Bennetts has proposed merging lp:~spiv/bzr/no-sigwinch-583941 into lp:bzr/2.1.
    > Requested reviews:
    > bzr-core (bzr-core)
    > Related bugs:
    > #583941 program crashes with "bzr: ERROR: socket.error: (4, 'Interrupted system call')"
    > https://bugs.launchpad.net/bugs/583941

    > Final, simple fix for EINTR?

    > This simply removes the SIGWINCH handler introduced in 2.1.0rc1,
    > and adjusts the osutils.terminal_width function accordingly.

    > I'm a little concerned that the logic inside terminal_width looks
    > somewhat different now, because it tries _terminal_size before it
    > tries $COLUMNS — but, strangely, the SIGWINCH handler had actually
    > been setting $COLUMNS, so I don't think this is actually so
    > different.

qblame to the rescue, have a look at 4747.4.3
(revid:<email address hidden>[1]):

,----
| * bzrlib/osutils.py:
| (terminal_width): COLUMNS takes priority on termios.TIOCGWINSZ as
| shown by tests under emacs (on no counter examples are known).
`----

This is indeed a known case where COLUMNS should take precedence. And
the comment you deleted was trying to hint at that. If you find a better
wording, please feel free to update it but don't just delete the hints
about the rationale.

Unfortunately, this couldn't be translated into a test since the tests
are supposed to be runned even without a controlling terminal ;(

We should check the env variable first *at least* when we start.

Now, if SIGWINCH occurs, we can't get an updated value for COLUMNS, so
we have to resort to _terminal_size (even if it's wrong now, it seems
less wrong than keeping a knowingly out-of-date COLUMNS) and setting
COLUMNS was the easiest way to go then.

[1]: Amazingly this revid includes the string 'bug' :)

[Martin Pool (mbp)]

That looks nice.

I'd like to check that running eg that log command does not take any longer with this patch.

I guess you can work out with vila what to do under emacs. I would guess that the ioctl just does not work there and so it's enough to use $COLUMNS if the ioctl fails, but it may be more complicated. Perhaps we can add a big comment about this.

review: Approve

[Andrew Bennetts (spiv)]

vila and I have had an extensive discussion on IRC. I'll try to write a comprehensive summary soon, but right now it's dinner time, so here's the short version:

1) I'll rework this patch for 2.1 to preserve the 2.1 behaviour as much as possible without using SIGWINCH (i.e. use COLUMNS until we observe the size from TIOCGWINSZ change), to minimise risk of regressions in 2.1.

2) I'll submit another patch for trunk that I think will be a better implementation of terminal_width: try $BZR_COLUMNS, then TIOCGWINSZ, then $COLUMNS in turn, until we find one that is set to a non-zero value. If none are set non-zero, return None. This patch will try to explicitly document the rationale and problematic cases to take into account, because the literature around determining terminal sizes is pretty confusing.

I'm popping this branch back to work-in-progress for now, but expect a resubmission soon.

[Martin Packman (gz)]

Looks good to me. There was a lot of detailed discussion on IRC about the specifics of getting the logic maximally compatible. Trying for a minimal change on 2.1 and investigating further for trunk sounds like a solid plan. So, this branch only really needs a tweak to make the initial result of _terminal_size weaker than COLUMNS before landing.

The question of speed has been raised a couple of times. Andrew timed _ioctl_terminal_size at 11μs and that can be trivially improved upon if needed. The common case could just call fcntl.ioctl and do a string comparison, no need for function-level imports or struct packing and unpacking.

review: Approve

[Andrew Bennetts (spiv)]

Here's the patch to remove SIGWINCH with minimal behaviour change for 2.1. I've added some comments that try to explain some of the requirement and rationale that was unclear before.

[Martin Pool (mbp)]

urk, all this to measure what should be a history-independent property
of the environment.

Still, that looks clean and matches my understanding of the
discussion. Nice comment too. Perhaps you should wait until Vincent
gets to check it.

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

[Andrew Bennetts (spiv)]

Martin Pool wrote:
> urk, all this to measure what should be a history-independent property
> of the environment.

Yes :(

> Still, that looks clean and matches my understanding of the
> discussion. Nice comment too. Perhaps you should wait until Vincent
> gets to check it.

I'll do that.

[Robert Collins (lifeless)]

Vincent, when you review it, if you're happy please *just land it* in 2.1 - so that I can release 2.1.2 tomorrow.

Cheers,
Rob

[Andrew Bennetts (spiv)]

btw, Martin P asked earlier:

> I'd like to check that running eg that log command does not take any longer with this patch.

I did some manual testing of "bzr log -r-100.. bzr.dev" (753 lines of output), and there's no noticeable difference. So that's good.

(As an aside, I notice that trunk is a little quicker than 2.1 on that task, hooray! About 320ms vs 340ms on my system.)

[Vincent Ladeuil (vila)]

Fine. Great job with the comments.

review: Approve

[Andrew Bennetts (spiv)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-05-21 01:55:34 +0000
+++ NEWS	2010-05-27 04:04:24 +0000
@@ -20,6 +20,12 @@
 * ``bzr switch`` does not die if a ConfigurableFileMerger is used.
   (Aaron Bentley, #559436)
 
+* Do not register a SIGWINCH signal handler, instead just poll for the
+  terminal width as needed.  This avoids the "Interrupted System Call"
+  problems that occur on POSIX with all currently released versions of
+  Python.
+  (Andrew Bennetts, #583941)
+
 * Fixed ``AssertionError`` when accessing smart servers running Bazaar
   versions before 1.6.
   (Andrew Bennetts, #528041)
@@ -27,7 +33,7 @@
 * 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.
+  errors compared to registering ``signal.signal`` directly.
   (Andrew Bennetts)
 
 * Reduce peak memory by one copy of compressed text.

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-04-30 06:27:15 +0000
+++ bzrlib/osutils.py	2010-05-27 04:04:24 +0000
@@ -1385,6 +1385,12 @@
 terminal_width() returns None.
 """
 
+# Keep some state so that terminal_width can detect if _terminal_size has
+# returned a different size since the process started.  See docstring and
+# comments of terminal_width for details.
+# _terminal_size_state has 3 possible values: no_data, unchanged, and changed.
+_terminal_size_state = 'no_data'
+_first_terminal_size = None
 
 def terminal_width():
     """Return terminal width.
@@ -1394,20 +1400,34 @@
     The rules are:
     - if BZR_COLUMNS is set, returns its value
     - if there is no controlling terminal, returns None
+    - query the OS, if the queried size has changed since the last query,
+      return its value,
     - if COLUMNS is set, returns its value,
+    - if the OS has a value (even though it's never changed), return its value.
 
     From there, we need to query the OS to get the size of the controlling
     terminal.
 
-    Unices:
+    On Unices we query the OS by:
     - get termios.TIOCGWINSZ
     - if an error occurs or a negative value is obtained, returns None
 
-    Windows:
-    
+    On Windows we query the OS by:
     - win32utils.get_console_size() decides,
     - returns None on error (provided default value)
     """
+    # Note to implementors: if changing the rules for determining the width,
+    # make sure you've considered the behaviour in these cases:
+    #  - M-x shell in emacs, where $COLUMNS is set and TIOCGWINSZ returns 0,0.
+    #  - bzr log | less, in bash, where $COLUMNS not set and TIOCGWINSZ returns
+    #    0,0.
+    #  - (add more interesting cases here, if you find any)
+    # Some programs implement "Use $COLUMNS (if set) until SIGWINCH occurs",
+    # but we don't want to register a signal handler because it is impossible
+    # to do so without risking EINTR errors in Python <= 2.6.5 (see
+    # <http://bugs.python.org/issue8354>).  Instead we check TIOCGWINSZ every
+    # time so we can notice if the reported size has changed, which should have
+    # a similar effect.
 
     # If BZR_COLUMNS is set, take it, user is always right
     try:
@@ -1416,24 +1436,39 @@
         pass
 
     isatty = getattr(sys.stdout, 'isatty', None)
-    if  isatty is None or not isatty():
+    if isatty is None or not isatty():
         # Don't guess, setting BZR_COLUMNS is the recommended way to override.
         return None
 
-    # If COLUMNS is set, take it, the terminal knows better (even inside a
-    # given terminal, the application can decide to set COLUMNS to a lower
-    # value (splitted screen) or a bigger value (scroll bars))
+    # Query the OS
+    width, height = os_size = _terminal_size(None, None)
+    global _first_terminal_size, _terminal_size_state
+    if _terminal_size_state == 'no_data':
+        _first_terminal_size = os_size
+        _terminal_size_state = 'unchanged'
+    elif (_terminal_size_state == 'unchanged' and
+          _first_terminal_size != os_size):
+        _terminal_size_state = 'changed'
+
+    # If the OS claims to know how wide the terminal is, and this value has
+    # ever changed, use that.
+    if _terminal_size_state == 'changed':
+        if width is not None and width > 0:
+            return width
+
+    # If COLUMNS is set, use it.
     try:
         return int(os.environ['COLUMNS'])
     except (KeyError, ValueError):
         pass
 
-    width, height = _terminal_size(None, None)
-    if width <= 0:
-        # Consider invalid values as meaning no width
-        return None
+    # Finally, use an unchanged size from the OS, if we have one.
+    if _terminal_size_state == 'unchanged':
+        if width is not None and width > 0:
+            return width
 
-    return width
+    # The width could not be determined.
+    return None
 
 
 def _win32_terminal_size(width, height):
@@ -1466,29 +1501,6 @@
     _terminal_size = _ioctl_terminal_size
 
 
-def _terminal_size_changed(signum, frame):
-    """Set COLUMNS upon receiving a SIGnal for WINdow size CHange."""
-    width, height = _terminal_size(None, None)
-    if width is not None:
-        os.environ['COLUMNS'] = str(width)
-
-
-_registered_sigwinch = False
-
-def watch_sigwinch():
-    """Register for SIGWINCH, once and only once."""
-    global _registered_sigwinch
-    if not _registered_sigwinch:
-        if sys.platform == 'win32':
-            # Martin (gz) mentioned WINDOW_BUFFER_SIZE_RECORD from
-            # ReadConsoleInput but I've no idea how to plug that in
-            # the current design -- vila 20091216
-            pass
-        else:
-            set_signal_handler(signal.SIGWINCH, _terminal_size_changed)
-        _registered_sigwinch = True
-
-
 def supports_executable():
     return sys.platform != "win32"
 

=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/test_osutils.py	2010-05-27 04:04:24 +0000
@@ -1928,6 +1928,18 @@
 
 class TestTerminalWidth(tests.TestCase):
 
+    def setUp(self):
+        tests.TestCase.setUp(self)
+        self._orig_terminal_size_state = osutils._terminal_size_state
+        self._orig_first_terminal_size = osutils._first_terminal_size
+        self.addCleanup(self.restore_osutils_globals)
+        osutils._terminal_size_state = 'no_data'
+        osutils._first_terminal_size = None
+
+    def restore_osutils_globals(self):
+        osutils._terminal_size_state = self._orig_terminal_size_state
+        osutils._first_terminal_size = self._orig_first_terminal_size
+        
     def replace_stdout(self, new):
         orig_stdout = sys.stdout
         def restore():

=== modified file 'bzrlib/ui/text.py'
--- bzrlib/ui/text.py	2010-03-23 06:45:56 +0000
+++ bzrlib/ui/text.py	2010-05-27 04:04:24 +0000
@@ -37,8 +37,6 @@
 
 """)
 
-from bzrlib.osutils import watch_sigwinch
-
 from bzrlib.ui import (
     UIFactory,
     NullProgressView,
@@ -62,8 +60,6 @@
         self.stderr = stderr
         # paints progress, network activity, etc
         self._progress_view = self.make_progress_view()
-        # hook up the signals to watch for terminal size changes
-        watch_sigwinch()
 
     def be_quiet(self, state):
         if state and not self._quiet: