Fix HTTP request result processing after slot reuse

PATCH In review Nick Hengeveld 2 participants 3 messages Jun 15, 2016

Share & tools

Nick Hengeveld

author

2016-06-15 22:42 UTC #0 lore raw

PATCH 1/1 Fix HTTP request result processing after slot reuse

Add a way to store the results of an HTTP request when a slot finishes so the results can be processed after the slot has been reused.

Signed-off-by Nick Hengeveld <[email protected]>

---

 http-fetch.c |   20 ++++++++++++++------
 http.c       |    9 ++++++++-
 http.h       |    7 +++++++
 3 files changed, 29 insertions(+), 7 deletions(-)

ecda781d5eec60edc3683a4e7408c99eb93b463c

http-fetch.c +14 -6

index 61b2188..92326f9 100644
@@ -375,6 +375,7 @@ static int fetch_index(struct alt_base *
 
 	FILE *indexfile;
 	struct active_request_slot *slot;
+	static struct slot_results results;
 
 	if (has_pack_index(sha1))
 		return 0;

@@ -393,6 +394,7 @@ static int fetch_index(struct alt_base *
 			     filename);
 
 	slot = get_active_slot();
+	slot->results = &results;
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, indexfile);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);

@@ -414,7 +416,7 @@ static int fetch_index(struct alt_base *
 
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
-		if (slot->curl_result != CURLE_OK) {
+		if (results.curl_result != CURLE_OK) {
 			fclose(indexfile);
 			return error("Unable to get pack index %s\n%s", url,
 				     curl_errorstr);

@@ -616,6 +618,7 @@ static int fetch_indices(struct alt_base
 	int i = 0;
 
 	struct active_request_slot *slot;
+	static struct slot_results results;
 
 	if (repo->got_indices)
 		return 0;

@@ -632,15 +635,16 @@ static int fetch_indices(struct alt_base
 	sprintf(url, "%s/objects/info/packs", repo->base);
 
 	slot = get_active_slot();
+	slot->results = &results;
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
-		if (slot->curl_result != CURLE_OK) {
-			if (slot->http_code == 404 ||
-			    slot->curl_result == CURLE_FILE_COULDNT_READ_FILE) {
+		if (results.curl_result != CURLE_OK) {
+			if (results.http_code == 404 ||
+			    results.curl_result == CURLE_FILE_COULDNT_READ_FILE) {
 				repo->got_indices = 1;
 				free(buffer.buffer);
 				return 0;

@@ -695,6 +699,7 @@ static int fetch_pack(struct alt_base *r
 	struct curl_slist *range_header = NULL;
 
 	struct active_request_slot *slot;
+	static struct slot_results results;
 
 	if (fetch_indices(repo))
 		return -1;

@@ -721,6 +726,7 @@ static int fetch_pack(struct alt_base *r
 			     filename);
 
 	slot = get_active_slot();
+	slot->results = &results;
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, packfile);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);

@@ -742,7 +748,7 @@ static int fetch_pack(struct alt_base *r
 
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
-		if (slot->curl_result != CURLE_OK) {
+		if (results.curl_result != CURLE_OK) {
 			fclose(packfile);
 			return error("Unable to get pack file %s\n%s", url,
 				     curl_errorstr);

@@ -894,6 +900,7 @@ int fetch_ref(char *ref, unsigned char *
         struct buffer buffer;
 	char *base = alt->base;
 	struct active_request_slot *slot;
+	static struct slot_results results;
         buffer.size = 41;
         buffer.posn = 0;
         buffer.buffer = hex;

@@ -901,13 +908,14 @@ int fetch_ref(char *ref, unsigned char *
         
 	url = quote_ref_url(base, ref);
 	slot = get_active_slot();
+	slot->results = &results;
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
-		if (slot->curl_result != CURLE_OK)
+		if (results.curl_result != CURLE_OK)
 			return error("Couldn't get %s for %s\n%s",
 				     url, ref, curl_errorstr);
 	} else {

http.c +8 -1

index 75e6717..eefb0f0 100644
@@ -335,6 +335,7 @@ struct active_request_slot *get_active_s
 	active_requests++;
 	slot->in_use = 1;
 	slot->local = NULL;
+	slot->results = NULL;
 	slot->callback_data = NULL;
 	slot->callback_func = NULL;
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, pragma_header);

@@ -421,7 +422,13 @@ static void finish_active_slot(struct ac
         active_requests--;
         slot->in_use = 0;
         curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code);
- 
+
+	/* Store slot results so they can be read after the slot is reused */
+	if (slot->results != NULL) {
+		slot->results->curl_result = slot->curl_result;
+		slot->results->http_code = slot->http_code;
+	}
+
         /* Run callback if appropriate */
         if (slot->callback_func != NULL) {
                 slot->callback_func(slot->callback_data);

http.h +7 -0

index ed4ea33..d6dc9d8 100644
@@ -22,6 +22,12 @@
 #define NO_CURL_EASY_DUPHANDLE
 #endif
 
+struct slot_results
+{
+	CURLcode curl_result;
+	long http_code;
+};
+
 struct active_request_slot
 {
 	CURL *curl;

@@ -29,6 +35,7 @@ struct active_request_slot
 	int in_use;
 	CURLcode curl_result;
 	long http_code;
+	struct slot_results *results;
 	void *callback_data;
 	void (*callback_func)(void *data);
 	struct active_request_slot *next;

Signature · 1 line

1.1.6.g1a5c3-dirty

Junio C Hamano

2016-06-15 22:42 UTC #1 lore raw

Quoted 4 lines · Nick Hengeveld

Add a way to store the results of an HTTP request when a slot finishes so the results can be processed after the slot has been reused.
Signed-off-by Nick Hengeveld <[email protected]>

I do not claim I understand this part of the code well, but it feels awkward to see that a problem caused by a structure being reused can be solved by having a pointer from that structure that points at a static location.

These static variables are probably correct, provided if fetch_index, fetch_indices and friends do not recurse into themselves, but it just gives me this funny feeling...

Care to enlighten me, please?

Nick Hengeveld

author

2016-06-15 22:42 UTC #2 lore raw

Quoted 3 lines · Junio C Hamano

These static variables are probably correct, provided if fetch_index, fetch_indices and friends do not recurse into themselves, but it just gives me this funny feeling...

It's true that in the current implementation we either don't recurse into these functions or we explicitly handle cases where we do such as fetch_alternates. However, I've got no argument as to why the structures should be static and can imagine it just causing problems down the road if we were to eg. start downloading multiple packs concurrently.

I'll follow up with a patch.

--
For a successful technology, reality must take precedence over public relations, for nature cannot be fooled.