Bazaar

Merge lp:~spiv/bzr/check-signals-2.0 into lp:bzr/2.0

  1. check-signals-2.0
  2. Merge into 2.0
Proposed by Andrew Bennetts
Status: Merged
Approved by: Martin Pool
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~spiv/bzr/check-signals-2.0
Merge into: lp:bzr/2.0
Diff against target: 103 lines (+20/-7)
2 files modified
NEWS (+4/-0)
bzrlib/_readdir_pyx.pyx (+16/-7)
To merge this branch: bzr merge lp:~spiv/bzr/check-signals-2.0
Reviewer Review Type Date Requested Status
Martin Pool Approve
John A Meinel Approve
Review via email: mp+16520@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

This patch makes _readdir_pyx call PyErr_CheckSignals if the errno is EINTR. We don't have any concrete evidence that this has caused problems (see bug 495023), but it does seem like the correct thing to do regardless. If nothing else it should make _readdir_pyx a little more responsive to Ctrl-C (don't retry the readdir if the user wants to interrupt bzr!).

I've made the patch against lp:bzr/2.0, and this proposal is targetted to 2.0, but as 2.1 draws closer we may decide to play it safe with 2.0.

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/check-signals-2.0 into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #495023 Interrupting commit to smart server sometimes removes files
> https://bugs.launchpad.net/bugs/495023
>
>
> This patch makes _readdir_pyx call PyErr_CheckSignals if the errno is EINTR. We don't have any concrete evidence that this has caused problems (see bug 495023), but it does seem like the correct thing to do regardless. If nothing else it should make _readdir_pyx a little more responsive to Ctrl-C (don't retry the readdir if the user wants to interrupt bzr!).
>
> I've made the patch against lp:bzr/2.0, and this proposal is targetted to 2.0, but as 2.1 draws closer we may decide to play it safe with 2.0.
>

It would be nice to have testing of this, but if you've at least done
manual testing, that would be good enough for me.

John
=:->
  review: approve

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

iEYEARECAAYFAksxhBsACgkQJdeBCYSNAAM4yACghF94deLZ6g0iuFTZFWZ4XoUE
zSYAnR1D89INiM2FXtt2/PFDPF+GkClV
=CdEx
-----END PGP SIGNATURE-----

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

John A Meinel wrote:
>
> It would be nice to have testing of this, but if you've at least done
> manual testing, that would be good enough for me.

Well, I haven't go so far as to actually find a way to trigger EINTR.
But it hasn't broken anything that I can see in manual testing.

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

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

Andrew Bennetts wrote:
> John A Meinel wrote:
>> It would be nice to have testing of this, but if you've at least done
>> manual testing, that would be good enough for me.
>
> Well, I haven't go so far as to actually find a way to trigger EINTR.
> But it hasn't broken anything that I can see in manual testing.

Resizing the window generates SIGWINCH, which is often a good way to do
it. You just need a dir big enough that readdir() gives you a good
window to trigger it. (Which is easier if you drop the page cache first.)

Though the original bug was about ^C getting suppressed. So you could
try something like that.

Anyway, the code looks fine. The main reason for a test is to make sure
that PyErr_CheckSignals() really does what we think it does.

John
=:->

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

iEYEARECAAYFAksxnAEACgkQJdeBCYSNAANJvwCgsB36dbYUHNUhCpspWSMP5JSV
tmoAn09EmnF7Sd+O3aZDJiD635yD+Mnf
=nNZW
-----END PGP SIGNATURE-----

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

Passing errnum seems a bit redundant since it's always errno, but either way is fine. Possible passing a regular int reduces the scope for errno magically changing.

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

oh, also i wonder if this should be mentioned as a tip in the hacking guide - though perhaps it's better to just rely on memory.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-12-22 04:10:41 +0000
3+++ NEWS 2009-12-23 02:43:13 +0000
4@@ -31,6 +31,10 @@
5 This will likely have an impact on any other process that is serving for
6 an extended period of time. (John Arbash Meinel, #494406)
7
8+* Check for SIGINT (Ctrl-C) and other signals immediately if ``readdir``
9+ returns ``EINTR`` by calling ``PyErr_CheckSignals``. This affected the
10+ optional ``_readdir_pyx`` extension. (Andrew Bennetts, #495023)
11+
12 * Give a clearer message if the lockdir disappears after being apparently
13 successfully taken. (Martin Pool, #498378)
14
15
16=== modified file 'bzrlib/_readdir_pyx.pyx'
17--- bzrlib/_readdir_pyx.pyx 2009-10-08 07:03:05 +0000
18+++ bzrlib/_readdir_pyx.pyx 2009-12-23 02:43:13 +0000
19@@ -78,6 +78,7 @@
20
21
22 cdef extern from 'Python.h':
23+ int PyErr_CheckSignals() except -1
24 char * PyString_AS_STRING(object)
25 ctypedef int Py_ssize_t # Required for older pyrex versions
26 ctypedef struct PyObject:
27@@ -271,6 +272,12 @@
28 return result
29
30
31+cdef raise_os_error(int errnum, char *msg_prefix, path):
32+ if errnum == EINTR:
33+ PyErr_CheckSignals()
34+ raise OSError(errnum, msg_prefix + strerror(errnum), path)
35+
36+
37 cdef _read_dir(path):
38 """Like os.listdir, this reads the contents of a directory.
39
40@@ -298,16 +305,16 @@
41 # passing full paths every time.
42 orig_dir_fd = open(".", O_RDONLY, 0)
43 if orig_dir_fd == -1:
44- raise OSError(errno, "open: " + strerror(errno), ".")
45+ raise_os_error(errno, "open: ", ".")
46 if -1 == chdir(path):
47- raise OSError(errno, "chdir: " + strerror(errno), path)
48+ raise_os_error(errno, "chdir: ", path)
49 else:
50 orig_dir_fd = -1
51
52 try:
53 the_dir = opendir(".")
54 if NULL == the_dir:
55- raise OSError(errno, "opendir: " + strerror(errno), path)
56+ raise_os_error(errno, "opendir: ", path)
57 try:
58 result = []
59 entry = &sentinel
60@@ -319,6 +326,8 @@
61 errno = 0
62 entry = readdir(the_dir)
63 if entry == NULL and (errno == EAGAIN or errno == EINTR):
64+ if errno == EINTR:
65+ PyErr_CheckSignals()
66 # try again
67 continue
68 else:
69@@ -330,7 +339,7 @@
70 # we consider ENOTDIR to be 'no error'.
71 continue
72 else:
73- raise OSError(errno, "readdir: " + strerror(errno), path)
74+ raise_os_error(errno, "readdir: ", path)
75 name = entry.d_name
76 if not (name[0] == c"." and (
77 (name[1] == 0) or
78@@ -340,7 +349,7 @@
79 stat_result = lstat(entry.d_name, &statvalue._st)
80 if stat_result != 0:
81 if errno != ENOENT:
82- raise OSError(errno, "lstat: " + strerror(errno),
83+ raise_os_error(errno, "lstat: ",
84 path + "/" + entry.d_name)
85 else:
86 # the file seems to have disappeared after being
87@@ -358,7 +367,7 @@
88 statvalue, None))
89 finally:
90 if -1 == closedir(the_dir):
91- raise OSError(errno, "closedir: " + strerror(errno), path)
92+ raise_os_error(errno, "closedir: ", path)
93 finally:
94 if -1 != orig_dir_fd:
95 failed = False
96@@ -366,7 +375,7 @@
97 # try to close the original directory anyhow
98 failed = True
99 if -1 == close(orig_dir_fd) or failed:
100- raise OSError(errno, "return to orig_dir: " + strerror(errno))
101+ raise_os_error(errno, "return to orig_dir: ", "")
102
103 return result
104

Subscribers

People subscribed via source and target branches