Ticket #1160 (closed defect: fixed )

Opened 4 years ago

Last modified 3 years ago

FTPClient failed commands return a FirstError

Reported by: orbitz Assigned to: therve
Type: defect Priority: highest
Milestone: Component: ftp
Keywords: Cc: spiv, orbitz, therve
Branch: Author:
Launchpad Bug:

Attachments

Change History

  2005-09-06 21:00:55+00:00 changed by 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?

  2005-09-07 07:04:56+00:00 changed by 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. :)

  2005-09-07 07:11:30+00:00 changed by 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>
> _________________________________________________________

  2005-09-07 07:38:34+00:00 changed by 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.

  2005-09-13 19:32:18+00:00 changed by 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.

  2007-03-20 09:30:30+00:00 changed by therve

  • cc changed from spiv, orbitz to spiv, orbitz, therve
  • keywords set to review
  • status changed from assigned to new
  • owner deleted
  • priority changed from high to highest

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.

  2007-03-30 13:03:41+00:00 changed by jerub

  • keywords deleted
  • owner set to therve

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.

  2007-03-30 16:17:10+00:00 changed by therve

  • status changed from new to closed
  • resolution set to fixed

(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.

  2007-03-30 16:54:17+00:00 changed by therve

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

Refs #1160 Refs #1107

  2007-03-30 16:55:37+00:00 changed by therve

  • status changed from closed to reopened
  • resolution deleted

  2007-03-30 16:55:46+00:00 changed by therve

  • status changed from reopened to new

  2007-03-31 16:22:22+00:00 changed by therve

  • keywords set to review
  • owner changed from therve to jerub

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

  2007-03-31 22:20:29+00:00 changed by jerub

  • keywords deleted
  • owner changed from jerub to therve

Nasty, thanks for fixing that up. Please merge.

  2007-04-01 07:10:15+00:00 changed by therve

  • status changed from new to closed
  • resolution set to fixed

(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.

Note: See TracTickets for help on using tickets.