Bazaar

Merge lp:~songofacandy/bzr/fix-524560 into lp:bzr/2.0

  1. fix-524560
  2. Merge into 2.0
Proposed by methane
Status: Merged
Merged at revision: not available
Proposed branch: lp:~songofacandy/bzr/fix-524560
Merge into: lp:bzr/2.0
Diff against target: 117 lines (+52/-7)
3 files modified
bzrlib/atomicfile.py (+1/-1)
bzrlib/osutils.py (+47/-2)
bzrlib/transport/local.py (+4/-4)
To merge this branch: bzr merge lp:~songofacandy/bzr/fix-524560
Reviewer Review Type Date Requested Status
Vincent Ladeuil Approve
Martin Packman (community) Approve
Andrew Bennetts Approve
Review via email: mp+19737@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

It would be nice to have a test for this, but I'm not sure what that would look like.

This looks like a good candidate for the 2.0 branch, so thanks for preparing it against that.

review: Approve
Revision history for this message
methane (songofacandy) wrote :

I've found open with mode="wbN" is enough.
mode 'N' means NOINHERIT on win32 and ignored on Linux.

Should I remove osutils.open and add 'N' flag directly?

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

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

INADA Naoki wrote:
> I've found open with mode="wbN" is enough.
> mode 'N' means NOINHERIT on win32 and ignored on Linux.
>
> Should I remove osutils.open and add 'N' flag directly?

If that works, I would recommend it. But we need to make sure that it
doesn't cause problems with Python 2.4.

John
=:->

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

iEYEARECAAYFAkuC2fkACgkQJdeBCYSNAAPWoACgoZQT+WU/bhw3CDVKhVBMgEjv
ZogAn1zgWNTubK4mjgIB8HlcsF+MsNqg
=UidF
-----END PGP SIGNATURE-----

Revision history for this message
methane (songofacandy) wrote :

> INADA Naoki wrote:
> > I've found open with mode="wbN" is enough.
> > mode 'N' means NOINHERIT on win32 and ignored on Linux.
> >
> > Should I remove osutils.open and add 'N' flag directly?
>
> If that works, I would recommend it. But we need to make sure that it
> doesn't cause problems with Python 2.4.
>
> John
> =:->

I confirmed that glibc doesn't use 'N' for mode so it just ignore the 'N'.
But I don't know about other libc. (on Solaris, FreeBSD, MacOS, etc...)
So I think using 'N' only on win32 is safe.

Python 2.4 uses msvcrt71 (VC++2003?) and msdn doesn't document 'N' mode
and O_NOINHERIT. But os module in Python 2.4 have O_NOINHERIT so fcntl.h in
VC++2003 should have O_NOINHERIT.
I don't know the file is inherited to child process or not.
But I installed Python 2.4 and ``open('foo', 'wbN').write('foo')`` just works.

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

Some general comments:

* Adding O_NOINHERIT to the default flags bzr uses seems sensible.
* Don't like the style of having shadows in osutils that are almost-but-not-quite builtins or standard library functions.

> Python 2.4 uses msvcrt71 (VC++2003?) and msdn doesn't document 'N' mode
> and O_NOINHERIT. But os module in Python 2.4 have O_NOINHERIT so fcntl.h in
> VC++2003 should have O_NOINHERIT.
> I don't know the file is inherited to child process or not.
> But I installed Python 2.4 and ``open('foo', 'wbN').write('foo')`` just works.

MSDN is correct, it does *not* work on Pythons compiled with VS < 2005. The open succeeds but the flag is ignored. I threw together some quick tests that show this, and don't think this change should go in untested or this lightly documented.

review: Abstain
Revision history for this message
methane (songofacandy) wrote :

> * Don't like the style of having shadows in osutils that are almost-but-not-
> quite builtins or standard library functions.

So, which name is suitable?

* open_noinherit()
* open_noshare()
* or other name...

> > Python 2.4 uses msvcrt71 (VC++2003?) and msdn doesn't document 'N' mode
> > and O_NOINHERIT. But os module in Python 2.4 have O_NOINHERIT so fcntl.h in
> > VC++2003 should have O_NOINHERIT.
> > I don't know the file is inherited to child process or not.
> > But I installed Python 2.4 and ``open('foo', 'wbN').write('foo')`` just
> works.
>
> MSDN is correct, it does *not* work on Pythons compiled with VS < 2005. The
> open succeeds but the flag is ignored. I threw together some quick tests that
> show this, and don't think this change should go in untested or this lightly
> documented.

Should we support this case on Python 2.4 on Windows?
If should, using os.fdopen(os.open()) may work on Python 2.4.
http://bazaar.launchpad.net/~songofacandy/bzr/fix-524560/revision/4742

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

On 23 February 2010 12:54, INADA Naoki <email address hidden> wrote:
>> * Don't like the style of having shadows in osutils that are almost-but-not-
>> quite builtins or standard library functions.
>
> So, which name is suitable?
>
> * open_noinherit()
> * open_noshare()
> * or other name...

open_file.

I agree with gzlist that using exactly the same name is likely to
cause confusion, but the fact that you want noinherit seems more like
an option than an entirely different function. Or would it be
feasible to just pass the right option to plain open()?

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

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

> > * Don't like the style of having shadows in osutils that are almost-but-not-
> > quite builtins or standard library functions.

Just realised there's a clash with another current merge proposal here:
<https://code.launchpad.net/~parthm/bzr/376388-dot-bazaar-ownership/+merge/19691>

Which I guess proves my point.

> So, which name is suitable?
>
> * open_noinherit()
> * open_noshare()
> * or other name...

I like the first one best there, but pine for a better way...

> Should we support this case on Python 2.4 on Windows?
> If should, using os.fdopen(os.open()) may work on Python 2.4.
> http://bazaar.launchpad.net/~songofacandy/bzr/fix-524560/revision/4742

Python 2.5 is also pre-VS2005. Using O_NOINHERIT does work on everything, though if it's the best option I don't know.

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

> I like the first one best there, but pine for a better way...

Nope, can't find one. Only vile hacks and Vista-or-later functionality.

Personally, I wouldn't object to putting 'N' in the open flags of any long-lived file bazaar uses, and documenting that people should expect things to break with Python < 2.6 if they don't use paramiko. About the only person that'll harm will be me, and I've not run into this bug yet.

Revision history for this message
methane (songofacandy) wrote :

> On 23 February 2010 12:54, INADA Naoki <email address hidden> wrote:
> >> * Don't like the style of having shadows in osutils that are almost-but-
> not-
> >> quite builtins or standard library functions.
> >
> > So, which name is suitable?
> >
> > * open_noinherit()
> > * open_noshare()
> > * or other name...
>
> open_file.
>
> I agree with gzlist that using exactly the same name is likely to
> cause confusion, but the fact that you want noinherit seems more like
> an option than an entirely different function. Or would it be
> feasible to just pass the right option to plain open()?
>

OK, I agree open_file is good.
I've changed the name.

Revision history for this message
methane (songofacandy) wrote :

> > > * Don't like the style of having shadows in osutils that are almost-but-
> not-
> > > quite builtins or standard library functions.
>
> Just realised there's a clash with another current merge proposal here:
> <https://code.launchpad.net/~parthm/bzr/376388-dot-bazaar-
> ownership/+merge/19691>

Thanks for point it.
open_file() should fixes this and it.

> > Should we support this case on Python 2.4 on Windows?
> > If should, using os.fdopen(os.open()) may work on Python 2.4.
> > http://bazaar.launchpad.net/~songofacandy/bzr/fix-524560/revision/4742
>
> Python 2.5 is also pre-VS2005. Using O_NOINHERIT does work on everything,
> though if it's the best option I don't know.

OK. I should use O_NOINHERIT flag instead of 'N' mode.
I've fixed it now.

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

Okay, I think some version of this should go in.

I've got a competing branch that does the minimal 'N' flag adding here:
<https://code.launchpad.net/~gz/bzr/noinherit_file_handles_524560>

But I was under the impression that the all-in-one installer used the latest Python. As it's still on 2.5 my branch wouldn't actually fix the edge case in the bug for most people.

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

I'll merge

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

@Martin [gz], damn, our messages crossed on the wire, I had already sent the pqm submission long before I saw your message...

Can you do a merge proposal explaining your branch intent or
is it now irrelevant (you said "some version of should go in"
and Naoki's version hs landed in 2.0 now) ?

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

What you've landed is fine Vincent, Naoki's more complicated version is needed for the older Python versions.

I don't see it on bzr.dev yet though, it needs upmerging still?

Revision history for this message
Vincent Ladeuil (vila) wrote :

yes, I'll will do that (merging 2.0 in 2.1 and 2.1 into bzr.dev) asap.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Done.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/atomicfile.py'
2--- bzrlib/atomicfile.py 2009-03-23 14:59:43 +0000
3+++ bzrlib/atomicfile.py 2010-02-23 08:13:17 +0000
4@@ -60,7 +60,7 @@
5
6 self.realfilename = filename
7
8- flags = os.O_EXCL | os.O_CREAT | os.O_WRONLY
9+ flags = os.O_EXCL | os.O_CREAT | os.O_WRONLY | osutils.O_NOINHERIT
10 if mode == 'wb':
11 flags |= osutils.O_BINARY
12 elif mode != 'wt':
13
14=== modified file 'bzrlib/osutils.py'
15--- bzrlib/osutils.py 2009-10-08 03:55:30 +0000
16+++ bzrlib/osutils.py 2010-02-23 08:13:17 +0000
17@@ -74,8 +74,11 @@
18 # be opened in binary mode, rather than text mode.
19 # On other platforms, O_BINARY doesn't exist, because
20 # they always open in binary mode, so it is okay to
21-# OR with 0 on those platforms
22+# OR with 0 on those platforms.
23+# O_NOINHERIT and O_TEXT exists only on win32 too.
24 O_BINARY = getattr(os, 'O_BINARY', 0)
25+O_TEXT = getattr(os, 'O_TEXT', 0)
26+O_NOINHERIT = getattr(os, 'O_NOINHERIT', 0)
27
28
29 def get_unicode_argv():
30@@ -639,7 +642,7 @@
31 def sha_file_by_name(fname):
32 """Calculate the SHA1 of a file by reading the full text"""
33 s = sha()
34- f = os.open(fname, os.O_RDONLY | O_BINARY)
35+ f = os.open(fname, os.O_RDONLY | O_BINARY | O_NOINHERIT)
36 try:
37 while True:
38 b = os.read(f, 1<<16)
39@@ -1907,3 +1910,45 @@
40 if use_cache:
41 _cached_concurrency = concurrency
42 return concurrency
43+
44+if sys.platform == 'win32':
45+ def open_file(filename, mode='r', bufsize=-1):
46+ """This function works like builtin ``open``. But use O_NOINHERIT
47+ flag so file handle is not inherited to child process.
48+ So deleting or renaming closed file that opened with this function
49+ is not blocked by child process.
50+ """
51+ writing = 'w' in mode
52+ appending = 'a' in mode
53+ updating = '+' in mode
54+ binary = 'b' in mode
55+
56+ flags = O_NOINHERIT
57+ # see http://msdn.microsoft.com/en-us/library/yeby3zcb%28VS.71%29.aspx
58+ # for flags for each modes.
59+ if binary:
60+ flags |= O_BINARY
61+ else:
62+ flags |= O_TEXT
63+
64+ if writing:
65+ if updating:
66+ flags |= os.O_RDWR
67+ else:
68+ flags |= os.O_WRONLY
69+ flags |= os.O_CREAT | os.O_TRUNC
70+ elif appending:
71+ if updating:
72+ flags |= os.O_RDWR
73+ else:
74+ flags |= os.O_WRONLY
75+ flags |= os.O_CREAT | os.O_APPEND
76+ else: #reading
77+ if updating:
78+ flags |= os.O_RDWR
79+ else:
80+ flags |= os.O_RDONLY
81+
82+ return os.fdopen(os.open(filename, flags), mode, bufsize)
83+else:
84+ open_file = open
85
86=== modified file 'bzrlib/transport/local.py'
87--- bzrlib/transport/local.py 2009-06-23 01:59:20 +0000
88+++ bzrlib/transport/local.py 2010-02-23 08:13:17 +0000
89@@ -42,8 +42,8 @@
90 from bzrlib.transport import Transport, Server
91
92
93-_append_flags = os.O_CREAT | os.O_APPEND | os.O_WRONLY | osutils.O_BINARY
94-_put_non_atomic_flags = os.O_CREAT | os.O_TRUNC | os.O_WRONLY | osutils.O_BINARY
95+_append_flags = os.O_CREAT | os.O_APPEND | os.O_WRONLY | osutils.O_BINARY | osutils.O_NOINHERIT
96+_put_non_atomic_flags = os.O_CREAT | os.O_TRUNC | os.O_WRONLY | osutils.O_BINARY | osutils.O_NOINHERIT
97
98
99 class LocalTransport(Transport):
100@@ -160,7 +160,7 @@
101 transport._file_streams[canonical_url].flush()
102 try:
103 path = self._abspath(relpath)
104- return open(path, 'rb')
105+ return osutils.open_file(path, 'rb')
106 except (IOError, OSError),e:
107 if e.errno == errno.EISDIR:
108 return LateReadError(relpath)
109@@ -329,7 +329,7 @@
110 # initialise the file
111 self.put_bytes_non_atomic(relpath, "", mode=mode)
112 abspath = self._abspath(relpath)
113- handle = open(abspath, 'wb')
114+ handle = osutils.open_file(abspath, 'wb')
115 if mode is not None:
116 self._check_mode_and_size(abspath, handle.fileno(), mode)
117 transport._file_streams[self.abspath(relpath)] = handle

Subscribers

People subscribed via source and target branches