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

[INADA Naoki]

[Andrew Bennetts (spiv)]

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

[lp:~songofacandy/bzr/fix-524560]

4737. By INADA Naoki on 2010-02-22

Add osutils.open() that uses O_NOINHERIT on Win32.

4738. By INADA Naoki on 2010-02-22

Add bufsize argument in osutils.open()

4739. By INADA Naoki on 2010-02-22

Fix easy miss in previous commit.

4740. By INADA Naoki on 2010-02-22

Use O_NOINHERIT flag in AtomicFile.

4741. By INADA Naoki on 2010-02-22

Add comment to osutils.open()

4742. By INADA Naoki on 2010-02-22

Add missing flags. (O_CREAT, O_TRUNC, etc...)

[INADA Naoki (songofacandy)]

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?

[lp:~songofacandy/bzr/fix-524560]

4743. By INADA Naoki on 2010-02-22

Change osutils.open implementation. Just add 'N' to mode.

[John A Meinel (jameinel)]

-----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-----

[INADA Naoki (songofacandy)]

> 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.

[Martin Packman (gz)]

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

[INADA Naoki (songofacandy)]

> * 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

[Martin Pool (mbp)]

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/>

[Martin Packman (gz)]

> > * 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.

[lp:~songofacandy/bzr/fix-524560]

4744. By INADA Naoki on 2010-02-23

Revert to previous implementation using os.fdopen(os.open())

4745. By INADA Naoki on 2010-02-23

Change name from osutils.open to osutils.open_file

[Martin Packman (gz)]

> 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.

[INADA Naoki (songofacandy)]

> 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.

[INADA Naoki (songofacandy)]

> > > * 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.

[lp:~songofacandy/bzr/fix-524560]

4746. By INADA Naoki on 2010-02-23

Follow renaming open to open_file in transport.local

4747. By INADA Naoki on 2010-02-23

small clean up.

[Martin Packman (gz)]

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

[Vincent Ladeuil (vila)]

I'll merge

review: Approve

[Vincent Ladeuil (vila)]

@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) ?

[Martin Packman (gz)]

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?

[Vincent Ladeuil (vila)]

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

[Vincent Ladeuil (vila)]

Done.

=== modified file 'bzrlib/atomicfile.py'
--- bzrlib/atomicfile.py	2009-03-23 14:59:43 +0000
+++ bzrlib/atomicfile.py	2010-02-23 08:13:17 +0000
@@ -60,7 +60,7 @@
 
         self.realfilename = filename
 
-        flags = os.O_EXCL | os.O_CREAT | os.O_WRONLY
+        flags = os.O_EXCL | os.O_CREAT | os.O_WRONLY | osutils.O_NOINHERIT
         if mode == 'wb':
             flags |= osutils.O_BINARY
         elif mode != 'wt':

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2009-10-08 03:55:30 +0000
+++ bzrlib/osutils.py	2010-02-23 08:13:17 +0000
@@ -74,8 +74,11 @@
 # be opened in binary mode, rather than text mode.
 # On other platforms, O_BINARY doesn't exist, because
 # they always open in binary mode, so it is okay to
-# OR with 0 on those platforms
+# OR with 0 on those platforms.
+# O_NOINHERIT and O_TEXT exists only on win32 too.
 O_BINARY = getattr(os, 'O_BINARY', 0)
+O_TEXT = getattr(os, 'O_TEXT', 0)
+O_NOINHERIT = getattr(os, 'O_NOINHERIT', 0)
 
 
 def get_unicode_argv():
@@ -639,7 +642,7 @@
 def sha_file_by_name(fname):
     """Calculate the SHA1 of a file by reading the full text"""
     s = sha()
-    f = os.open(fname, os.O_RDONLY | O_BINARY)
+    f = os.open(fname, os.O_RDONLY | O_BINARY | O_NOINHERIT)
     try:
         while True:
             b = os.read(f, 1<<16)
@@ -1907,3 +1910,45 @@
     if use_cache:
         _cached_concurrency = concurrency
     return concurrency
+
+if sys.platform == 'win32':
+    def open_file(filename, mode='r', bufsize=-1):
+        """This function works like builtin ``open``. But use O_NOINHERIT
+        flag so file handle is not inherited to child process.
+        So deleting or renaming closed file that opened with this function
+        is not blocked by child process.
+        """
+        writing = 'w' in mode
+        appending = 'a' in mode
+        updating = '+' in mode
+        binary = 'b' in mode
+
+        flags = O_NOINHERIT
+        # see http://msdn.microsoft.com/en-us/library/yeby3zcb%28VS.71%29.aspx
+        # for flags for each modes.
+        if binary:
+            flags |= O_BINARY
+        else:
+            flags |= O_TEXT
+
+        if writing:
+            if updating:
+                flags |= os.O_RDWR
+            else:
+                flags |= os.O_WRONLY
+            flags |= os.O_CREAT | os.O_TRUNC
+        elif appending:
+            if updating:
+                flags |= os.O_RDWR
+            else:
+                flags |= os.O_WRONLY
+            flags |= os.O_CREAT | os.O_APPEND
+        else: #reading
+            if updating:
+                flags |= os.O_RDWR
+            else:
+                flags |= os.O_RDONLY
+
+        return os.fdopen(os.open(filename, flags), mode, bufsize)
+else:
+    open_file = open

=== modified file 'bzrlib/transport/local.py'
--- bzrlib/transport/local.py	2009-06-23 01:59:20 +0000
+++ bzrlib/transport/local.py	2010-02-23 08:13:17 +0000
@@ -42,8 +42,8 @@
 from bzrlib.transport import Transport, Server
 
 
-_append_flags = os.O_CREAT | os.O_APPEND | os.O_WRONLY | osutils.O_BINARY
-_put_non_atomic_flags = os.O_CREAT | os.O_TRUNC | os.O_WRONLY | osutils.O_BINARY
+_append_flags = os.O_CREAT | os.O_APPEND | os.O_WRONLY | osutils.O_BINARY | osutils.O_NOINHERIT
+_put_non_atomic_flags = os.O_CREAT | os.O_TRUNC | os.O_WRONLY | osutils.O_BINARY | osutils.O_NOINHERIT
 
 
 class LocalTransport(Transport):
@@ -160,7 +160,7 @@
             transport._file_streams[canonical_url].flush()
         try:
             path = self._abspath(relpath)
-            return open(path, 'rb')
+            return osutils.open_file(path, 'rb')
         except (IOError, OSError),e:
             if e.errno == errno.EISDIR:
                 return LateReadError(relpath)
@@ -329,7 +329,7 @@
         # initialise the file
         self.put_bytes_non_atomic(relpath, "", mode=mode)
         abspath = self._abspath(relpath)
-        handle = open(abspath, 'wb')
+        handle = osutils.open_file(abspath, 'wb')
         if mode is not None:
             self._check_mode_and_size(abspath, handle.fileno(), mode)
         transport._file_streams[self.abspath(relpath)] = handle