Support for partial HTTP transfers
PATCH 2/3 Support for partial HTTP transfers
Support for partial HTTP transfers - if a previous temp file is detected, read it in and start the HTTP transfer from where the previous left off.
---
http-fetch.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 67 insertions(+), 5 deletions(-)
34a692953368188cbaefbd6c60e400053f8528b4
http-fetch.c +67 -5
@@ -13,8 +13,12 @@ #define curl_global_init(a) do { /* nothing */ } while(0)#endif +#define PREV_BUF_SIZE 4096 +#define RANGE_HEADER_SIZE 30 +static CURL *curl;static struct curl_slist *no_pragma_header; +static struct curl_slist *no_range_header;static char *initial_base;
@@ -351,14 +355,26 @@ int fetch_object(struct alt_base *repo,char *filename =sha1_file_name (sha1);unsigned char real_sha1[20 ];char tmpfile[PATH_MAX ]; +char prevfile[PATH_MAX ];int ret;char *url;char *posn; +int prevlocal; +unsigned char prev_buf[PREV_BUF_SIZE ]; +ssize_t prev_read =0 ; +long prev_posn =0 ; +char range[RANGE_HEADER_SIZE ]; +struct curl_slist *range_header =NULL ; +CURLcode curl_result; + +snprintf (tmpfile,sizeof (tmpfile),"%s.temp" , filename); +snprintf (prevfile,sizeof (prevfile),"%s.prev" , filename); +unlink (prevfile); +rename (tmpfile, prevfile); +unlink (tmpfile); -snprintf (tmpfile,sizeof (tmpfile),"%s/obj_XXXXXX" , -get_object_directory ()); + local =open (tmpfile,O_WRONLY |O_CREAT |O_EXCL ,0666 ); - local =mkstemp (tmpfile);if (local <0 )return error ("Couldn't create temporary file %s for %s: %s\n" , tmpfile, filename,strerror (errno));
@@ -386,8 +402,52 @@ int fetch_object(struct alt_base *repo,curl_easy_setopt (curl,CURLOPT_URL , url); -if (curl_easy_perform (curl)) { -unlink (filename); +/* If a previous temp file is present, process what was already +fetched. */ + prevlocal =open (prevfile,O_RDONLY ); +if (prevlocal !=-1 ) { +do { + prev_read =read (prevlocal, prev_buf,PREV_BUF_SIZE ); +if (prev_read>0 ) { +if (fwrite_sha1_file (prev_buf, +1 , + prev_read, +NULL ) == prev_read) { + prev_posn += prev_read; + }else { + prev_read =-1 ; + } + } + }while (prev_read >0 ); +close (prevlocal); + } +unlink (prevfile); + +/* Reset inflate/SHA1 if there was an error reading the previous temp +file; also rewind to the beginning of the local file. */ +if (prev_read ==-1 ) { +memset (&stream,0 ,sizeof (stream)); +inflateInit (&stream); +SHA1_Init (&c); +if (prev_posn>0 ) { + prev_posn =0 ; +lseek (local,SEEK_SET ,0 ); + } + } + +/* If we have successfully processed data from a previous fetch +attempt, only fetch the data we don't already have. */ +if (prev_posn>0 ) { +sprintf (range,"Range: bytes=%ld-" , prev_posn); + range_header =curl_slist_append (range_header, range); +curl_easy_setopt (curl,CURLOPT_HTTPHEADER , range_header); + } + +/* Clear out the Range: header after performing the request, so +other curl requests don't inherit inappropriate header data */ + curl_result =curl_easy_perform (curl); +curl_easy_setopt (curl,CURLOPT_HTTPHEADER , no_range_header); +if (curl_result !=0 ) {return -1 ; }
@@ -517,6 +577,7 @@ int main(int argc, char **argv) curl =curl_easy_init (); no_pragma_header =curl_slist_append (no_pragma_header,"Pragma:" ); + no_range_header =curl_slist_append (no_range_header,"Range:" );/* Set SSL parameters if they were provided */ if (ssl_cert !=NULL ) {
@@ -549,6 +610,7 @@ int main(int argc, char **argv)return 1 ;curl_slist_free_all (no_pragma_header); +curl_slist_free_all (no_range_header);curl_global_cleanup ();return 0 ; }
Quoted 2 lines · Nick Hengeveld
Support for partial HTTP transfers - if a previous temp file is detected, read it in and start the HTTP transfer from where the previous left off.
Please read my recent posting:
http://permalink.gmane.org/gmane.comp.version-control.git/8991.
Regards
Stephan
http-fetch.c int fetch_object(struct alt_base *repo, · line 374
Quoted 3 lines · Nick Hengeveld
http-fetch.c +3
-0+unlink (prevfile); +rename (tmpfile, prevfile); +unlink (tmpfile);
If rename() succeeds then tmpfile is no more. If rename() fails because there were no tmpfile to begin with, it is not an error (i.e. you are not recovering) and that case there would not be tmpfile either. Otherwise, if tmpfile still remains to unlink() because rename() failed for any other reason, wouldn't you rather report it as an error and abort?
I wonder what happens if by mistake or intentionally we run two http-fetch instances simultaneously. IIRC, the current code is safe -- the resulting object database will have the object file fetched by one of the instance, and the updating of ref is done via write_ref_sha1(), so it also is safe. But your change may introduce an interesting case where one creates a tmpfile, the other one moves it to prevfile and starts using its partial contents, and possibly gets confused -- it will probabaly fail at the end detecting inconsistent object so it is probably not a big loss.
Personally, I do not think people would mind much if we introduce the BKL at the beginning of git-fetch.sh to prevent multiple fetches stomping on each other, if somebody cared enough (hint, hint).
http-fetch.c int fetch_object(struct alt_base *repo, · line 361
Quoted 4 lines
http-fetch.c +1
-3-snprintf (tmpfile,sizeof (tmpfile),"%s/obj_XXXXXX" , -get_object_directory ()); + local =open (tmpfile,O_WRONLY |O_CREAT |O_EXCL ,0666 ); - local =mkstemp (tmpfile);
I introduced this mkstemp() part recently to mimic the local object creation where newly created object files are built in the same directory, hoping that they would be allocated close to each other when more than one are created in sequence. I do not know it matters much in practice -- has anybody measured, and does anybody care?
http-fetch.c int fetch_object(struct alt_base *repo, · line 427
Quoted 2 lines
http-fetch.c +2
-0+/* Reset inflate/SHA1 if there was an error reading the previous temp +file; also rewind to the beginning of the local file. */
Maybe not just rewind but truncate as well? It probably does not matter in practice much, but previous representation your fetch was interrupted in the middle could have been much larger than the representation you are slurping right now.
There was a discussion about an object file of the same SHA1 and the same contents can have different compressed representations (we hash then compress so the resulting filesize depends on the compression level without affecting the contents of the object). In a "doctor, it hurts when I do this -- don't do it, then" kind of corner case, a DNS rotated pair of webservers could be serving the same object in different representations and you may get interrupted while fetching from one, and restart the transfer from the other. The SHA1 check at the end hopefully would catch this kind of situation, and that round of http-fetch would fail -- the user needs to re-run the fetch so it is not a big loss, but it is something to keep in mind.
Quoted 6 lines · Junio C Hamano
If
rename()succeeds then tmpfile is no more. Ifrename()fails because there were no tmpfile to begin with, it is not an error (i.e. you are not recovering) and that case there would not be tmpfile either. Otherwise, if tmpfile still remains tounlink()becauserename()failed for any other reason, wouldn't you rather report it as an error and abort?
I didn't see that as a fatal condition since the transfer would still take place, albeit a full transfer rather than a partial one. It does seem worth at least reporting it as a warning though. If a previous tmpfile exists and both the rename and unlink fail, the subsequent tmpfile open will also fail - unlikely but probably also worth detecting and reporting.
Quoted 10 lines
I wonder what happens if by mistake or intentionally we run two http-fetch instances simultaneously. IIRC, the current code is safe -- the resulting object database will have the object file fetched by one of the instance, and the updating of ref is done via
write_ref_sha1(), so it also is safe. But your change may introduce an interesting case where one creates a tmpfile, the other one moves it to prevfile and starts using its partial contents, and possibly gets confused -- it will probabaly fail at the end detecting inconsistent object so it is probably not a big loss.
How about using mkstemp on the prev file to keep multiple instances from stepping on each other? Since O_CREAT | O_EXCL is used to open the tmpfile, only one instance will be able to succeed and continue.
Quoted 7 lines
http-fetch.c int fetch_object(struct alt_base *repo, · line 427
Quoted 2 lines
http-fetch.c +2
-0+/* Reset inflate/SHA1 if there was an error reading the previous temp +file; also rewind to the beginning of the local file. */ Maybe not just rewind but truncate as well? It probably does not matter in practice much, but previous representation your fetch was interrupted in the middle could have been much larger than the representation you are slurping right now.
Good point, I'll update the patch.
Quoted 4 lines
There was a discussion about an object file of the same SHA1 and the same contents can have different compressed representations (we hash then compress so the resulting filesize depends on the compression level without affecting the contents of the object).
Quoted 8 lines
In a "doctor, it hurts when I do this -- don't do it, then" kind of corner case, a DNS rotated pair of webservers could be serving the same object in different representations and you may get interrupted while fetching from one, and restart the transfer from the other. The SHA1 check at the end hopefully would catch this kind of situation, and that round of http-fetch would fail -- the user needs to re-run the fetch so it is not a big loss, but it is something to keep in mind.
That's an annoying case, all right... Would it be worth including a full retry if a partial failed the SHA1 check?
Signature · 2 lines
For a successful technology, reality must take precedence over public relations, for nature cannot be fooled.
Quoted 4 lines · Nick Hengeveld
How about using mkstemp on the prev file to keep multiple instances from stepping on each other? Since
O_CREAT|O_EXCLis used to open the tmpfile, only one instance will be able to succeed and continue.
I think (O_CREAT|O_EXCL) in your code can be easily defeated by this sequence:
one two
unlink prevfile
rename tmpfile, prevfile
unlink tmpfile
local = open tmpfile
unlink prevfile rename tmpfile, prevfile unlink tmpfile local = open tmpfile prevlocal = open prevfile
write local
copyfile prevlocal, local
write local
???
Not that I think the multiple instances should be prevented at this low level --- if they stomp on each other at this level, it is very likely the they are doing duplicated work on the network side as well, and should be prevented from doing so at much higher level than this, I think. That's why I said I do not mind BKL upfront in git-fetch.sh.
Quoted 2 lines
That's an annoying case, all right... Would it be worth including a full retry if a partial failed the SHA1 check?
Probably not. Just failing loudly and asking the upper layer to retry would be fine, I think.
Quoted 5 lines · Junio C Hamano
I think (O_CREAT|O_EXCL) in your code can be easily defeated by this sequence:
one two
Quoted 13 lines
unlink prevfile
rename tmpfile, prevfile
unlink tmpfile
local = open tmpfile
unlink prevfile rename tmpfile, prevfile unlink tmpfile local = open tmpfile prevlocal = open prevfile
write local
copyfile prevlocal, local
write local
???
The way I understand this sequence, one and two will not be writing to the same file. When two unlinks tmpfile, one will still be able to write to its local, but the body of the file that one is writing will be removed when it closes local.
Quoted 6 lines
Not that I think the multiple instances should be prevented at this low level --- if they stomp on each other at this level, it is very likely the they are doing duplicated work on the network side as well, and should be prevented from doing so at much higher level than this, I think. That's why I said I do not mind BKL upfront in
git-fetch.sh.
True, is that something I should include with the partial patch?
Signature · 2 lines
For a successful technology, reality must take precedence over public relations, for nature cannot be fooled.
Quoted 4 lines · Nick Hengeveld
The way I understand this sequence, one and two will not be writing to the same file. When two unlinks tmpfile, one will still be able to write to its local, but the body of the file that one is writing will be removed when it closes local.
True. What I meant by "interesting" is that two is reading from what one is writing.
Quoted 8 lines
Quoted 6 lines
Not that I think the multiple instances should be prevented at this low level --- if they stomp on each other at this level, it is very likely the they are doing duplicated work on the network side as well, and should be prevented from doing so at much higher level than this, I think. That's why I said I do not mind BKL upfront in
git-fetch.sh.True, is that something I should include with the partial patch?
No.
Quoted 2 lines · Junio C Hamano
True. What I meant by "interesting" is that two is reading from what one is writing.
Excellent point. Given the potential for problems related to this issue or to compressed representation, would it make sense to enable partial transfers via a command-line option and leave the feature disabled by default?
Signature · 2 lines
For a successful technology, reality must take precedence over public relations, for nature cannot be fooled.
Quoted 9 lines · Nick Hengeveld
Quoted 2 lines · Junio C Hamano
True. What I meant by "interesting" is that two is reading from what one is writing.
Excellent point. Given the potential for problems related to this issue or to compressed representation, would it make sense to enable partial transfers via a command-line option and leave the feature disabled by default?
Here is what I think. Multiple transfers at the same time, if it becomes problem in pratcice, should be prevented at the upper layer anyway, so this "interesting" problem is a moot point. So I'd say let's not worry about that.
Doing partial transfer support for packs and indices would be more useful addition to your code than dealing with the above -- packs are much bigger than individual objects and would get much bigger benefit from restartable download.
Different compressed representation is just a theoretical curiosity and we do not know if it is a problem in practice. As long as you can detect the situation and die, it would be OK, I think.
Quoted 2 lines · Nick Hengeveld
Support for partial HTTP transfers - if a previous temp file is detected, read it in and start the HTTP transfer from where the previous left off.
You probably want the corresponding code for where it's downloading pack and index files, which tend to be a lot bigger than individual objects.
-Daniel
*This .sig left intentionally blank*