[ZODB-Dev] FileStorage.pack: why do we need the commit_lock (so long)

[Dieter Maurer]

I had a look at the new (Zope 2.6.2) "FileStorage.pack" implementation.

I have been confused by the "commit_lock" handling:

  It is acquired inside the new "packer" class but released outside
  of it in "FileStorage.pack".

  At least a comment should indicate that the caller is expected
  to release the lock. It might be even better to move
  code in "FileStorage.pack" up to the "release" into the
  packer, to have lock acquisition and release clearly together.


Another issue with the commit lock:

  It is temporarily released in the "packer". However,
  the "locked" flag is not updated, accordingly.
  "FileStorage.pack" will release the lock when it sees
  the "locked" attribute set (in a "try: finally:").
  However, when an exception is raised while the lock is
  released, we will get an additional exception in
  the release (as the lock is already released) which
  masks the first exception.
  "packer.locked" should be updated as the commit lock
  is acquired and released.


The next question: why do we need the commit lock at all?

  I see the necessity that we must protect our check of
  the terminiation condition. If we see that we have
  copied the complete content to the new file, we must
  keep the lock until the files are renamed and the new
  file copied.
  The storage lock seems to be used for this purpose.

  However, I do not see why we must held the commit lock.
  The fact, that we release it temporarily (during the copying
  of a single transaction)  indicates
  that it does not need to be held during the copying.
  Thus, it seems to be somehow relevant for the check
  of the termination condition. But for this, we
  need the storage lock anyway.
  Thus, what's to modivation for tampering with both locks?


Dieter