about "i#52: lock database"

[Vlad Skvortsov]

Ivan Glushkov wrote:
> On 5/15/07, Vlad Skvortsov <vss at 73rus.com> wrote:
>> Ivan Glushkov wrote:
>> >
>> > The final decision should be the following.
>> > 1. DITrack.Util.Locking should provide our project with OS-independent
>> > way for locking and unlocking of any files.
>> > Functions in this module:
>> >   * DITrack.Util.Locking.lock(file_descriptor) - lock file descriptor.
>> > Doesn't return value.
>> >      If this file descriptor has already been locked, raise some
>> > exception (FileIsLockedError?)
>> >   * DITrack.Util.Locking.unlock(file_descriptor) - unlock file
>> > descriptor. Doesn't return value.
>> Several comments.
>>
>> * After giving it another thought, I suggest to place this compatibility
>> layer in DITrack.Compat.Locking, since it's not just a utility 
>> [bikeshed].
> "compatibility" - do you mean os-independent?
> i suppose all the code should not depend on platform, so it's not a
> correct decision to isolate such code into separate module.

Apparently some code does depend on the platform, so the idea is to 
place such a code into a "compat" module.

> from i#155 we have logical splitting into modules (and compatibility
> layer might be everywhere in them):
>
> DITrack/        - everything DITrack-related
>        ThirdParty      - third-party modules
>        Util            - utility/abstraction modules
>        Lib             - public-accessible DITrack interface
>        dt              - command-line client specific modules
>
> file lock function should be the part of "utility/abstraction module".

Ok.

>> * We do need a notion of lock type: shared/read vs. exclusive/write, so
>> it should be mentioned in the arguments.
>
> do we need to distinguish shared/read lock with the absent of lock?
> (i.e. do we need shared/read lock at all? What for?)

Yes, we do. Otherwise data might be modified by another process while we 
are reading it. And the other way around: we shouldn't start modifying 
data until we are sure there are no readers.

This brings us to the topic of a lock lifetime. I think our initial 
approach (locking for the whole lifetime of a Database object) should be 
rejected; the database should be locked for as short time span as 
possible. At the same time locks should ensure the whole database 
consistency. E.g. for the 'ls' command we should lock at the very 
beginning and release the lock at the very end, to guarantee no changes 
take place in the middle of the operation.


>> * We do need a support for blocking on lock (i.e. when we are trying to
>> acquire a read lock, but there is another process holding a write lock:
>> rather then failing right away, the implementation should try waiting
>> for specified amount of time).
>
> i believe that commands that make only reads from database shouldn't
> be blocked. We want to get opportunity to read information from db
> while changing it.

If we are going to implement read locks, we'll need the blocking 
functionality.


[skipped]

>>         lock = DITrack.Compat.Lock.Lock("r", timeout=5)
> hmm... where is "f" (file object) in last string?

Oops, missing.

[skipped]

>> >      This function should just create special file (.ditrack/LOCK for
>> > now) write self pid into it and lock it, saving file descriptor.
>> >      If errors occurs, exceptions would raise (in algorithm described
>> > by Vlad in previous letter):
>> >>       if there is no lock on the file,
>> >>          raise DITrack.DB.Exceptions.DBStaleLockError
>> >>       else
>> >>          if there is sensible PID in the file,
>> >>             raise DITrack.DB.Exceptions.DBIsLockedError(PID)
>> >>          else
>> >>             raise DITrack.DB.Exceptions.DBLockFileCorruptedError
>> >   * DITrack.DB.Common.unlock() - unlock database. Doesn't return 
>> value.
>> >
>> > Proposals?
>>
>> Hmm, looking at these exceptions with fresh eyes, do we really need them
>> all? The caller would [probably] like to _know_ the details (e.g. was
>> there a stale lock, etc), but I guess we should either lock or fail.  So
>> from the API user perspective, the DB constructor should probably raise
>> only DBIsLockedError.
> i don't understand. We do either lock or fail. (return fd or raise
> some exception), don't we?

Yes, and my point [now :-)] is that we should raise only just one exception.

> DBIsLockedError should not be the same as DBStaleLockError, as the
> last one may be catched and processed with some external code.
> DBLockFileCorruptedError is useless, i agree.

Actually, DBStaleLockError is also useless for two reasons:

1. Now, with a concept of multiple readers / one writer, there isn't 
much sense in storing a PID in the lock file at all.

2. Each caller would have to handle the stale lock condition (and most 
likely in exactly the same way: ignore it), so this handling code 
screams to be factored out and encapsulated into the Database object.

-- 
Vlad Skvortsov, vss at 73rus.com, http://vss.73rus.com