win32 file locking

[Andrew Bennetts]

On Wed, May 25, 2005 at 01:43:43AM -0500, John A Meinel wrote:
[...]
> 
>     def __del__(self):
>         """Abort if the file has not already been closed."""
> -        self.close()
> +        if not self.closed:
> +            raise RuntimeError('AtomicFile must have
> commit()/abort()/close() called before it expires.')
> 
>     def commit(self):
>         """Close the file and move to final name."""
> 
> This way AtomicFile will throw an exception if it is not used correctly.

There's not much point raising an exception in a __del__ -- who's going to
catch it?  There's no caller other than the garbage collector, which will
just log a warning about it.  Just explicitly log a warning instead :)

(See, another reason not to use __del__ <wink>)

> The problem for Branch() is that I don't think anyone is going to go
> around and call "unlock()" when they are done with it, meaning that all
> branches are locked for the lifetime of bzrlib. Since bzrlib is meant to
> be a library, potentially used by a GUI, this could be a long time.
> 
> The problem is that Branch() is commonly used as a temporary object. For
> instance the 'bzr revno' command is done like:
> 
> print Branch('.').revno()
> 
> How do you unlock a branch if you never have an object to call
> 'unlock()' on?

It would be nice if this could just work.  I don't have an easy answer.  If
you can be confident that Branch holds references to things that will never
lead to a reference cycle, __del__ is ok, but I think that's unrealistic.

At a glance, it looks like the DebugInfo idea could be adapted to unlocking
the Branch:

*** modified file 'bzrlib/branch.py'
--- bzrlib/branch.py
+++ bzrlib/branch.py
@@ -155,18 +155,20 @@
                     raise e

             fcntl.lockf(lockfile, lm)
-            def unlock():
+            def unlocker():
                 fcntl.lockf(lockfile, fcntl.LOCK_UN)
                 os.close(lockfile)
-                self._lockmode = None
-            self.unlock = unlock
-            self._lockmode = mode
         except ImportError:
             warning("please write a locking method for platform %r" % sys.platform)
-            def unlock():
-                self._lockmode = None
-            self.unlock = unlock
-            self._lockmode = mode
+            def unlocker():
+                None
+        self._lockmode = mode
+        def unlock():
+            unlocker()
+            self._lockmode = None
+            self._unlocker.unlock = None
+        self.unlock = unlock
+        self._unlocker = BranchUnlocker(unlocker)


     def _need_readlock(self):
@@ -710,6 +712,12 @@
         self._write_inventory(inv)


+class BranchUnlocker:
+    def __init__(self, unlock):
+        self.unlock = unlock
+    def __del__(self):
+        if self.unlock is not None:
+            self.unlock()


 class ScratchBranch(Branch):


The key is to avoid reference cycles with objects with __del__.  It's hard
to prevent that with objects in a public API, like Branch.  It's possible
with private objects, like BranchUnlocker.

What do you think?

-Andrew.