[MERGE] Change DirStateRevisionTree

[John Arbash Meinel]

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

So, as many have encountered, we've had some problems with OS locks on
win32. This happens because on Linux, the same process can lock a file
multiple times, without having problems, but not on win32. (I've
mentioned this before, but I'm summarizing to remind people what is
going on.)

I believe I've tracked down the primary cause of this:

  WorkingTree.basis_tree()

The issue is that for dirstate formats, it returns a
DirStateRevisionTree that grabs direct access to the
'.bzr/checkout/dirstate' file.

So far, nothing critically bad has happened. However, whenever you
lock/unlock a WorkingTree, it releases its access to the dirstate file
and then re-acquires it.

What that means, is that if you do:

  tree = WorkingTree.open('.')
  basis = tree.basis_tree()

  tree.lock_read()
  basis.lock_read()
  tree._current_dirstate() is basis._dirstate # False

And *this* is a problem. Specifically, 'basis.lock_read()' takes out a
shared lock on .bzr/checkout/dirstat, but WT4.lock_write() takes out an
*exclusive* lock on the same file. On Linux, this doesn't cause a
problem (though arguably it should).

Ultimately, I think this is just because of sloppy programming. Namely
"WT.basis_tree()" has @needs_read_lock, so it will automatically lock
and unlock the WT (reading the dirstate file, then throwing it away, and
reading it again later when you go to use it again.) [technically, the
@needs_read_lock is in WT4.revision_tree, but it is the same issue].


So what is happening is that any code that does:

tree = WT.open()
basis = tree.basis_tree()
...
tree.lock_write()
basis.lock_read()
try:
finally:
  basis.unlock()
  tree.unlock()

Is 'broken' by this on win32.


My first idea was to change the contract for WorkingTree.basis_tree(),
to require that the caller manages the lifetime, rather than auto-locking.

However, that is a pain to fix all the code. So instead I went the easy
way, and allow code to be sloppy.


The attached patch changes DirStateRevisionTree so that rather than
hanging on to its own DirState object, it hangs on to the WT object, and
access the DirState *through* the WT object.

Our code is generally already quite good about calling tree.lock_write()
when it is about to modify the tree, it just isn't good about holding
the lock from before it called tree.basis_tree().

Anyway, with this patch, I would imagine most of the win32 failures
about LockContention will go away. I don't really like fixing it by
allowing programmers to be sloppy, but we've had 6months => 1year of
this problem, and nothing has actually happened.

I also added code to give DeprecationWarnings for callers that do
naughty things. However, as I don't want to spend the time fixing them
right now, I just commented out the warning.

With this patch, things like:

  bzr push ../other

Works rather than giving:
bzr: ERROR: Could not acquire lock
"C:/Users/jameinel/dev/,tmp/test2/.bzr/checkout/dirstate"

And "bzr shelve" / "bzr unshelve" also work.

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

iEYEARECAAYFAkl6DNgACgkQJdeBCYSNAAO0lACgnwoTizHec1IPrPBt15w4RRo5
1JsAn2qr+TixACHtw8c3PrYB2t89BHpm
=rvcj
-----END PGP SIGNATURE-----
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: win32-dirstate-locking.patch
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20090123/a70c882c/attachment-0001.diff