[ditrack commit] r1504 - in src/trunk: . DITrack DITrack/DB DITrack/Util webui
Vlad Skvortsov
vss at 73rus.com
Sun Apr 29 15:00:33 PDT 2007
Hi!
I've got several issues with this commit. Please see my comments below.
gli at ditrack.org wrote:
> Author: gli
> Date: 2007-04-03 02:51:15 -0700 (Tue, 03 Apr 2007)
> New Revision: 1504
>
> Modified:
> src/trunk/DITrack/DB/Common.py
> src/trunk/DITrack/DB/Exceptions.py
> src/trunk/DITrack/LMA.py
> src/trunk/DITrack/Util/common.py
> src/trunk/dt
> src/trunk/webui/index.cgi
> Log:
> fixing i#52. Lock database. Added functionality (without tests yet).
>
I suggest starting bottom-up, namely:
1) add tests for locking files (not DB-related);
2) add a module to do OS-independent locking (something like
DITrack.Util.Locking);
3) make use of this new module in the DB;
4) add tests for database locking.
> Modified: src/trunk/DITrack/DB/Common.py
> ===================================================================
> --- src/trunk/DITrack/DB/Common.py 2007-03-29 09:07:44 UTC (rev 1503)
> +++ src/trunk/DITrack/DB/Common.py 2007-04-03 09:51:15 UTC (rev 1504)
> @@ -32,6 +32,8 @@
> import pprint
> import re
> import string
> +import fcntl
> +import errno
>
>
> # DITrack modules
> import DITrack.DB.Issue
> @@ -48,6 +50,13 @@
> # Current database format version.
> FORMAT_VERSION = "2"
>
> +# Local modifications area directory and file name
> +LMA_DIR = ".ditrack"
>
Should be renamed to something like LOCAL_DT_DIR, since now it contains
files besides LMA.
> +LMA_FILE = "LMA"
>
It seems that it belongs to LMA.py, not here.
> +
> +# Lock file
> +LOCK_FILE = "LOCK"
> +
> #
> # Classes
> #
> @@ -113,7 +122,7 @@
> Class representing complete database object.
> """
>
> - def __init__(self, path, globals):
> + def __init__(self, path, globals, opts):
> """
> Opens a DITrack database at PATH. The SVN path is a path to the
> Subversion command line client executable.
> @@ -144,7 +153,10 @@
> # Check if this is supported version.
> if v != FORMAT_VERSION:
> raise DITrack.DB.Exceptions.InvalidVersionError
> -
> +
> + # Lock database
> + self.lockfd = self.lock(path, globals, opts)
>
New locking API should be orthogonal and independent to the Database
internals, so the above line would look like:
self.lock = DITrack.Util.Locking.lock(path)
> +
> # Read up the configuration.
> self.cfg = DITrack.DB.Configuration.Configuration(path, globals)
>
> @@ -154,6 +166,10 @@
> # Working copy interface.
> self.wc = DITrack.WC.WorkingCopy(path, globals.svn_path)
>
> + def __del__(self):
> + # Unlock database
> + self.unlock()
>
The lock should be automatically removed by
DITrack.Util.Locking.Lock.__del__(). So there should be no need to
remove it explicitly here.
> +
> def __getitem__(self, key):
> """
> Return an issue by KEY (string id). Raises KeyError is the issue is
> @@ -466,3 +482,65 @@
> # XXX: check that KeyError will be raised if COMMENT_NAME is not
> # contained in the LMA issue
> self.lma.remove_comment(issue_id, comment_name)
> +
> + def lock(self, path, globals, opts):
> + """
> + Lock the database (or do nothing for ro-access methods)
> + If the database has already been locked,
> + raise DBIsLockedError exception.
> + If lockfile doesn't contain correct pid,
> + raise DBLockFileCorruptedError exception.
> + If there's no lock on a lockfile content,
> + raise DBStaleLockError exception.
> + Returns a file descriptor of a lockfile, or None if database
> + was locked and we request read-only access.
> + """
> +
> + datadir = os.path.join(path, LMA_DIR)
> +
> + # don't lock database for read-only access methods
> + # and for interfaces where opts is None (webui)
> + ct = globals.command_table
> + allowed_cmds = ct.ro_methods()
>
I think we've discussed this, 'globals' is dt-specific and should not be
relied on in any Database classes.
> + if not opts or (opts and ct.table[opts.fixed[0]] in allowed_cmds):
> + return None
> +
> + if not os.path.exists(datadir):
> + os.mkdir(datadir, 0755)
> +
> + lock_fname = os.path.join(datadir, LOCK_FILE)
> + self.lockfd = None
> +
> + if not os.path.exists(lock_fname):
> + # if there's no lock file - create it and lock it
> + try:
> + fd = open (lock_fname, "w")
> + fcntl.flock(fd.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
> + except IOError, (errid, strerror):
> + DITrack.Util.common.err("Can't lock file %s: %s" %
> + (lock_fname, strerror))
> + fd.write(repr(os.getpid()))
> + fd.flush()
> + return fd
> + # else if file exists
> + try:
> + fd = open (lock_fname, "r")
> + fcntl.flock(fd.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB)
> + except IOError, (err, strerror):
> + if ((err == errno.EACCES) or (err == errno.EAGAIN)):
> + # Lock on file conent exists
> + pid = open(lock_fname, "r").readline().strip()
> + if not pid.isdigit():
> + raise DITrack.DB.Exceptions.DBLockFileCorruptedError
> + # lockfile exists, but is corrupted
> + raise DITrack.DB.Exceptions.DBIsLockedError(pid)
> + else:
> + raise
> + else:
> + raise DITrack.DB.Exceptions.DBStaleLockError
> +
> + def unlock(self):
> + # if database path is wrong, object doesn't have lockfd attribute
> + if hasattr(self, "lockfd") and self.lockfd:
> + fcntl.flock(self.lockfd.fileno(), fcntl.LOCK_UN)
> + os.unlink(self.lockfd.name)
>
...so all this should go to the new module.
> Modified: src/trunk/DITrack/DB/Exceptions.py
> ===================================================================
> --- src/trunk/DITrack/DB/Exceptions.py 2007-03-29 09:07:44 UTC (rev 1503)
> +++ src/trunk/DITrack/DB/Exceptions.py 2007-04-03 09:51:15 UTC (rev 1504)
> @@ -70,6 +70,16 @@
> class FilterIsPredefinedError(Exception):
> message = "This looks like a predefined filter name"
>
> +class DBIsLockedError(Exception):
> + message = "Database is locked"
> + def __init__(self, pid):
> + self.pid = pid
> +
> +class DBLockFileCorruptedError(Exception):
> + message = "Database lockfile is corrupted"
>
"lock file" ?
> +
> +class DBStaleLockError(Exception):
> + message = "Stale lock"
>
"Database lock file is stale" ?
> #
> # Not errors, but various conditions.
> #
>
> Modified: src/trunk/DITrack/LMA.py
> ===================================================================
> --- src/trunk/DITrack/LMA.py 2007-03-29 09:07:44 UTC (rev 1503)
> +++ src/trunk/DITrack/LMA.py 2007-04-03 09:51:15 UTC (rev 1504)
> @@ -35,10 +35,6 @@
> import DITrack.DB.Issue
> from DITrack.Common import *
>
> -# Local modifications area directory and file name
> -LMA_DIR = ".ditrack"
> -LMA_FILE = "LMA"
> -
> class LocalModsArea:
> """
> Class representing local modifications area.
> @@ -62,12 +58,12 @@
>
> def __init__(self, path):
>
> - datadir = os.path.join(path, LMA_DIR)
> + datadir = os.path.join(path, DITrack.DB.Common.LMA_DIR)
>
> if not os.path.exists(datadir):
> os.mkdir(datadir, 0755)
>
> - fname = os.path.join(datadir, LMA_FILE)
> + fname = os.path.join(datadir, DITrack.DB.Common.LMA_FILE)
>
> self.data = shelve.open(fname)
>
>
> Modified: src/trunk/DITrack/Util/common.py
> ===================================================================
> --- src/trunk/DITrack/Util/common.py 2007-03-29 09:07:44 UTC (rev 1503)
> +++ src/trunk/DITrack/Util/common.py 2007-04-03 09:51:15 UTC (rev 1504)
> @@ -53,7 +53,7 @@
> return opts.var["database"], "specified in command line"
> elif os.environ.has_key(DITRACK_ROOT):
> return os.environ[DITRACK_ROOT], \
> - "specified by " + DITRACK_ROOT + "environment variable"
> + "specified by " + DITRACK_ROOT + " environment variable"
>
It's better to separate logic-affecting changes from cosmetics.
> else:
> return ".", "default path"
>
> @@ -64,7 +64,7 @@
>
> # Now try opening this database.
> try:
> - db = DITrack.DB.Common.Database(dbroot, globals)
> + db = DITrack.DB.Common.Database(dbroot, globals, opts)
> except DITrack.DB.Exceptions.NotDatabaseError, e:
> err("'" + dbroot + "' (" + source + ") is not an issue database root")
> except DITrack.DB.Exceptions.NotDirectoryError, e:
> @@ -73,6 +73,13 @@
> except DITrack.DB.Exceptions.InvalidVersionError, e:
> err("'" + dbroot + "' (" + source + ") is not of a supported database "
> "format version")
> + except DITrack.DB.Exceptions.DBIsLockedError, e:
> + err("Database directory '" + dbroot + "' (" + source +
> + ") is locked, PID=%s" % e.pid)
>
"...is locked by process %d" ?
> + except DITrack.DB.Exceptions.DBLockFileCorruptedError:
> + err("Database %s locked with the corrupted lock file" % dbroot)
> + except DITrack.DB.Exceptions.DBStaleLockError:
> + err("Stale lock on database %s" % dbroot)
> except:
> raise
>
>
> Modified: src/trunk/dt
> ===================================================================
> --- src/trunk/dt 2007-03-29 09:07:44 UTC (rev 1503)
> +++ src/trunk/dt 2007-04-03 09:51:15 UTC (rev 1504)
> @@ -74,6 +74,14 @@
> else:
> usage()
>
> + def ro_methods(self):
> + methods = []
> + for cmd in ["list", "status", "cat"]:
> + if self.table.has_key(cmd):
> + methods.append(self.table[cmd])
> + return methods
>
I didn't quite get the purpose of this construct.
> +
> +
> ############################### FUNCTIONS ###########################
>
> def usage():
>
> Modified: src/trunk/webui/index.cgi
> ===================================================================
> --- src/trunk/webui/index.cgi 2007-03-29 09:07:44 UTC (rev 1503)
> +++ src/trunk/webui/index.cgi 2007-04-03 09:51:15 UTC (rev 1504)
> @@ -175,7 +175,7 @@
> script_url = os.environ["SCRIPT_NAME"]
>
> globals = DITrack.dt.globals.Globals()
> -db = DITrack.DB.Common.Database(dbroot, globals)
> +db = DITrack.DB.Common.Database(dbroot, globals, None)
> dt = DITrack.Client.Client(db)
> form = cgi.FieldStorage()
>
>
> _______________________________________________
> Commit mailing list
> Commit at lists.ditrack.org
> http://lists.ditrack.org/mailman/listinfo/commit
>
--
Vlad Skvortsov, vss at 73rus.com, http://vss.73rus.com
More information about the Dev
mailing list