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
1=== modified file 'NEWS'
2--- NEWS 2010-05-21 01:55:34 +0000
3+++ NEWS 2010-05-24 06:02:25 +0000
4@@ -24,11 +24,10 @@
5 versions before 1.6.
6 (Andrew Bennetts, #528041)
7
8-* Reset ``siginterrupt`` flag to False every time we handle a signal
9- installed with ``set_signal_handler(..., restart_syscall=True)`` (from
10- ``bzrlib.osutils``. Reduces the likelihood of "Interrupted System Call"
11- errors after two window resizes.
12- (Andrew Bennetts)
13+* Implemented ``SIGWINCH`` signal handler in C, avoiding bugs and
14+ limitations in various versions of Python. This fixes the major cause
15+ of "Interrupted System Call" errors.
16+ (Andrew Bennetts, #583941)
17
18 * Reduce peak memory by one copy of compressed text.
19 (John Arbash Meinel, #566940)
20
21=== added file 'bzrlib/_sigwinch_handler_c.c'
22--- bzrlib/_sigwinch_handler_c.c 1970-01-01 00:00:00 +0000
23+++ bzrlib/_sigwinch_handler_c.c 2010-05-24 06:02:25 +0000
24@@ -0,0 +1,112 @@
25+/* Copyright (C) 2010 Canonical Ltd
26+ *
27+ * This program is free software; you can redistribute it and/or modify
28+ * it under the terms of the GNU General Public License as published by
29+ * the Free Software Foundation; either version 2 of the License, or
30+ * (at your option) any later version.
31+ *
32+ * This program is distributed in the hope that it will be useful,
33+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
34+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
35+ * GNU General Public License for more details.
36+ *
37+ * You should have received a copy of the GNU General Public License
38+ * along with this program; if not, write to the Free Software
39+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
40+ */
41+
42+/* Minimal handler for SIGWINCH.
43+ *
44+ * Works on any platform with sigaction, regardless of Python version.
45+ */
46+
47+#include <Python.h>
48+#include <signal.h>
49+
50+/* A flag to record if a SIGWINCH has happened since we last checked. Note that
51+ * we don't bother to do any synchronization when setting or getting this value.
52+ */
53+
54+static int _terminal_size_changed;
55+
56+static void sigwinch_handler(int signum)
57+{
58+ _terminal_size_changed = 1;
59+}
60+
61+static char py_install_doc[] =
62+ "Install simple SIGWINCH handler.\n"
63+ "\n"
64+ "After calling install(), you can find out if a SIGWINCH has occurred by\n"
65+ "calling terminal_size_changed().\n";
66+
67+static PyObject *
68+py_install(PyObject *ignored1, PyObject *ignored2)
69+{
70+ struct sigaction new_action, old_action;
71+ PyObject *res;
72+ _terminal_size_changed = 0;
73+ new_action.sa_handler = sigwinch_handler;
74+ sigemptyset(&(new_action.sa_mask));
75+ new_action.sa_flags = SA_RESTART;
76+ if (sigaction(SIGWINCH, &new_action, &old_action) != 0) {
77+ return PyErr_SetFromErrno(PyExc_OSError);
78+ }
79+ res = Py_None;
80+ Py_INCREF(res);
81+ return res;
82+}
83+
84+static char py_terminal_size_changed_doc[] =
85+ "Has SIGWINCH occured since the last call to terminal_size_changed()?\n"
86+ "\n"
87+ "Returns True or False. If True, the value will be reset to False until\n"
88+ "the next occurence of SIGWINCH.\n";
89+
90+static PyObject *
91+py_terminal_size_changed(PyObject *ignored1, PyObject *ignored2)
92+{
93+ PyObject *res;
94+ int has_changed;
95+ has_changed = _terminal_size_changed;
96+ /* Note that it doesn't matter if SIGWINCH occurs after that assignment but
97+ * before the end of this function.
98+ * If SIGWINCH occurs then either:
99+ * - has_changed is set and we'll return True and reset
100+ * _terminal_size_changed anyway, or
101+ * - has_changed is not set, we'll return False and the SIGWINCH will be
102+ * reported on the next call to py_terminal_size_changed.
103+ */
104+ res = PyBool_FromLong(has_changed);
105+ if (!res) {
106+ return res;
107+ }
108+ if (has_changed) {
109+ /* Reset _terminal_size_changed */
110+ _terminal_size_changed = 0;
111+ }
112+ return res;
113+}
114+
115+static PyMethodDef sigwinch_handler_methods[] = {
116+ {"install", py_install, METH_NOARGS, py_install_doc},
117+ {"terminal_size_changed", py_terminal_size_changed, METH_NOARGS,
118+ py_terminal_size_changed_doc},
119+ {NULL, NULL}
120+};
121+
122+PyMODINIT_FUNC
123+init_sigwinch_handler_c(void)
124+{
125+ PyObject* m;
126+ _terminal_size_changed = 0;
127+ m = Py_InitModule3("_sigwinch_handler_c", sigwinch_handler_methods,
128+ "Minimal handler for SIGWINCH.\n"
129+ "\n"
130+ "Call install(), then call terminal_size_changed() to check if a\n"
131+ "SIGWINCH has occurred since the last check.");
132+ if (m == NULL)
133+ return;
134+}
135+
136+// vim: tabstop=4 sw=4 expandtab
137
138=== modified file 'bzrlib/osutils.py'
139--- bzrlib/osutils.py 2010-04-30 06:27:15 +0000
140+++ bzrlib/osutils.py 2010-05-24 06:02:25 +0000
141@@ -1423,6 +1423,7 @@
142 # If COLUMNS is set, take it, the terminal knows better (even inside a
143 # given terminal, the application can decide to set COLUMNS to a lower
144 # value (splitted screen) or a bigger value (scroll bars))
145+ _check_for_sigwinch()
146 try:
147 return int(os.environ['COLUMNS'])
148 except (KeyError, ValueError):
149@@ -1466,18 +1467,26 @@
150 _terminal_size = _ioctl_terminal_size
151
152
153-def _terminal_size_changed(signum, frame):
154- """Set COLUMNS upon receiving a SIGnal for WINdow size CHange."""
155- width, height = _terminal_size(None, None)
156- if width is not None:
157- os.environ['COLUMNS'] = str(width)
158+def _check_for_sigwinch():
159+ """Set COLUMNS upon receiving a SIGnal for WINdow size CHange.
160+
161+ If _sigwinch_handler_c is not installed, this queries the OS for the size
162+ every time.
163+ """
164+ if (not _have_sigwinch_handler_c or
165+ _sigwinch_handler_c.terminal_size_changed()):
166+ width, height = _terminal_size(None, None)
167+ if width is not None:
168+ os.environ['COLUMNS'] = str(width)
169
170
171 _registered_sigwinch = False
172+_have_sigwinch_handler_c = False
173+_sigwinch_handler_c = None
174
175 def watch_sigwinch():
176 """Register for SIGWINCH, once and only once."""
177- global _registered_sigwinch
178+ global _registered_sigwinch, _have_sigwinch_handler_c, _sigwinch_handler_c
179 if not _registered_sigwinch:
180 if sys.platform == 'win32':
181 # Martin (gz) mentioned WINDOW_BUFFER_SIZE_RECORD from
182@@ -1485,7 +1494,13 @@
183 # the current design -- vila 20091216
184 pass
185 else:
186- set_signal_handler(signal.SIGWINCH, _terminal_size_changed)
187+ try:
188+ from bzrlib import _sigwinch_handler_c
189+ except ImportError:
190+ pass
191+ else:
192+ _sigwinch_handler_c.install()
193+ _have_sigwinch_handler_c = True
194 _registered_sigwinch = True
195
196
197
198=== modified file 'setup.py'
199--- setup.py 2010-03-07 13:33:37 +0000
200+++ setup.py 2010-05-24 06:02:25 +0000
201@@ -96,7 +96,7 @@
202 BZRLIB['packages'] = get_bzrlib_packages()
203
204
205-from distutils import log
206+from distutils import ccompiler, log
207 from distutils.core import setup
208 from distutils.command.install_scripts import install_scripts
209 from distutils.command.install_data import install_data
210@@ -317,6 +317,10 @@
211 ext_modules.append(Extension('bzrlib._static_tuple_c',
212 ['bzrlib/_static_tuple_c.c']))
213 add_pyrex_extension('bzrlib._btree_serializer_pyx')
214+compiler = ccompiler.new_compiler()
215+if compiler.has_function('sigaction'):
216+ ext_modules.append(Extension('bzrlib._sigwinch_handler_c',
217+ ['bzrlib/_sigwinch_handler_c.c']))
218
219
220 if unavailable_files:

Subscribers

People subscribed via source and target branches