[BUG] push over sftp hangs when no room on remote location

[John Arbash Meinel]

Alexander Belchenko wrote:
> Situation: user push over sftp. On remote site there is no room to
> complete push. bzr hangs until user manually remove some files and free
> space appears.
> 
> Log attached.
> 
> Alexander
> 

Does it hang forever, or just for 5 minutes until bzr would give up
trying to grab a lock?

I think the issue is we would like to distinguish between a
PermissionDenied (which means you can't ever do it unless something else
changes), and a FileExists (which means the lock directory already exists).

Though the other possibility is to just change when we catch certain
exceptions. Specifically, the code is:

try:
    tmpname = '%s/pending.%s.tmp' % (self.path, rand_chars(20))
    try:
        self.transport.mkdir(tmpname)
    except NoSuchFile:
        # This may raise a FileExists exception
        # which is okay, it will be caught later and determined
        # to be a LockContention.
        self.create(mode=self._dir_modebits)

        # After creating the lock directory, try again
        self.transport.mkdir(tmpname)

    info_bytes = self._prepare_info()
    # We use put_file_non_atomic because we just created a new unique
    # directory so we don't have to worry about files existing there.
    # We'll rename the whole directory into place to get atomic
    # properties
    self.transport.put_bytes_non_atomic(tmpname + self.__INFO_NAME,
                                        info_bytes)

    self.transport.rename(tmpname, self._held_dir)
    self._lock_held = True
    self.confirm()
except (PathError, DirectoryNotEmpty, FileExists, ResourceBusy), e:
    mutter("contention on %r: %s", self, e)
    raise LockContention(self)


What we could do instead is:

tmpname = '%s/pending.%s.tmp' % (self.path, rand_chars(20))
try:
    self.transport.mkdir(tmpname)
except NoSuchFile:
    # This may raise a FileExists exception
    # which is okay, it will be caught later and determined
    # to be a LockContention.
    self.create(mode=self._dir_modebits)

    # After creating the lock directory, try again
    self.transport.mkdir(tmpname)

try:
    info_bytes = self._prepare_info()
    # We use put_file_non_atomic because we just created a new unique
    # directory so we don't have to worry about files existing there.
    # We'll rename the whole directory into place to get atomic
    # properties
    self.transport.put_bytes_non_atomic(tmpname + self.__INFO_NAME,
                                        info_bytes)

    self.transport.rename(tmpname, self._held_dir)
    self._lock_held = True
    self.confirm()
except (PathError, DirectoryNotEmpty, FileExists, ResourceBusy), e:
    mutter("contention on %r: %s", self, e)
    raise LockContention(self)


The key bit here is that we don't catch FileExists when we are trying to
mkdir() our temporary location. We only catch FileExists when we are
trying to rename() our temp location to the held/ status.

While we're discussing this, we would really like to either re-use an
existing temporary directory, or clean up the directory. Otherwise if
you ever have lock contention, you end up with a whole bunch of
pending-xaoeuaoeu directories lying around that never go away.

Anyway, does this first change fix your problem? I think the second
change would involve moving the code into a helper, which can return an
existing directory if it has already opened one.

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061207/ead17f8f/attachment.pgp