[RFC] Implement bzrlib.lock with CreateFile rather than LockFileEx on win32

[Martin (gzlist)]

Thanks for the feedback.

2009/6/16 Martin Pool <mbp at sourcefrog.net>:
> 2009/6/10 Martin (gzlist) <gzlist at googlemail.com>:
>>
>> Questions:
>> # Would a change along these lines be acceptable?
>
> I think these a change along these lines would be fine, assuming it
> pases tests (or doesn't cause any new failures.)  We should also
> manually check interoperation with old versions, to make sure they
> correctly exclude each other.

Can test that just on the patch above. It basically works, the old
two-step process gets Failed vs. Contention confused though, eg:

>>> from bzrlib.lock import _pywin32_CreateFile_WriteLock, _w32c_WriteLock
>>> a = _pywin32_CreateFile_WriteLock("/tmp.lock")
>>> b = _w32c_WriteLock("/tmp.lock")
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "C:\bzr\bzr\lock_on_create\bzrlib\lock.py", line 426, in __init__
    self._lock(filename, 'rb+', LOCK_EX + LOCK_NB)
  File "C:\bzr\bzr\lock_on_create\bzrlib\lock.py", line 372, in _lock
    self._open(filename, openmode)
  File "C:\bzr\bzr\lock_on_create\bzrlib\lock.py", line 125, in _open
    raise errors.LockFailed(self.filename, str(e))
bzrlib.errors.LockFailed: Cannot lock C:/tmp.lock: [Errno 13]
Permission denied: u'C:/tmp.lock'
>>> a.unlock()
>>> b = _w32c_WriteLock("/tmp.lock")
>>> a = _pywin32_CreateFile_WriteLock("/tmp.lock")
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "C:\bzr\bzr\lock_on_create\bzrlib\lock.py", line 342, in __init__
    os.O_RDWR, "rb+")
  File "C:\bzr\bzr\lock_on_create\bzrlib\lock.py", line 325, in _open
    raise errors.LockContention(filename, e)
bzrlib.errors.LockContention: Could not acquire lock "/tmp.lock": (32,
'CreateFile', 'The process cannot access the file because it is being
used by another process.')

>
>> # Are there any expectations of the lock classes that aren't exercised in tests?
>
> Possibly.  The main limitations seem to me to be:
>
> 1. We don't have very good code for testing locking across different
> unrelated processes, different threads, etc -- and the behaviour of
> the locks we use there varies, which is bad.  Eventually we want to
> get away from using OS file locks.

I think you said something similar in Alexander's bug from 2007 that I
linked. File locks don't seem to be going anywhere fast.

Have been bothered before by unittest not doing multiprocess well, do
you know of any work that's been done to get around that?

>> # Should the existing win32 classes be left in place for the moment at least?
>
> I don't think so.  Do you?  If it turns out to be a problem we can
> revert the change or (in the short term) people can downgrade.

This seems fair, if new-over-old testing as per above is sufficient.

>> # Would altering the design to avoid all the code duplication be warrented?
>
> Feel free too, but maybe as a separate patch.

Will look at as a later change as you suggest.

>> # Are locks only taken on byte string filenames, or is unicode used as well?
>
> I'm not sure tbh.  It can probably be either.

Looking at it again, it seems _win32_realpath in bzrlib.osutils is
effectively ensuring the filename is unicode (and that str input is an
ascii, or it throws a UnicodeDecodeError).

>> # How *are* you meant to do unicode exceptions in bazaar...
>
> What do you mean?

Sorry, I explained in a bit more detail in the message with the patch
preceeding this one. Basically, windows gives error messages in the
user's language, which pywin32 and ctypes expose as byte strings, that
may well not be in the same encoding as the terminal/whatever bzr
wants to write to the log. I wanted to know the right way to encode
the message to get it to display correctly.

Having looked at the bzrlib.errors code now though, it seems there is
no right way. Even simple things will end up like:
LockFailed(Unprintable exception LockFailed: dict={'msg': '', 'lock':
u'somefile', 'why': 'O acesso \xe9 negado.'}, fmt='Cannot lock
%(lock)s: %(why)s', error=<exceptions.UnicodeDecodeError instance at
0x00E33378>)
Ensuring all params are unicode doesn't help much either, as the repr
ends up as UTF-8 and thus mangled on the console. For
non-ascii-superset languages, this means they're not going to be able
to read the error messages at all.

As it's now obvious this isn't a problem with an easy-but-undocumented
solution, I'll ignore it for the moment.

Martin