FTPClient failed commands return a FirstError

[orbitz]

A failed FTPClient command returns something like:
FirstError(<twisted.python.failure.Failure twisted.internet.defer.FirstError>, 0)
Is this what one wants to be returned?  Is there any good reason to return that
object rather than the error?

[spiv]

No, it's not intentional.  The reason is that the system I wrote the FTP client
for didn't care about details of errors, just whether one had happened or not.
It's a bug in the API, and should be fixed.  I thought there was already a bug
on this, but I'm clearly confusing my mental todo list with the issue tracker :)
FirstError is just an unfortunate implementation details that users of FTPClient
shouldn't have to know about.
And, of course, there should be tests for the fixed behaviour added too. :)

[orbitz]

Is a FirstError what a DeferredList gives when fireOnError is set?  Why
is it a FirstError in a FirstError as well?  To fix, does the inner
error have to be extracted or is it a simple option?
On Sep 6, 2005, at 9:05 PM, Andrew Bennetts [[Twisted issue tracker]]
wrote:
>
> Andrew Bennetts <andrew-twistedbugs@puzzling.org> added the comment:
>
> No, it's not intentional.  The reason is that the system I wrote the
> FTP client
> for didn't care about details of errors, just whether one had happened
> or not.
>
> It's a bug in the API, and should be fixed.  I thought there was
> already a bug
> on this, but I'm clearly confusing my mental todo list with the issue
> tracker :)
>
> FirstError is just an unfortunate implementation details that users of
> FTPClient
> shouldn't have to know about.
>
> And, of course, there should be tests for the fixed behaviour added
> too. :)
>
> ----------
> status: new -> chatting
>
> _________________________________________________________
> Twisted issue tracker <twisted.roundup@twistedmatrix.com>
> <http://twistedmatrix.com/bugs/#1160>
> _________________________________________________________

[spiv]

Right, FirstError is from a DeferredList with fireOnOneErrback set.
And there's two layers of them because inside that part of FTPClient there's two
layers of DeferredList.
Inside those two FirstErrors there's the failure that caused the original
problem.  Without looking into the code (which I don't have time for right at
this moment) I couldn't say if that's the right error to hand to the caller of
FTPClient or not.  If it is, it's just a simple matter of extracting it -- and
writing tests.
Also, I wonder if there may be some value in adding a helper somewhere to make
unwrapping FirstErrors easier, if that is what is needed here.  Perhaps an
'unwrap' method to FirstError?  Use would look like:
    dl = DeferredList(..., fireOnOneErrback=True)
    def unwrapFirstError(failure):
        failure.trap(FirstError)
        failure.unwrap()  # would raise the inner error
    dl.addErrback(unwrapFirstError)
Although even that's a lot of boilerplate... perhaps an "unwrapFirstError=True"
flag to DeferredList instead?  Or unwrapFirstError could be helper function that
lives in defer.py (or as a classmethod on FirstError?)...  Anyway, something to
think about later, after we get some experience with using FirstErrors.

[spiv]

Fixed in r14444, for the passive case anyway.  The fix for the non-passive case
looks trivial, but really should have a test too.

[therve]

I added a test for non-passive. In the process I fixed the problem in #1107.

Ready to review in ftp-client-error-1160+1107.

[jerub]

Hey, this looks excellent. I really wish there wasn't so much verbage in those unit tests - you kinda lose the point of the test when reading it. The Docstring is accurate, and that's what counts.

I think this looks good, I'm okay with it being merged.

[therve]

(In [19930]) Merge ftp-client-error-1160+1107

Author: therve Reviewer: Jerub Fixes #1160 Fixes #1107

Unwrap the FirtError? exception from FTPClient failed commands in non-passive mode, and fire errback of current command (usually happen when losing a connection during a transfer). Add tests for this situation.

[therve]

(In [19931]) Revert r19930: failure in web2 tests.

Refs #1160 Refs #1107

[therve]

There was a problem with another ftp tests, that appears only when the deferred was garbage collected. I corrected the test, it looks fine.

[jerub]

Nasty, thanks for fixing that up. Please merge.

[therve]

(In [19936]) Merge ftp-client-error-1160+1107

Author: therve Reviewer: jerub Fixes #1160 Fixes #1107

Unwrap the FirtError? exception from FTPClient failed commands in non-passive mode, and fire errback of current command (usually happen when losing a connection during a transfer). Add tests for this situation, and fix a test.