Opened 13 years ago

Closed 13 years ago

#1805 defect closed fixed (fixed)

SFTP adapter doesn't translate deferred errors

Reported by: spiv Owned by:
Priority: normal Milestone:
Component: vfs Keywords:
Cc: Branch:
Author:

Description

The translateErrors decorator in twisted/vfs/adapters/sftp.py doesn't catch and translate errors if the decorated function returns a Deferred.

(Although twisted.vfs isn't really properly able to return Deferreds everywhere, aside from this bug it works just fine in the case of SFTP, because Conch works just fine with them.)

Change History (7)

comment:1 Changed 13 years ago by spiv

Keywords: review added

comment:2 Changed 13 years ago by spiv

See source:branches/vfs-sftp-deferred-errors-1805/twisted/vfs

comment:3 Changed 13 years ago by Jean-Paul Calderone

Keywords: review removed

_eb_translateErrors doesn't follow the naming convention. contained underscores are for separating static prefixes from dynamic suffixes which are programmatically dispatched on.

Also I don't think this code will work with a Deferred which hasn't already fired.

   def testTranslateDeferredError(self):
       # If the decorated function returns a Deferred, the error should still
       # be translated.
       d = defer.Deferred()
       def f():
           return d
       f = sftp.translateErrors(f)
       d2 = f()
       d.errback(ivfs.VFSError("foo"))
       return self.assertFailure(d2, SFTPError)

The code would seem to be better off doing error handling with an errback and using maybeDeferred.

comment:4 Changed 13 years ago by spiv

Keywords: review added
Owner: changed from spiv to Jean-Paul Calderone

I've fixed the naming issue.

I've added that test, and it passes. The key part is that the _ebtranslateErrors errback is itself decorated with translateErrors, so it gets the exact same error handling as the synchronous case.

Is this ok to merge?

comment:5 Changed 13 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from Jean-Paul Calderone to spiv

Looks good, just turn the comments into docstrings and merge.

comment:6 Changed 13 years ago by spiv

Resolution: fixed
Status: newclosed

This was fixed by changeset 17140, 2 months ago...

comment:7 Changed 8 years ago by <automation>

Owner: spiv deleted
Note: See TracTickets for help on using tickets.