Bazaar

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

  1. sigwinch-without-signalmodule-583941
  2. Merge into 2.1
Proposed by Andrew Bennetts
Status: Rejected
Rejected by: Andrew Bennetts
Proposed branch: lp:~spiv/bzr/sigwinch-without-signalmodule-583941
Merge into: lp:bzr/2.1
Diff against target: 220 lines (+143/-13)
4 files modified
NEWS (+4/-5)
bzrlib/_sigwinch_handler_c.c (+112/-0)
bzrlib/osutils.py (+22/-7)
setup.py (+5/-1)
To merge this branch: bzr merge lp:~spiv/bzr/sigwinch-without-signalmodule-583941
Reviewer Review Type Date Requested Status
Andrew Bennetts Disapprove
Martin Packman (community) Abstain
Review via email: mp+25870@code.launchpad.net

Commit message

Implement pure C SIGWINCH signal handler to avoid EINTR bugs (#583941)

Description of the change

EINTR, round 99. I believe that this should fix most if not all occurrences of EINTR with the 'bzr' command.

Basically, we cannot catch SIGWINCH in CPython <= 2.6.5 (i.e. any currently released version of Python) without causing EINTR in many places, many of which are in the standard library and hence impossible to fix in bzrlib.

However, we want to get notified when the terminal size changes, which means either asking the OS every time we want to know the terminal size (perhaps not so bad?), or side-stepping the 'signal' module in Python. This patch takes the latter route. It implements a very simple C module that can install a SIGWINCH handler with the SA_RESTART flag (thus avoiding EINTR from the majority of syscalls). That handler simply sets a boolean flag. The module also provides a function to check and reset that flag. If the module isn't available bzrlib.osutils.terminal_width will fallback to asking the OS for the terminal size every time.

The has_function check in setup.py is a bit unfortunate, because it will be triggered on every setup.py invocation (even those that don't try to build extensions), and it can produce some noise on stderr. It would be possible to fix this by pushing the check into a custom Extension subclass or perhaps the overridden build_ext command, but that would be more complex. We can do something about that if it causes trouble, but for now I'm hoping simple is good enough.

To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Why does the has_function cause spew?

Also, why do we override COLUMNS when we get sigwinch, thats non obvious to me.

 I would needs info but no gpg for me in gmail!

Revision history for this message
Martin Pool (mbp) wrote :

On 24 May 2010 16:49, Robert Collins <email address hidden> wrote:

>  I would needs info but no gpg for me in gmail!

https://bugs.edge.launchpad.net/launchpad-registry/+bug/316272 has a
fix underway! Comments on the LEP welcome.

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

Revision history for this message
Martin Pool (mbp) wrote :

On 24 May 2010 16:02, Andrew Bennetts <email address hidden> wrote:

> However, we want to get notified when the terminal size changes, which means either asking the OS every time we want to know the terminal size (perhaps not so bad?), or side-stepping the 'signal' module in Python.  This patch takes the latter route.

Perhaps we should try the former route? (I'm kind of sorry to say
that after you've already done this, but on the other hand history
suggests this may not be the final fix, and it's not all that small.)

I suppose the only drawback is that it may slow us down if we do it
really often. On the other hand I think the progress code already
tries to throttle updates to some extent. Could you try something
with timeit to see just how expensive it would be?

Revision history for this message
Andrew Bennetts (spiv) wrote :

Robert Collins wrote:
> Why does the has_function cause spew?

Why indeed! Here's what happens if I tweak the check to look for
NO_SUCH_FUNCTION rather than sigaction:

$ make
building extension modules.
python setup.py build_ext -i
tmp/NO_SUCH_FUNCTION1nRZ2y.o: In function `main':
NO_SUCH_FUNCTION1nRZ2y.c:(.text+0x7): undefined reference to `NO_SUCH_FUNCTION'
collect2: ld returned 1 exit status
running build_ext

> Also, why do we override COLUMNS when we get sigwinch, thats non obvious to me.

I'm not sure, but as I was targetting this fix to 2.1.x I thought it best to
disturb the existing behaviour as little as possible.

Revision history for this message
Andrew Bennetts (spiv) wrote :

Martin Pool wrote:
> On 24 May 2010 16:02, Andrew Bennetts <email address hidden> wrote:
>
> > However, we want to get notified when the terminal size changes,
> > which means either asking the OS every time we want to know the
> > terminal size (perhaps not so bad?), or side-stepping the 'signal'
> > module in Python.  This patch takes the latter route.
>
> Perhaps we should try the former route? (I'm kind of sorry to say
> that after you've already done this, but on the other hand history
> suggests this may not be the final fix, and it's not all that small.)
>
> I suppose the only drawback is that it may slow us down if we do it
> really often. On the other hand I think the progress code already
> tries to throttle updates to some extent. Could you try something
> with timeit to see just how expensive it would be?

I'll take a look. I can't easily time the different code path taken by
win32, but obviously SIGWINCH never applied there anyway.

I know the _ioctl_terminal_size isn't going to be über-efficient, being
an ioctl and struct packing and unpacking, but as you say we probably
don't do it often.

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr/sigwinch-without-signalmodule-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
>
>
> EINTR, round 99. I believe that this should fix most if not all occurrences of EINTR with the 'bzr' command.

I didn't read the code, but could you do what you need with ctypes
instead of writing a custom extension?

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkv60msACgkQJdeBCYSNAAOBcwCgiEu1w5yM84ZMz3KOPOPMtaHK
n8kAn0Ck0oV5ArHCRMYcVOMUgxF0Mz+N
=JN1C
-----END PGP SIGNATURE-----

Revision history for this message
Andrew Bennetts (spiv) wrote :

John A Meinel wrote:
[...]
> I didn't read the code, but could you do what you need with ctypes
> instead of writing a custom extension?

I don't think so. Although ctypes allows you to define a Python
function for a C function prototype, I wouldn't expect that to be safe
to use as a signal handler. It would presumably attempt to run the
Python function whenever the signal arrived, without taking any care
about already being mid-way through the ceval loop, or garbage
collection, or the GIL being held by a different thread, etc.

Revision history for this message
Martin Packman (gz) wrote :

Thanks for writing this, it's interesting to see. It's unfortunate we can't do the work that actually needs to happen in response to the signal (setting the COLUMNS envvar) as it's not an atomic operation. So, this essentially moves us back to polling, at which point, why bother with signals at all? Moving the width variable to a Python int wouldn't help either.

A few misc comments on the patch:

+if compiler.has_function('sigaction'):

Think that should be instead:

    if compiler.has_function('sigaction', includes=['signal.h']):

Also, putting it in the not-win32 section would at least mean no stderr kipple for me. ;)

Have been reading ncurses source, and they check for SA_RESTART on top of sigaction. Probably not something to worry about, have fewer platforms and less history with Bazaar.

review: Abstain
Revision history for this message
Martin Packman (gz) wrote :

> Moving the width variable to a Python int wouldn't help either.

I'm missing the point somewhat here, that problem could be worked around. What can't be is the need to use ioctl to get the actual number, and am pretty sure that does shocking things like using malloc so isn't signal-reentrant.

Scheduling a Python function to run once the interpreter is restarted like the Python signal module would be another option, but I don't think there's a general way of doing that.

Revision history for this message
Andrew Bennetts (spiv) wrote :

 merge rejected

Martin [gz] wrote:
> Review: Abstain
> Thanks for writing this, it's interesting to see. It's unfortunate we can't do
> the work that actually needs to happen in response to the signal (setting the
> COLUMNS envvar) as it's not an atomic operation. So, this essentially moves us
> back to polling, at which point, why bother with signals at all? Moving the
> width variable to a Python int wouldn't help either.

Well, it changes the cost of osutils.terminal_width from invoking the
ioctl (and associated struct.pack/unpack) every time, to only when the
the width may have changed. So in the common case the poll is check
glancing at an int, rather than interrogating the OS. Micro-benchmarks
with timeit suggest this is about 200x faster.

Martin P makes a good point that we don't really check the
terminal_width often enough for this matter though --
_ioctl_terminal_size only takes 11 microseconds on my fairly slow
laptop, and we simply don't call it very often.

So I'm going to abandon this patch (even though I'm quite confident in
its correctness) and instead propose one to take out all SIGWINCH code
entirely.

> A few misc comments on the patch:
>
> +if compiler.has_function('sigaction'):
>
> Think that should be instead:
>
> if compiler.has_function('sigaction', includes=['signal.h']):
>
> Also, putting it in the not-win32 section would at least mean no stderr kipple for me. ;)

Good points, something I'll keep in mind for next time :)

> Have been reading ncurses source, and they check for SA_RESTART on top
> of sigaction. Probably not something to worry about, have fewer
> platforms and less history with Bazaar.

That was my hope too ;)

review: Disapprove

Unmerged revisions

4847. By Andrew Bennetts

Cosmetic tweaks.

4846. By Andrew Bennetts

Add some docstrings, and only reset _terminal_size_changed if it was set.

4845. By Andrew Bennetts

Use consistent whitespace .c file.

4844. By Andrew Bennetts

Add pure C signal handler for SIGWINCH, avoiding bugs in CPython 2.6.[0-5] (and the absence of signal.siginterrupt in CPython < 2.6).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'NEWS'
--- NEWS 2010-05-21 01:55:34 +0000
+++ NEWS 2010-05-24 06:02:25 +0000
@@ -24,11 +24,10 @@
24 versions before 1.6.24 versions before 1.6.
25 (Andrew Bennetts, #528041)25 (Andrew Bennetts, #528041)
2626
27* Reset ``siginterrupt`` flag to False every time we handle a signal27* Implemented ``SIGWINCH`` signal handler in C, avoiding bugs and
28 installed with ``set_signal_handler(..., restart_syscall=True)`` (from28 limitations in various versions of Python. This fixes the major cause
29 ``bzrlib.osutils``. Reduces the likelihood of "Interrupted System Call"29 of "Interrupted System Call" errors.
30 errors after two window resizes.30 (Andrew Bennetts, #583941)
31 (Andrew Bennetts)
3231
33* Reduce peak memory by one copy of compressed text.32* Reduce peak memory by one copy of compressed text.
34 (John Arbash Meinel, #566940)33 (John Arbash Meinel, #566940)
3534
=== added file 'bzrlib/_sigwinch_handler_c.c'
--- bzrlib/_sigwinch_handler_c.c 1970-01-01 00:00:00 +0000
+++ bzrlib/_sigwinch_handler_c.c 2010-05-24 06:02:25 +0000
@@ -0,0 +1,112 @@
1/* Copyright (C) 2010 Canonical Ltd
2 *
3 * This program is free software; you can redistribute it and/or modify
4 * it under the terms of the GNU General Public License as published by
5 * the Free Software Foundation; either version 2 of the License, or
6 * (at your option) any later version.
7 *
8 * This program is distributed in the hope that it will be useful,
9 * but WITHOUT ANY WARRANTY; without even the implied warranty of
10 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11 * GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License
14 * along with this program; if not, write to the Free Software
15 * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
16 */
17
18/* Minimal handler for SIGWINCH.
19 *
20 * Works on any platform with sigaction, regardless of Python version.
21 */
22
23#include <Python.h>
24#include <signal.h>
25
26/* A flag to record if a SIGWINCH has happened since we last checked. Note that
27 * we don't bother to do any synchronization when setting or getting this value.
28 */
29
30static int _terminal_size_changed;
31
32static void sigwinch_handler(int signum)
33{
34 _terminal_size_changed = 1;
35}
36
37static char py_install_doc[] =
38 "Install simple SIGWINCH handler.\n"
39 "\n"
40 "After calling install(), you can find out if a SIGWINCH has occurred by\n"
41 "calling terminal_size_changed().\n";
42
43static PyObject *
44py_install(PyObject *ignored1, PyObject *ignored2)
45{
46 struct sigaction new_action, old_action;
47 PyObject *res;
48 _terminal_size_changed = 0;
49 new_action.sa_handler = sigwinch_handler;
50 sigemptyset(&(new_action.sa_mask));
51 new_action.sa_flags = SA_RESTART;
52 if (sigaction(SIGWINCH, &new_action, &old_action) != 0) {
53 return PyErr_SetFromErrno(PyExc_OSError);
54 }
55 res = Py_None;
56 Py_INCREF(res);
57 return res;
58}
59
60static char py_terminal_size_changed_doc[] =
61 "Has SIGWINCH occured since the last call to terminal_size_changed()?\n"
62 "\n"
63 "Returns True or False. If True, the value will be reset to False until\n"
64 "the next occurence of SIGWINCH.\n";
65
66static PyObject *
67py_terminal_size_changed(PyObject *ignored1, PyObject *ignored2)
68{
69 PyObject *res;
70 int has_changed;
71 has_changed = _terminal_size_changed;
72 /* Note that it doesn't matter if SIGWINCH occurs after that assignment but
73 * before the end of this function.
74 * If SIGWINCH occurs then either:
75 * - has_changed is set and we'll return True and reset
76 * _terminal_size_changed anyway, or
77 * - has_changed is not set, we'll return False and the SIGWINCH will be
78 * reported on the next call to py_terminal_size_changed.
79 */
80 res = PyBool_FromLong(has_changed);
81 if (!res) {
82 return res;
83 }
84 if (has_changed) {
85 /* Reset _terminal_size_changed */
86 _terminal_size_changed = 0;
87 }
88 return res;
89}
90
91static PyMethodDef sigwinch_handler_methods[] = {
92 {"install", py_install, METH_NOARGS, py_install_doc},
93 {"terminal_size_changed", py_terminal_size_changed, METH_NOARGS,
94 py_terminal_size_changed_doc},
95 {NULL, NULL}
96};
97
98PyMODINIT_FUNC
99init_sigwinch_handler_c(void)
100{
101 PyObject* m;
102 _terminal_size_changed = 0;
103 m = Py_InitModule3("_sigwinch_handler_c", sigwinch_handler_methods,
104 "Minimal handler for SIGWINCH.\n"
105 "\n"
106 "Call install(), then call terminal_size_changed() to check if a\n"
107 "SIGWINCH has occurred since the last check.");
108 if (m == NULL)
109 return;
110}
111
112// vim: tabstop=4 sw=4 expandtab
0113
=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py 2010-04-30 06:27:15 +0000
+++ bzrlib/osutils.py 2010-05-24 06:02:25 +0000
@@ -1423,6 +1423,7 @@
1423 # If COLUMNS is set, take it, the terminal knows better (even inside a1423 # If COLUMNS is set, take it, the terminal knows better (even inside a
1424 # given terminal, the application can decide to set COLUMNS to a lower1424 # given terminal, the application can decide to set COLUMNS to a lower
1425 # value (splitted screen) or a bigger value (scroll bars))1425 # value (splitted screen) or a bigger value (scroll bars))
1426 _check_for_sigwinch()
1426 try:1427 try:
1427 return int(os.environ['COLUMNS'])1428 return int(os.environ['COLUMNS'])
1428 except (KeyError, ValueError):1429 except (KeyError, ValueError):
@@ -1466,18 +1467,26 @@
1466 _terminal_size = _ioctl_terminal_size1467 _terminal_size = _ioctl_terminal_size
14671468
14681469
1469def _terminal_size_changed(signum, frame):1470def _check_for_sigwinch():
1470 """Set COLUMNS upon receiving a SIGnal for WINdow size CHange."""1471 """Set COLUMNS upon receiving a SIGnal for WINdow size CHange.
1471 width, height = _terminal_size(None, None)1472
1472 if width is not None:1473 If _sigwinch_handler_c is not installed, this queries the OS for the size
1473 os.environ['COLUMNS'] = str(width)1474 every time.
1475 """
1476 if (not _have_sigwinch_handler_c or
1477 _sigwinch_handler_c.terminal_size_changed()):
1478 width, height = _terminal_size(None, None)
1479 if width is not None:
1480 os.environ['COLUMNS'] = str(width)
14741481
14751482
1476_registered_sigwinch = False1483_registered_sigwinch = False
1484_have_sigwinch_handler_c = False
1485_sigwinch_handler_c = None
14771486
1478def watch_sigwinch():1487def watch_sigwinch():
1479 """Register for SIGWINCH, once and only once."""1488 """Register for SIGWINCH, once and only once."""
1480 global _registered_sigwinch1489 global _registered_sigwinch, _have_sigwinch_handler_c, _sigwinch_handler_c
1481 if not _registered_sigwinch:1490 if not _registered_sigwinch:
1482 if sys.platform == 'win32':1491 if sys.platform == 'win32':
1483 # Martin (gz) mentioned WINDOW_BUFFER_SIZE_RECORD from1492 # Martin (gz) mentioned WINDOW_BUFFER_SIZE_RECORD from
@@ -1485,7 +1494,13 @@
1485 # the current design -- vila 200912161494 # the current design -- vila 20091216
1486 pass1495 pass
1487 else:1496 else:
1488 set_signal_handler(signal.SIGWINCH, _terminal_size_changed)1497 try:
1498 from bzrlib import _sigwinch_handler_c
1499 except ImportError:
1500 pass
1501 else:
1502 _sigwinch_handler_c.install()
1503 _have_sigwinch_handler_c = True
1489 _registered_sigwinch = True1504 _registered_sigwinch = True
14901505
14911506
14921507
=== modified file 'setup.py'
--- setup.py 2010-03-07 13:33:37 +0000
+++ setup.py 2010-05-24 06:02:25 +0000
@@ -96,7 +96,7 @@
96BZRLIB['packages'] = get_bzrlib_packages()96BZRLIB['packages'] = get_bzrlib_packages()
9797
9898
99from distutils import log99from distutils import ccompiler, log
100from distutils.core import setup100from distutils.core import setup
101from distutils.command.install_scripts import install_scripts101from distutils.command.install_scripts import install_scripts
102from distutils.command.install_data import install_data102from distutils.command.install_data import install_data
@@ -317,6 +317,10 @@
317 ext_modules.append(Extension('bzrlib._static_tuple_c',317 ext_modules.append(Extension('bzrlib._static_tuple_c',
318 ['bzrlib/_static_tuple_c.c']))318 ['bzrlib/_static_tuple_c.c']))
319add_pyrex_extension('bzrlib._btree_serializer_pyx')319add_pyrex_extension('bzrlib._btree_serializer_pyx')
320compiler = ccompiler.new_compiler()
321if compiler.has_function('sigaction'):
322 ext_modules.append(Extension('bzrlib._sigwinch_handler_c',
323 ['bzrlib/_sigwinch_handler_c.c']))
320324
321325
322if unavailable_files:326if unavailable_files:

Subscribers

People subscribed via source and target branches