{"thread":{"id":"1952","subject":"[PATCH 2/3] Support for partial HTTP transfers","startedAt":"2005-09-26T17:52:04Z","lastAt":"2005-09-27T22:24:20Z","messageCount":10,"participants":["Nick Hengeveld","Junio C Hamano","Daniel Barkalow","sf"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"9309","messageId":"20050926175204.GC9410@reactrix.com","threadId":"1952","inReplyTo":null,"subject":"[PATCH 2/3] Support for partial HTTP transfers","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2005-09-26T17:52:04Z","receivedAt":"2005-09-26T17:52:04Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"\nSupport for partial HTTP transfers - if a previous temp file is detected,\nread it in and start the HTTP transfer from where the previous left off.\n\nSigned-off-by: Nick Hengeveld \n\n\n---\n\n http-fetch.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----\n 1 files changed, 67 insertions(+), 5 deletions(-)\n\n34a692953368188cbaefbd6c60e400053f8528b4\ndiff --git a/http-fetch.c b/http-fetch.c\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -13,8 +13,12 @@\n #define curl_global_init(a) do { /* nothing */ } while(0)\n #endif\n \n+#define PREV_BUF_SIZE 4096\n+#define RANGE_HEADER_SIZE 30\n+\n static CURL *curl;\n static struct curl_slist *no_pragma_header;\n+static struct curl_slist *no_range_header;\n \n static char *initial_base;\n \n@@ -351,14 +355,26 @@ int fetch_object(struct alt_base *repo, \n \tchar *filename = sha1_file_name(sha1);\n \tunsigned char real_sha1[20];\n \tchar tmpfile[PATH_MAX];\n+\tchar prevfile[PATH_MAX];\n \tint ret;\n \tchar *url;\n \tchar *posn;\n+\tint prevlocal;\n+\tunsigned char prev_buf[PREV_BUF_SIZE];\n+\tssize_t prev_read = 0;\n+\tlong prev_posn = 0;\n+\tchar range[RANGE_HEADER_SIZE];\n+\tstruct curl_slist *range_header = NULL;\n+\tCURLcode curl_result;\n+\n+\tsnprintf(tmpfile, sizeof(tmpfile), \"%s.temp\", filename);\n+\tsnprintf(prevfile, sizeof(prevfile), \"%s.prev\", filename);\n+\tunlink(prevfile);\n+\trename(tmpfile, prevfile);\n+\tunlink(tmpfile);\n \n-\tsnprintf(tmpfile, sizeof(tmpfile), \"%s/obj_XXXXXX\",\n-\t\t get_object_directory());\n+\tlocal = open(tmpfile, O_WRONLY | O_CREAT | O_EXCL, 0666);\n \n-\tlocal = mkstemp(tmpfile);\n \tif (local < 0)\n \t\treturn error(\"Couldn't create temporary file %s for %s: %s\\n\",\n \t\t\t tmpfile, filename, strerror(errno));\n@@ -386,8 +402,52 @@ int fetch_object(struct alt_base *repo, \n \n \tcurl_easy_setopt(curl, CURLOPT_URL, url);\n \n-\tif (curl_easy_perform(curl)) {\n-\t\tunlink(filename);\n+\t/* If a previous temp file is present, process what was already\n+\t fetched. */\n+\tprevlocal = open(prevfile, O_RDONLY);\n+\tif (prevlocal != -1) {\n+\t\tdo {\n+\t\t\tprev_read = read(prevlocal, prev_buf, PREV_BUF_SIZE);\n+\t\t\tif (prev_read>0) {\n+\t\t\t\tif (fwrite_sha1_file(prev_buf,\n+\t\t\t\t\t\t 1,\n+\t\t\t\t\t\t prev_read,\n+\t\t\t\t\t\t NULL) == prev_read) {\n+\t\t\t\t\tprev_posn += prev_read;\n+\t\t\t\t} else {\n+\t\t\t\t\tprev_read = -1;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t} while (prev_read > 0);\n+\t\tclose(prevlocal);\n+\t}\n+\tunlink(prevfile);\n+\n+\t/* Reset inflate/SHA1 if there was an error reading the previous temp\n+\t file; also rewind to the beginning of the local file. */\n+\tif (prev_read == -1) {\n+\t\tmemset(&stream, 0, sizeof(stream));\n+\t\tinflateInit(&stream);\n+\t\tSHA1_Init(&c);\n+\t\tif (prev_posn>0) {\n+\t\t\tprev_posn = 0;\n+\t\t\tlseek(local, SEEK_SET, 0);\n+\t\t}\n+\t}\n+\n+\t/* If we have successfully processed data from a previous fetch\n+\t attempt, only fetch the data we don't already have. */\n+\tif (prev_posn>0) {\n+\t\tsprintf(range, \"Range: bytes=%ld-\", prev_posn);\n+\t\trange_header = curl_slist_append(range_header, range);\n+\t\tcurl_easy_setopt(curl, CURLOPT_HTTPHEADER, range_header);\n+\t}\n+\n+\t/* Clear out the Range: header after performing the request, so\n+\t other curl requests don't inherit inappropriate header data */\n+\tcurl_result = curl_easy_perform(curl);\n+\tcurl_easy_setopt(curl, CURLOPT_HTTPHEADER, no_range_header);\n+\tif (curl_result != 0) {\n \t\treturn -1;\n \t}\n \n@@ -517,6 +577,7 @@ int main(int argc, char **argv)\n \n \tcurl = curl_easy_init();\n \tno_pragma_header = curl_slist_append(no_pragma_header, \"Pragma:\");\n+\tno_range_header = curl_slist_append(no_range_header, \"Range:\");\n \n /* Set SSL parameters if they were provided */\n \tif (ssl_cert != NULL) {\n@@ -549,6 +610,7 @@ int main(int argc, char **argv)\n \t\treturn 1;\n \n \tcurl_slist_free_all(no_pragma_header);\n+\tcurl_slist_free_all(no_range_header);\n \tcurl_global_cleanup();\n \treturn 0;\n }\n"},{"id":"9328","messageId":"7vd5mvedcs.fsf@assigned-by-dhcp.cox.net","threadId":"1952","inReplyTo":"20050926175204.GC9410@reactrix.com","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-26T21:19:31Z","receivedAt":"2005-09-26T21:19:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nick Hengeveld writes:\n\n> +\tunlink(prevfile);\n> +\trename(tmpfile, prevfile);\n> +\tunlink(tmpfile);\n\nIf rename() succeeds then tmpfile is no more. If rename() fails\nbecause there were no tmpfile to begin with, it is not an error\n(i.e. you are not recovering) and that case there would not be\ntmpfile either. Otherwise, if tmpfile still remains to unlink()\nbecause rename() failed for any other reason, wouldn't you\nrather report it as an error and abort?\n\nI wonder what happens if by mistake or intentionally we run two\nhttp-fetch instances simultaneously. IIRC, the current code is\nsafe -- the resulting object database will have the object file\nfetched by one of the instance, and the updating of ref is done\nvia write_ref_sha1(), so it also is safe. But your change may\nintroduce an interesting case where one creates a tmpfile, the\nother one moves it to prevfile and starts using its partial\ncontents, and possibly gets confused -- it will probabaly fail\nat the end detecting inconsistent object so it is probably not a\nbig loss.\n\nPersonally, I do not think people would mind much if we\nintroduce the BKL at the beginning of git-fetch.sh to prevent\nmultiple fetches stomping on each other, if somebody cared\nenough (hint, hint).\n\n> -\tsnprintf(tmpfile, sizeof(tmpfile), \"%s/obj_XXXXXX\",\n> -\t\t get_object_directory());\n> +\tlocal = open(tmpfile, O_WRONLY | O_CREAT | O_EXCL, 0666);\n> -\tlocal = mkstemp(tmpfile);\n\nI introduced this mkstemp() part recently to mimic the local\nobject creation where newly created object files are built in\nthe same directory, hoping that they would be allocated close to\neach other when more than one are created in sequence. I do not\nknow it matters much in practice -- has anybody measured, and\ndoes anybody care?\n\n> +\t/* Reset inflate/SHA1 if there was an error reading the previous temp\n> +\t file; also rewind to the beginning of the local file. */\n\nMaybe not just rewind but truncate as well? It probably does\nnot matter in practice much, but previous representation your\nfetch was interrupted in the middle could have been much larger\nthan the representation you are slurping right now.\n\nThere was a discussion about an object file of the same SHA1 and\nthe same contents can have different compressed representations\n(we hash then compress so the resulting filesize depends on the\ncompression level without affecting the contents of the object).\nIn a \"doctor, it hurts when I do this -- don't do it, then\" kind\nof corner case, a DNS rotated pair of webservers could be\nserving the same object in different representations and you may\nget interrupted while fetching from one, and restart the\ntransfer from the other. The SHA1 check at the end hopefully\nwould catch this kind of situation, and that round of http-fetch\nwould fail -- the user needs to re-run the fetch so it is not a\nbig loss, but it is something to keep in mind.\n"},{"id":"9339","messageId":"Pine.LNX.4.63.0509261817080.23242@iabervon.org","threadId":"1952","inReplyTo":"20050926175204.GC9410@reactrix.com","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2005-09-26T22:23:45Z","receivedAt":"2005-09-26T22:23:45Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Mon, 26 Sep 2005, Nick Hengeveld wrote:\n\n> Support for partial HTTP transfers - if a previous temp file is detected,\n> read it in and start the HTTP transfer from where the previous left off.\n\nYou probably want the corresponding code for where it's downloading pack\nand index files, which tend to be a lot bigger than individual objects. \n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"9348","messageId":"20050927000931.GA15615@reactrix.com","threadId":"1952","inReplyTo":"7vd5mvedcs.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2005-09-27T00:09:31Z","receivedAt":"2005-09-27T00:09:31Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Mon, Sep 26, 2005 at 02:19:31PM -0700, Junio C Hamano wrote:\n\n> If rename() succeeds then tmpfile is no more. If rename() fails\n> because there were no tmpfile to begin with, it is not an error\n> (i.e. you are not recovering) and that case there would not be\n> tmpfile either. Otherwise, if tmpfile still remains to unlink()\n> because rename() failed for any other reason, wouldn't you\n> rather report it as an error and abort?\n\nI didn't see that as a fatal condition since the transfer would still\ntake place, albeit a full transfer rather than a partial one. It does\nseem worth at least reporting it as a warning though. If a previous\ntmpfile exists and both the rename and unlink fail, the subsequent\ntmpfile open will also fail - unlikely but probably also worth\ndetecting and reporting.\n\n> I wonder what happens if by mistake or intentionally we run two\n> http-fetch instances simultaneously. IIRC, the current code is\n> safe -- the resulting object database will have the object file\n> fetched by one of the instance, and the updating of ref is done\n> via write_ref_sha1(), so it also is safe. But your change may\n> introduce an interesting case where one creates a tmpfile, the\n> other one moves it to prevfile and starts using its partial\n> contents, and possibly gets confused -- it will probabaly fail\n> at the end detecting inconsistent object so it is probably not a\n> big loss.\n\nHow about using mkstemp on the prev file to keep multiple instances\nfrom stepping on each other? Since O_CREAT | O_EXCL is used to\nopen the tmpfile, only one instance will be able to succeed and\ncontinue.\n\n> > +\t/* Reset inflate/SHA1 if there was an error reading the previous temp\n> > +\t file; also rewind to the beginning of the local file. */\n> \n> Maybe not just rewind but truncate as well? It probably does\n> not matter in practice much, but previous representation your\n> fetch was interrupted in the middle could have been much larger\n> than the representation you are slurping right now.\n\nGood point, I'll update the patch.\n\n> There was a discussion about an object file of the same SHA1 and\n> the same contents can have different compressed representations\n> (we hash then compress so the resulting filesize depends on the\n> compression level without affecting the contents of the object).\n> In a \"doctor, it hurts when I do this -- don't do it, then\" kind\n> of corner case, a DNS rotated pair of webservers could be\n> serving the same object in different representations and you may\n> get interrupted while fetching from one, and restart the\n> transfer from the other. The SHA1 check at the end hopefully\n> would catch this kind of situation, and that round of http-fetch\n> would fail -- the user needs to re-run the fetch so it is not a\n> big loss, but it is something to keep in mind.\n\nThat's an annoying case, all right... Would it be worth including a \nfull retry if a partial failed the SHA1 check?\n\n-- \nFor a successful technology, reality must take precedence over public\nrelations, for nature cannot be fooled.\n"},{"id":"9363","messageId":"7vfyrr3vwp.fsf@assigned-by-dhcp.cox.net","threadId":"1952","inReplyTo":"20050927000931.GA15615@reactrix.com","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-27T05:46:30Z","receivedAt":"2005-09-27T05:46:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nick Hengeveld writes:\n\n> How about using mkstemp on the prev file to keep multiple instances\n> from stepping on each other? Since O_CREAT | O_EXCL is used to\n> open the tmpfile, only one instance will be able to succeed and\n> continue.\n\nI think (O_CREAT|O_EXCL) in your code can be easily defeated by\nthis sequence:\n\n one \ttwo\n\n unlink prevfile\n rename tmpfile, prevfile\n unlink tmpfile\n local = open tmpfile\n unlink prevfile\n rename tmpfile, prevfile\n unlink tmpfile\n local = open tmpfile\n prevlocal = open prevfile\n write local\n copyfile prevlocal, local\n write local\n\t\t\t\t???\n\nNot that I think the multiple instances should be prevented at\nthis low level --- if they stomp on each other at this level, it\nis very likely the they are doing duplicated work on the network\nside as well, and should be prevented from doing so at much\nhigher level than this, I think. That's why I said I do not\nmind BKL upfront in git-fetch.sh.\n\n> That's an annoying case, all right... Would it be worth including a \n> full retry if a partial failed the SHA1 check?\n\nProbably not. Just failing loudly and asking the upper layer to\nretry would be fine, I think.\n"},{"id":"9384","messageId":"43392071.7080202@b-i-t.de","threadId":"1952","inReplyTo":"20050926175204.GC9410@reactrix.com","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"sf","fromEmail":"sf@b-i-t.de","sentAt":"2005-09-27T10:35:29Z","receivedAt":"2005-09-27T10:35:29Z","isPatch":true,"sender":{"key":"sf@b-i-t.de","avatar":null},"body":"Nick Hengeveld wrote:\n> Support for partial HTTP transfers - if a previous temp file is detected,\n> read it in and start the HTTP transfer from where the previous left off.\n\nPlease read my recent posting:\n\n\thttp://permalink.gmane.org/gmane.comp.version-control.git/8991.\n\nRegards\n\n\tStephan\n"},{"id":"9393","messageId":"20050927153636.GA1377@reactrix.com","threadId":"1952","inReplyTo":"7vfyrr3vwp.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2005-09-27T15:36:36Z","receivedAt":"2005-09-27T15:36:36Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Mon, Sep 26, 2005 at 10:46:30PM -0700, Junio C Hamano wrote:\n\n> I think (O_CREAT|O_EXCL) in your code can be easily defeated by\n> this sequence:\n> \n> one \ttwo\n> \n> unlink prevfile\n> rename tmpfile, prevfile\n> unlink tmpfile\n> local = open tmpfile\n> unlink prevfile\n> rename tmpfile, prevfile\n> unlink tmpfile\n> local = open tmpfile\n> prevlocal = open prevfile\n> write local\n> copyfile prevlocal, local\n> write local\n> \t\t\t\t???\n\nThe way I understand this sequence, one and two will not be writing to\nthe same file. When two unlinks tmpfile, one will still be able to \nwrite to its local, but the body of the file that one is writing will\nbe removed when it closes local.\n\n> Not that I think the multiple instances should be prevented at\n> this low level --- if they stomp on each other at this level, it\n> is very likely the they are doing duplicated work on the network\n> side as well, and should be prevented from doing so at much\n> higher level than this, I think. That's why I said I do not\n> mind BKL upfront in git-fetch.sh.\n\nTrue, is that something I should include with the partial patch?\n\n-- \nFor a successful technology, reality must take precedence over public\nrelations, for nature cannot be fooled.\n"},{"id":"9400","messageId":"7v1x3a309u.fsf@assigned-by-dhcp.cox.net","threadId":"1952","inReplyTo":"20050927153636.GA1377@reactrix.com","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-27T17:09:49Z","receivedAt":"2005-09-27T17:09:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nick Hengeveld writes:\n\n> The way I understand this sequence, one and two will not be writing to\n> the same file. When two unlinks tmpfile, one will still be able to \n> write to its local, but the body of the file that one is writing will\n> be removed when it closes local.\n\nTrue. What I meant by \"interesting\" is that two is reading from\nwhat one is writing.\n\n>> Not that I think the multiple instances should be prevented at\n>> this low level --- if they stomp on each other at this level, it\n>> is very likely the they are doing duplicated work on the network\n>> side as well, and should be prevented from doing so at much\n>> higher level than this, I think. That's why I said I do not\n>> mind BKL upfront in git-fetch.sh.\n>\n> True, is that something I should include with the partial patch?\n\nNo.\n"},{"id":"9401","messageId":"20050927171912.GC1377@reactrix.com","threadId":"1952","inReplyTo":"7v1x3a309u.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"Nick Hengeveld","fromEmail":"nickh@reactrix.com","sentAt":"2005-09-27T17:19:12Z","receivedAt":"2005-09-27T17:19:12Z","isPatch":true,"sender":{"key":"nickh@reactrix.com","avatar":null},"body":"On Tue, Sep 27, 2005 at 10:09:49AM -0700, Junio C Hamano wrote:\n\n> True. What I meant by \"interesting\" is that two is reading from\n> what one is writing.\n\nExcellent point. Given the potential for problems related to this\nissue or to compressed representation, would it make sense to enable\npartial transfers via a command-line option and leave the feature\ndisabled by default?\n\n-- \nFor a successful technology, reality must take precedence over public\nrelations, for nature cannot be fooled.\n"},{"id":"9412","messageId":"7vfyrqywrv.fsf@assigned-by-dhcp.cox.net","threadId":"1952","inReplyTo":"20050927171912.GC1377@reactrix.com","subject":"Re: [PATCH 2/3] Support for partial HTTP transfers","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-09-27T22:24:20Z","receivedAt":"2005-09-27T22:24:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nick Hengeveld writes:\n\n> On Tue, Sep 27, 2005 at 10:09:49AM -0700, Junio C Hamano wrote:\n>\n>> True. What I meant by \"interesting\" is that two is reading from\n>> what one is writing.\n>\n> Excellent point. Given the potential for problems related to this\n> issue or to compressed representation, would it make sense to enable\n> partial transfers via a command-line option and leave the feature\n> disabled by default?\n\nHere is what I think. Multiple transfers at the same time, if\nit becomes problem in pratcice, should be prevented at the upper\nlayer anyway, so this \"interesting\" problem is a moot point. So\nI'd say let's not worry about that.\n\nDoing partial transfer support for packs and indices would be\nmore useful addition to your code than dealing with the above --\npacks are much bigger than individual objects and would get much\nbigger benefit from restartable download.\n\nDifferent compressed representation is just a theoretical\ncuriosity and we do not know if it is a problem in practice. As\nlong as you can detect the situation and die, it would be OK, I\nthink. \n\n \n"}]}