From: Nicholas Nethercote <n.nethercote@gmail.com>
Date: Fri, 1 Sep 2023 01:41:22 +0000 (+1000)
Subject: hyper: remove `hyptransfer->endtask`
X-Git-Tag: curl-8_3_0~49
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=50aa325742f6fbed1fb82b535761e85339cf8dbb;p=thirdparty%2Fcurl.git

hyper: remove `hyptransfer->endtask`

`Curl_hyper_stream` needs to distinguish between two kinds of
`HYPER_TASK_EMPTY` tasks: (a) the `foreach` tasks it creates itself, and
(b) background tasks that hyper produces. It does this by recording the
address of any `foreach` task in `hyptransfer->endtask` before pushing
it into the executor, and then comparing that against the address of
tasks later polled out of the executor.

This works right now, but there is no guarantee from hyper that the
addresses are stable. `hyper_executor_push` says "The executor takes
ownership of the task, which should not be accessed again unless
returned back to the user with `hyper_executor_poll`". That wording is a
bit ambiguous but with my Rust programmer's hat on I read it as meaning
the task returned with `hyper_executor_poll` may be conceptually the
same as a task that was pushed, but that there are no other guarantees
and comparing addresses is a bad idea.

This commit instead uses `hyper_task_set_userdata` to mark the `foreach`
task with a `USERDATA_RESP_BODY` value which can then be checked for,
removing the need for `hyptransfer->endtask`. This makes the code look
more like that hyper C API examples, which use userdata for every task
and never look at task addresses.

Closes #11779
---

diff --git a/lib/c-hyper.c b/lib/c-hyper.c
index 05080fa9ac..61ca29a3ac 100644
--- a/lib/c-hyper.c
+++ b/lib/c-hyper.c
@@ -61,6 +61,11 @@
 #include "curl_memory.h"
 #include "memdebug.h"
 
+typedef enum {
+    USERDATA_NOT_SET = 0, /* for tasks with no userdata set; must be zero */
+    USERDATA_RESP_BODY
+} userdata_t;
+
 size_t Curl_hyper_recv(void *userp, hyper_context *ctx,
                        uint8_t *buf, size_t buflen)
 {
@@ -350,7 +355,6 @@ CURLcode Curl_hyper_stream(struct Curl_easy *data,
   struct hyptransfer *h = &data->hyp;
   hyper_task *task;
   hyper_task *foreach;
-  hyper_error *hypererr = NULL;
   const uint8_t *reasonp;
   size_t reason_len;
   CURLcode result = CURLE_OK;
@@ -393,19 +397,9 @@ CURLcode Curl_hyper_stream(struct Curl_easy *data,
       break;
     }
     t = hyper_task_type(task);
-    switch(t) {
-    case HYPER_TASK_ERROR:
-      hypererr = hyper_task_value(task);
-      break;
-    case HYPER_TASK_RESPONSE:
-      resp = hyper_task_value(task);
-      break;
-    default:
-      break;
-    }
-    hyper_task_free(task);
-
     if(t == HYPER_TASK_ERROR) {
+      hyper_error *hypererr = hyper_task_value(task);
+      hyper_task_free(task);
       if(data->state.hresult) {
         /* override Hyper's view, might not even be an error */
         result = data->state.hresult;
@@ -441,23 +435,30 @@ CURLcode Curl_hyper_stream(struct Curl_easy *data,
       hyper_error_free(hypererr);
       break;
     }
-    else if(h->endtask == task) {
-      /* end of transfer, forget the task handled, we might get a
-       * new one with the same address in the future. */
-      *done = TRUE;
-      h->endtask = NULL;
-      infof(data, "hyperstream is done");
-      if(!k->bodywrites) {
-        /* hyper doesn't always call the body write callback */
-        bool stilldone;
-        result = Curl_http_firstwrite(data, data->conn, &stilldone);
+    else if(t == HYPER_TASK_EMPTY) {
+      void *userdata = hyper_task_userdata(task);
+      hyper_task_free(task);
+      if((userdata_t)userdata == USERDATA_RESP_BODY) {
+        /* end of transfer */
+        *done = TRUE;
+        infof(data, "hyperstream is done");
+        if(!k->bodywrites) {
+          /* hyper doesn't always call the body write callback */
+          bool stilldone;
+          result = Curl_http_firstwrite(data, data->conn, &stilldone);
+        }
+        break;
+      }
+      else {
+        /* A background task for hyper; ignore */
+        continue;
       }
-      break;
-    }
-    else if(t != HYPER_TASK_RESPONSE) {
-      continue;
     }
-    /* HYPER_TASK_RESPONSE */
+
+    DEBUGASSERT(HYPER_TASK_RESPONSE);
+
+    resp = hyper_task_value(task);
+    hyper_task_free(task);
 
     *didwhat = KEEP_RECV;
     if(!resp) {
@@ -537,13 +538,12 @@ CURLcode Curl_hyper_stream(struct Curl_easy *data,
       result = CURLE_OUT_OF_MEMORY;
       break;
     }
-    DEBUGASSERT(hyper_task_type(foreach) == HYPER_TASK_EMPTY);
+    hyper_task_set_userdata(foreach, (void *)USERDATA_RESP_BODY);
     if(HYPERE_OK != hyper_executor_push(h->exec, foreach)) {
       failf(data, "Couldn't hyper_executor_push the body-foreach");
       result = CURLE_OUT_OF_MEMORY;
       break;
     }
-    h->endtask = foreach;
 
     hyper_response_free(resp);
     resp = NULL;
diff --git a/lib/c-hyper.h b/lib/c-hyper.h
index 4218cda75f..0c7de90b70 100644
--- a/lib/c-hyper.h
+++ b/lib/c-hyper.h
@@ -34,7 +34,6 @@ struct hyptransfer {
   hyper_waker *write_waker;
   hyper_waker *read_waker;
   const hyper_executor *exec;
-  hyper_task *endtask;
   hyper_waker *exp100_waker;
   hyper_waker *send_body_waker;
 };