If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

ftp should be able to resume download

RESOLVED FIXED in mozilla0.9.9

Status

()

Core
Networking: FTP
P1
enhancement
RESOLVED FIXED
16 years ago
4 years ago

People

(Reporter: Ben Guthrie, Assigned: bbaetz)

Tracking

Trunk
mozilla0.9.9
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

16 years ago
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
(Assignee)

Comment 1

16 years ago
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.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → mozilla0.9.7

Comment 2

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

Comment 3

16 years ago
bug 98288 covers the need of the UI once this bug gets fixed.
Blocks: 98288

Comment 4

16 years ago
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
(Assignee)

Comment 5

16 years ago
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
Priority: P3 → P2
Target Milestone: mozilla0.9.7 → mozilla0.9.9

Comment 6

16 years ago
*** Bug 112543 has been marked as a duplicate of this bug. ***
*** Bug 118091 has been marked as a duplicate of this bug. ***
Blocks: 18004
(Assignee)

Comment 8

16 years ago
Created attachment 64883 [details] [diff] [review]
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

16 years ago
bbaetz: mind including a summary of the changes in this patch?  thx!
(Assignee)

Comment 10

16 years ago
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)
(Assignee)

Comment 11

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

Comment 12

16 years ago
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.
Attachment #64883 - Flags: needs-work+
(Assignee)

Comment 13

16 years ago
> 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

16 years ago
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.
(Assignee)

Comment 15

16 years ago
Created attachment 66857 [details] [diff] [review]
new patch

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

Comments?
Attachment #64883 - Attachment is obsolete: true
(Assignee)

Comment 16

16 years ago
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...
Attachment #66857 - Flags: needs-work+

Comment 17

16 years ago
only nsbeta1+ bugs can have milestones, resetting to ---
Target Milestone: mozilla0.9.9 → ---
(Assignee)

Comment 18

16 years ago
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
Status: NEW → ASSIGNED
Priority: P2 → P1
Target Milestone: --- → mozilla0.9.9
(Assignee)

Comment 19

16 years ago
Created attachment 68267 [details] [diff] [review]
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)
Attachment #66857 - Attachment is obsolete: true

Comment 20

16 years ago
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.
(Assignee)

Comment 21

16 years ago
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

16 years ago
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
Attachment #68267 - Flags: review+
(Assignee)

Comment 23

16 years ago
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

16 years ago
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.
Attachment #68267 - Flags: superreview+
(Assignee)

Comment 25

16 years ago
Created attachment 69355 [details] [diff] [review]
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|
Attachment #68267 - Attachment is obsolete: true
(Assignee)

Comment 26

16 years ago
Comment on attachment 69355 [details] [diff] [review]
v4

r=dougt on AIM
Attachment #69355 - Flags: review+

Comment 27

16 years ago
Comment on attachment 69355 [details] [diff] [review]
v4

sr=darin
Attachment #69355 - Flags: superreview+

Comment 28

16 years ago
Created attachment 69389 [details] [diff] [review]
changes for the mac build system

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

Comment 29

16 years ago
Checked in. This is just the backend; blake will be doing UI for this, I believe
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 30

15 years ago
Annything I can do to verify this w/o the front-end?
Keywords: verifyme

Updated

14 years ago
No longer blocks: 98288

Updated

13 years ago
Blocks: 230870
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.