sftp locks can get stuck

[Robey Pointer]

Sorry I didn't reply to this earlier.  My mailbox grew out of my  
control over the holidays.


On 24 Dec 2005, at 11:29, John A Meinel wrote:

> Robey Pointer wrote:
>> On 23 Dec 2005, at 5:59, John A Meinel wrote:
>>> Robey Pointer wrote:
>>>>
>>>> I think simpler is better here:  We record tracking info in the  
>>>> write
>>>> lock (email, IP, timestamp) and provide a way to explicitly  
>>>> break the
>>>> lock.  Maybe if an operation fails because of a write lock, and  
>>>> that
>>>> lock is pretty old (by some arbitrary standard), bzr could hint  
>>>> to the
>>>> user:
>>>>
>>>>     The branch was locked by <robey at lag.net> 5 hours ago.
>>>>     If the lock is stale, you can break it with: bzr break-lock
>>>>
>>>> But no magic of trying to guess if the lock should be automatically
>>>> broken, etc.  Stale locks should be relatively rare.
>>>
>>> I'm +1 for adding the username, and not automatically breaking  
>>> the lock.
>>>  I don't think the ip address helps much, but it is easy to  
>>> record. I
>>> don't think we need the timestamp, because we can just stat() the  
>>> lock
>>> file to figure out what time it was taken out. I suppose we could  
>>> add
>>> the UCT timestamp to help with handling time-zone shift.
>> Exactly, yeah.
>> So probably what we should do is move the locking outward (to  
>> Branch?) and have Transport just provide something like
>>     create_lock_file(path, text_content)
>> and raise an exception if the lock file couldn't be created (via  
>> O_EXCL or whatever the transport's equivalent is).
>> If that sounds okay I can probably cook up a patch.
>> robey
>
> Well, transports already provide "lock_read()" and "lock_write()"  
> which are supposed to 'lock' a specific file. (sftp implements this  
> as creating a file with O_EXCL, local implements this as asking the  
> OS to lock the file).
>
> The question is how do we really want to do this. Do we want to  
> give up the auto-unlock properties of OS locks, because of SFTP. Or  
> do we just want SFTPTransport to add extra information to the lock.

I think it would be good to have all transports using the same lock.   
So that if someone is accessing a branch via HTTP and someone else  
via SFTP, they still don't trample over each other.

Whether we want to support concurrent local & remote access is a  
debatable question.  I'm leaning toward "yes".


> If we are okay playing the least-common-denominator game, then I  
> would say we change the Transport.lock_write() call, so that it  
> asks for a text_content string, and then LocalTransport will act  
> like SFTPTransport and create an O_EXCL file, and fill it with the  
> content.

Yeah what I would suggest is even to abstract it further and have the  
filename provided, too, so that handling where the lockfile is  
created is part of Storage or higher.

robey