Bazaar

Merge lp:~spiv/bzr/siginterrupt-to-avoid-eintr-496813-2.0 into lp:bzr/2.0

  1. siginterrupt-to-avoid-eintr-496813-2.0
  2. Merge into 2.0
Proposed by Andrew Bennetts
Status: Rejected
Rejected by: Andrew Bennetts
Proposed branch: lp:~spiv/bzr/siginterrupt-to-avoid-eintr-496813-2.0
Merge into: lp:bzr/2.0
Diff against target: 94 lines (+42/-2)
3 files modified
NEWS (+16/-0)
bzrlib/breakin.py (+4/-2)
bzrlib/osutils.py (+22/-0)
To merge this branch: bzr merge lp:~spiv/bzr/siginterrupt-to-avoid-eintr-496813-2.0
Reviewer Review Type Date Requested Status
Andrew Bennetts Disapprove
John A Meinel Approve
Martin Packman (community) Abstain
Review via email: mp+20103@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

This patch adds a new function: bzrlib.osutils.set_signal_handler(signum, handler, allow_restart=True). When allow_restart=True (the default), it will call signal.siginterrupt(signum, False) after registering the signal handler, preventing EINTR[1].

In lp:bzr/2.0 there's only one problematic signal handler registered in bzrlib, the SIGQUIT handler. (Later versions can also register a SIGWINCH handler, my merge proposal for those versions will take that into account.)

Separately, we should remove the buggy uses until_no_eintr (and perhaps the entire function), but this patch doesn't depend on doing that to be a useful fix (in fact it should make uses of until_no_eintr, buggy or not, irrelevant).

[1] the signal(7) man page says EINTR is still possible in some cases (search for SA_RESTART), but none of the cases where it might still happen seem to be relevant to us.

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

So, while I think this will work, I don't know if it'll be acceptable for people who depend on the debugging facility. It depends how often it's used to interrupt hangs as opposed to normal operation. This change means Python won't run the registered handler till after the system function returns, which might be network timeout, or never.

More info in this reply on list:
<https://lists.ubuntu.com/archives/bazaar/2010q1/067168.html>

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

I know we like to interrupt while stuff is going on, but I would guess that would be blocked after having read some data (which can then return some content), versus blocked on no data (which requires EINTR).

So I'd rather see this land then do nothing forever.

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

I'm withdrawing this particular merge proposal. The current plan for using siginterrupt is described at <https://lists.ubuntu.com/archives/bazaar/2010q1/067187.html>, in short:

 * SIGQUIT is probably better left as-is (and it only matters for debugging, thankfully, not normal use);
 * SIGWINCH should use siginterrupt(SIGWINCH, False), but that only matters for bzr 2.1 and later; and
 * so far bzrlib doesn't install any other signal handlers, and there are no default signal handlers installed by Python that we should change.

So I will go ahead with the merge for 2.1 (for SIGWINCH), but not for 2.0.

review: Disapprove

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-02-19 07:13:45 +0000
3+++ NEWS 2010-02-25 04:46:16 +0000
4@@ -34,6 +34,22 @@
5 * Added ``location-alias`` help topic.
6 (Andrew Bennetts, #337834)
7
8+API Changes
9+***********
10+
11+* Added ``bzrlib.osutils.set_signal_handler``, a convenience function that
12+ can set a signal handler and call ``signal.siginterrupt(signum,
13+ False)`` for it, if the platform and Python version supports it.
14+ (Andrew Bennetts, #496813)
15+
16+Internals
17+*********
18+
19+* ``bzrlib.breakin`` uses ``bzrlib.osutils.set_signal_handler`` to set the
20+ ``SIGQUIT`` signal handler, so that entering the debugger with
21+ ``SIGQUIT`` will no longer cause ``EINTR`` during blocking IO.
22+ (Andrew Bennetts, #173007)
23+
24 bzr 2.0.4
25 #########
26
27
28=== modified file 'bzrlib/breakin.py'
29--- bzrlib/breakin.py 2009-07-30 23:54:26 +0000
30+++ bzrlib/breakin.py 2010-02-25 04:46:16 +0000
31@@ -25,6 +25,7 @@
32 def _debug(signal_number, interrupted_frame):
33 import pdb
34 import sys
35+ from bzrlib.osutils import set_signal_handler
36 sys.stderr.write("** %s received, entering debugger\n"
37 "** Type 'c' to continue or 'q' to stop the process\n"
38 "** Or %s again to quit (and possibly dump core)\n"
39@@ -38,7 +39,7 @@
40 try:
41 pdb.set_trace()
42 finally:
43- signal.signal(_breakin_signal_number, _debug)
44+ set_signal_handler(_breakin_signal_number, _debug)
45
46
47 def hook_sigquit():
48@@ -90,4 +91,5 @@
49 if sig is None:
50 return
51 # print 'hooking into %s' % (_breakin_signal_name,)
52- signal.signal(sig, _debug)
53+ from bzrlib.osutils import set_signal_handler
54+ set_signal_handler(sig, _debug)
55
56=== modified file 'bzrlib/osutils.py'
57--- bzrlib/osutils.py 2009-10-08 03:55:30 +0000
58+++ bzrlib/osutils.py 2010-02-25 04:46:16 +0000
59@@ -38,6 +38,7 @@
60 from shutil import (
61 rmtree,
62 )
63+import signal
64 import subprocess
65 import tempfile
66 from tempfile import (
67@@ -1245,6 +1246,27 @@
68 normalized_filename = _inaccessible_normalized_filename
69
70
71+def set_signal_handler(signum, handler, restart_syscall=True):
72+ """A wrapper for signal.signal that also calls siginterrupt(signum, False)
73+ on platforms that support that.
74+
75+ :param restart_syscall: if set, allow syscalls interrupted by a signal to
76+ automatically restart (by calling `signal.siginterrupt(signum,
77+ False)`). May be ignored if the feature is not available on this
78+ platform or Python version.
79+ """
80+ old_handler = signal.signal(signum, handler)
81+ if restart_syscall:
82+ try:
83+ siginterrupt = signal.siginterrupt
84+ except AttributeError: # siginterrupt doesn't exist on this platform, or for this version of
85+ # Python.
86+ pass
87+ else:
88+ siginterrupt(signum, False)
89+ return old_handler
90+
91+
92 def terminal_width():
93 """Return estimated terminal width."""
94 if sys.platform == 'win32':

Subscribers

People subscribed via source and target branches