107552 - ftp should be able to resume download

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, enhancement, P1)

Description

[Ben Guthrie]

when a previous download of a file has failed before completion,
you should be able to resume the download at the point of failure
and complete the file instead of starting over from the beginning

Comment 1

[Bradley Baetz (:bbaetz)]

Yeah, I've been meaning to file this for a while.

Theres backend work and frontend work, and from a code point of view I need to
make sure that http can use the same interface. I'll start a discussion on
npm.netlib in the next couple of days.

Once thats done, ben can hack on the UI for this.

Comment 2

[R.K.Aa.]

*** Bug 109192 has been marked as a duplicate of this bug. ***

Comment 3

[Francisco León]

bug 98288 covers the need of the UI once this bug gets fixed.

Comment 4

[Francisco León]

Actually, now i am confused. Is resuming currently possive in http? If yes, 
then the dependency i just put is wrong and we need to create a new bug for the 
ui.
If we can share the ui to be triggered in both http and ftp then better

Comment 5

[Bradley Baetz (:bbaetz)]

No, its not, but the API is going to be designed to support http as well. See my
posting to npm.netlib on this.

I have no time, and am unavailable for 0.9.8

->0.9.9, P2

Comment 6

[R.K.Aa.]

*** Bug 112543 has been marked as a duplicate of this bug. ***

Comment 7

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 118091 has been marked as a duplicate of this bug. ***

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: patch

OK, here's a patch. It was actually simpler than I thought.

I haven't tested calling Suspecnd() or Resume() - I want to deprecate using
those for permenantly suspending.

dougt - why exactly do we need teh datarequestforwarder? the ftpchannel is
thats passed in to the observer as the nsirequest in all cases.

I also haven't testing using a non null fileinfo.

Comments?

Comment 9

[Darin Fisher]

bbaetz: mind including a summary of the changes in this patch?  thx!

Comment 10

[Bradley Baetz (:bbaetz)]

OK, heres a summary. First, read
http://groups.google.com/groups?hl=en&selm=slrn9s6udj.2bs.bbaetz%40banana.home
which was original design proposal (I changed it a tiny bit, though)
File by file:

nsIFileInfo.{idl,h,cpp}: Replacement for a 2 element structure. XPCOM is Fun.

nsIResumableChannel.idl: A new API for resuming a download

nsIResumableInfo.idl: A request can be QI'd to this to get information about the
file. the fileinfo not being present means that the server doesn't support
resumption. (Should I keep this in anyway, for consistancy, and have a separate
boolean?)

nsNetUtil.h: helper function of rcreating a new nsIFileInfo.idl

nsIFTPChannel.idl: Inherit from nsIResumableChannel instead.

nsFtpChannel.{cpp,h}: Implement the various interfaces, just forwarding to the
FTPState class

nsFtpConnectionThread.cpp: The DataRequestForwarder forwards the new routines.
The FTPState class is changes to send out a request for the last modification
time, and also to send the REST request to restart from the given offset, and
also parse/setup the nsIFileInfo

nsFtpProtocolHandler: Fix  atype \b -> \n.

TestProtocols.cpp: Test this.

+ the various makefile changes (The tests/Makefile.in change just removes a
duplicate entry which make warned about)

Comment 11

[Bradley Baetz (:bbaetz)]

The URL for the API discussion is
http://groups.google.com/groups?hl=en&selm=slrn9tuhvu.p2d.bbaetz%40banana.home

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 64883 [details] [diff] [review]
patch

I do not like the name of nsIFileInfo.	how about nsIPartialFileInfo?  

Given the length in the nsIPartialFileInfo, why must we pass a start position
to asyncOpenAt()?

Instead of asyncOpenAt(), how about a method which allows you to set|get a
nsIPartialFileInfo?  This would match the uploading semantecs.	It would also
allow to to collapse nsIResumableChannel with nsIResumableInfo.  

Don't include C stuff in the IDL.  If we ever go to freeze these interfaces, we
will have to clean it up.

Maybe the nsIPartialInfo should also accept a nsIFile.

Tabbing problem in base/src/makefile.win

So, in practice, a client will download something.  Something then happens, and
the download is aborted.  The client makes a new request and set a
nsIPartialFileInfo on the channel.  Question is, how is the client going to
know the precise Last modifition date?	Maybe instead of having clients create
this, it could be returned to them through the channel during onStopRequest?  I
guess this puts alot on the client too... hmf.

Comment 13

[Bradley Baetz (:bbaetz)]

> I do not like the name of nsIFileInfo.	how about nsIPartialFileInfo?

Well, its nothing to do with partial files per se - its still sent otherwise

>Given the length in the nsIPartialFileInfo, why must we pass a start position
>to asyncOpenAt()?

the length in nsIFileInfo is the total length of the file, while the length to
asyncOpenAt is the starting offset.

>Instead of asyncOpenAt(), how about a method which allows you to set|get a
>nsIPartialFileInfo?  This would match the uploading semantecs.	It would also
>allow to to collapse nsIResumableChannel with nsIResumableInfo.

See above - they're not the same. nsIFileInfo is a sanity check, so that you
don't resume a differnet file tothe one you started (thats why the length
attribute is dupolicated from getContentLength on nsIChannel - becuase it needs
to be settable, too)

>Don't include C stuff in the IDL.  If we ever go to freeze these interfaces, we
>will have to clean it up.

Where hsould the error code go, then?

>Maybe the nsIPartialInfo should also accept a nsIFile.

Why? they're different, since the last modification time won't be the same.

> Tabbing problem in base/src/makefile.win

Its a makefile

>So, in practice, a client will download something.  Something then happens, and
>the download is aborted.  The client makes a new request and set a
>nsIPartialFileInfo on the channel.  Question is, how is the client going to
>know the precise Last modifition date?

By QI'ing the nsIRequest to nsIFileInfo in the OnStart, and storing the results.

>Maybe instead of having clients create
>this, it could be returned to them through the channel during onStopRequest?  I
>guess this puts alot on the client too... hmf.

See above. They can QI on OnStop if they prefer, of course.

Comment 14

[Darin Fisher]

how about this:

interface nsIEntityID : nsISupports
{
  attribute long   size;
  attribute PRTime lastModified;
  attribute string tag;
  
  boolean equals(in nsIEntityID entityID);
};

interface nsIResumeableChannel : nsISupports
{
  readonly attribute nsIEntityID entityID;

  void asyncOpenAt(in nsIStreamListener listener,
                   in nsISupports ctxt,
                   in unsigned long startPos,
                   in nsIEntityID entityID);
};

then you get rid of nsIResumeableInfo (it really is the channel).  i like
nsIEntityID or nsIEntityInfo much better than nsIFileInfo.  this resume stuff
doesn't really translate to files... it could be used with some other target
besides the filesystem.  and, entity seems to match very well with ETag... the
point being that you need something to identify the entity be loaded.

  nsIEntityID, nsIEntityInfo, nsINetEntityID, nsINetEntityInfo,... 

or...

  nsIResumeableEntityInfo

or...
 
  nsIResumeableEntityID

i think i prefer this last one.

Comment 15

[Bradley Baetz (:bbaetz)]

Attachment: new patch

OK, new patch. I changed the various bits, fixed the compile errors, and it
Just Worked.

Comments?

Comment 16

[Bradley Baetz (:bbaetz)]

Comment on attachment 66857 [details] [diff] [review]
new patch

OK, I stuffed up the cache interactions (checked for 0, but I ignored -1, so
normal downloads wer enever cached). I'm also considering disabling resuming
directory downloads. If I enable those, then I'd have to check for the
entityID. I'd have to shuffle code arround, but since directories don't specify
a SIZE, and quite a few servers won't give a lst modification time, I'm ont
sure if I see the point. Maybe I'll jsut require a null entityID to use the
cache. This is an edge cache, since I'm deisabling the cache for resuming,
anyway.

This doesn't stop comments on the rest of the patch...

Comment 17

[Syd Logan]

only nsbeta1+ bugs can have milestones, resetting to ---

Comment 18

[Bradley Baetz (:bbaetz)]

Just because my email has 'syd' in it doesn't mean that my bugs are syd's....
Resetting milestone

I'll hack some more on this one today

Comment 19

[Bradley Baetz (:bbaetz)]

Attachment: v3

I've disabled the cache if we resume from somewhere which isn't the beginning,
or if we have an entity id (adding in code to store this as meta data isn't
hard, but since the cache won't do the partial downloads its not worth it,
since being given an entity id and starting from the beginning is a silly and
useless case).

I also assert if we get a directory listing and been asked to resume - this
code doesn't support that (and may not ever, given the uselsess mdtm times for
directories most servers give)

Comment 20

[Darin Fisher]

1- comments in nsIResumableEntityID.idl make reference to "file" which should
probably be replaced with entity.

2- error 21 is used for NS_ERROR_FTP_LOGIN

3- NS_ERROR_NOT_RESUMABLE should probably be defined in nsIResumableChannel.idl
for now... i know that i have a bug to move everything into one header file for
necko, but for now, i think we should "stick with the program" ;)

4- long long should be PRTime in IDL

5- ISUPPORTS MAP shouldn't be necessary now that nsIResumableChannel inherits
from nsISupports instead of nsIChannel

otherwise this patch looks good to me.  please submit a revised patch so i can
mark it as sr='d.

Comment 21

[Bradley Baetz (:bbaetz)]

3) dougt told me to. I'll move it back, I guess
5) Bug 123991 is going to mean another interface on this, so I'd prefer to leave
this as it is.

I'll have another patch tomorrow, fixing anything dougt comments on too.

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 68267 [details] [diff] [review]
v3

Nice work.  

Any patches to make this work in the mozilla executable?  Should we land this
patch without any gain on the application side?  Risk vs. Gain. 

Let the super review make that call.  :-)

r=dougt

Comment 23

[Bradley Baetz (:bbaetz)]

Blake was going to do that for the d/l manager. Don't know if he still is. It
has a test program, though....

Comment 24

[Darin Fisher]

Comment on attachment 68267 [details] [diff] [review]
v3

sr=darin (w/ or w/o 3 & 5)

perhaps once this is in we can push to get the download manager to make use of
this feature.

Comment 25

[Bradley Baetz (:bbaetz)]

Attachment: v4

Well, whilst I made the changes, I never uploaded the patch. The only other
change is that I propogated the PRInt64->PRTime to the implementations, and
used LL_INIT for the default value so that it would compile on platforms
without |long long|

Comment 26

[Bradley Baetz (:bbaetz)]

Comment on attachment 69355 [details] [diff] [review]
v4

r=dougt on AIM

Comment 27

[Darin Fisher]

Comment on attachment 69355 [details] [diff] [review]
v4

sr=darin

Comment 28

[Zach Lipton [:zach]]

Attachment: changes for the mac build system

Apply these changes with v4 to get the mac project files set right. Built and
tested on mac.

Comment 29

[Bradley Baetz (:bbaetz)]

Checked in. This is just the backend; blake will be doing UI for this, I believe

Comment 30

[benc]

Annything I can do to verify this w/o the front-end?

Comment 31

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old