Merge pull request #898 from floooh/issue882-sfetch_early_cancel

sokol_fetch.h: fix response state when cancelling a request before dispatch (fixes #882)
floooh · Sep 19, 2023 · b803c9a · b803c9a
2 parents 9d4a9e9 + f3858d2
commit b803c9a
Show file tree

Hide file tree

Showing 3 changed files with 87 additions and 19 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,12 @@
 ## Updates
 
+#### 19-Sep-2023
+
+- sokol_fetch.h: fixed a minor issue where a request that was cancelled before it was dispatched
+  had an incomplete response state set in the response callback (the `finished`, `failed` and
+  `error_code` fields were not set). This fixes issue https://github.com/floooh/sokol/issues/882
+  via PR https://github.com/floooh/sokol/pull/898
+
 #### 18-Sep-2023
 
 - PR https://github.com/floooh/sokol/pull/893 has been merged, this fixes a minor issue

diff --git a/sokol_fetch.h b/sokol_fetch.h
@@ -2457,6 +2457,12 @@ _SOKOL_PRIVATE void _sfetch_invoke_response_callback(_sfetch_item_t* item) {
     item->callback(&response);
 }
 
+_SOKOL_PRIVATE void _sfetch_cancel_item(_sfetch_item_t* item) {
+    item->state = _SFETCH_STATE_FAILED;
+    item->user.finished = true;
+    item->user.error_code = SFETCH_ERROR_CANCELLED;
+}
+
 /* per-frame channel stuff: move requests in and out of the IO threads, call response callbacks */
 _SOKOL_PRIVATE void _sfetch_channel_dowork(_sfetch_channel_t* chn, _sfetch_pool_t* pool) {
 
@@ -2469,9 +2475,16 @@ _SOKOL_PRIVATE void _sfetch_channel_dowork(_sfetch_channel_t* chn, _sfetch_pool_
         _sfetch_item_t* item = _sfetch_pool_item_lookup(pool, slot_id);
         SOKOL_ASSERT(item);
         SOKOL_ASSERT(item->state == _SFETCH_STATE_ALLOCATED);
+        // if the item was cancelled early, kick it out immediately
+        if (item->user.cancel) {
+            _sfetch_cancel_item(item);
+            _sfetch_invoke_response_callback(item);
+            _sfetch_pool_item_free(pool, slot_id);
+            continue;
+        }
         item->state = _SFETCH_STATE_DISPATCHED;
         item->lane = _sfetch_ring_dequeue(&chn->free_lanes);
-        /* if no buffer provided yet, invoke response callback to do so */
+        // if no buffer provided yet, invoke response callback to do so
         if (0 == item->buffer.ptr) {
             _sfetch_invoke_response_callback(item);
         }
@@ -2498,8 +2511,7 @@ _SOKOL_PRIVATE void _sfetch_channel_dowork(_sfetch_channel_t* chn, _sfetch_pool_
             item->user.cont = false;
         }
         if (item->user.cancel) {
-            item->state = _SFETCH_STATE_FAILED;
-            item->user.finished = true;
+            _sfetch_cancel_item(item);
         }
         switch (item->state) {
             case _SFETCH_STATE_DISPATCHED:
@@ -2541,7 +2553,7 @@ _SOKOL_PRIVATE void _sfetch_channel_dowork(_sfetch_channel_t* chn, _sfetch_pool_
         item->user.fetched_offset = item->thread.fetched_offset;
         item->user.fetched_size = item->thread.fetched_size;
         if (item->user.cancel) {
-            item->user.error_code = SFETCH_ERROR_CANCELLED;
+            _sfetch_cancel_item(item);
         }
         else {
             item->user.error_code = item->thread.error_code;
@@ -2558,7 +2570,7 @@ _SOKOL_PRIVATE void _sfetch_channel_dowork(_sfetch_channel_t* chn, _sfetch_pool_
         }
         _sfetch_invoke_response_callback(item);
 
-        /* when the request is finish, free the lane for another request,
+        /* when the request is finished, free the lane for another request,
            otherwise feed it back into the incoming queue
         */
         if (item->user.finished) {

diff --git a/tests/functional/sokol_fetch_test.c b/tests/functional/sokol_fetch_test.c
@@ -612,9 +612,9 @@ UTEST(sokol_fetch, load_file_chunked) {
 /* load N big files in small chunks interleaved on the same channel via lanes */
 #define LOAD_FILE_LANES_NUM_LANES (4)
 
-uint8_t load_file_lanes_chunk_buf[LOAD_FILE_LANES_NUM_LANES][8192];
-uint8_t load_file_lanes_content[LOAD_FILE_LANES_NUM_LANES][500000];
-int load_file_lanes_passed[LOAD_FILE_LANES_NUM_LANES];
+static uint8_t load_file_lanes_chunk_buf[LOAD_FILE_LANES_NUM_LANES][8192];
+static uint8_t load_file_lanes_content[LOAD_FILE_LANES_NUM_LANES][500000];
+static int load_file_lanes_passed[LOAD_FILE_LANES_NUM_LANES];
 static void load_file_lanes_callback(const sfetch_response_t* response) {
     assert((response->channel == 0) && (response->lane < LOAD_FILE_LANES_NUM_LANES));
     if (response->fetched) {
@@ -669,9 +669,9 @@ UTEST(sokol_fetch, load_file_lanes) {
 #define LOAD_FILE_THROTTLE_NUM_PASSES (3)
 #define LOAD_FILE_THROTTLE_NUM_REQUESTS (12)    // lanes * passes
 
-uint8_t load_file_throttle_chunk_buf[LOAD_FILE_THROTTLE_NUM_LANES][128000];
-uint8_t load_file_throttle_content[LOAD_FILE_THROTTLE_NUM_PASSES][LOAD_FILE_THROTTLE_NUM_LANES][500000];
-int load_file_throttle_passed[LOAD_FILE_THROTTLE_NUM_LANES];
+static uint8_t load_file_throttle_chunk_buf[LOAD_FILE_THROTTLE_NUM_LANES][128000];
+static uint8_t load_file_throttle_content[LOAD_FILE_THROTTLE_NUM_PASSES][LOAD_FILE_THROTTLE_NUM_LANES][500000];
+static int load_file_throttle_passed[LOAD_FILE_THROTTLE_NUM_LANES];
 
 static void load_file_throttle_callback(const sfetch_response_t* response) {
     assert((response->channel == 0) && (response->lane < LOAD_FILE_LANES_NUM_LANES));
@@ -733,8 +733,8 @@ UTEST(sokol_fetch, load_file_throttle) {
 
 /* test parallel fetches on multiple channels */
 #define LOAD_CHANNEL_NUM_CHANNELS (16)
-uint8_t load_channel_buf[LOAD_CHANNEL_NUM_CHANNELS][500000];
-bool load_channel_passed[LOAD_CHANNEL_NUM_CHANNELS];
+static uint8_t load_channel_buf[LOAD_CHANNEL_NUM_CHANNELS][500000];
+static bool load_channel_passed[LOAD_CHANNEL_NUM_CHANNELS];
 
 void load_channel_callback(const sfetch_response_t* response) {
     assert(response->channel < LOAD_CHANNEL_NUM_CHANNELS);
@@ -781,15 +781,13 @@ UTEST(sokol_fetch, load_channel) {
     sfetch_shutdown();
 }
 
-bool load_file_cancel_passed = false;
-void load_file_cancel_callback(const sfetch_response_t* response) {
+static bool load_file_cancel_passed = false;
+static void load_file_cancel_callback(const sfetch_response_t* response) {
     if (response->dispatched) {
         sfetch_cancel(response->handle);
     }
-    if (response->failed) {
-        if (response->cancelled && response->finished && (response->error_code == SFETCH_ERROR_CANCELLED)) {
-            load_file_cancel_passed = true;
-        }
+    if (response->cancelled && response->finished && response->failed && (response->error_code == SFETCH_ERROR_CANCELLED)) {
+        load_file_cancel_passed = true;
     }
 }
 
@@ -811,3 +809,54 @@ UTEST(sokol_fetch, load_file_cancel) {
     T(load_file_cancel_passed);
     sfetch_shutdown();
 }
+
+static bool load_file_cancel_before_dispatch_passed = false;
+static void load_file_cancel_before_dispatch_callback(const sfetch_response_t* response) {
+    // cancelled, finished, failed and error code must all be set
+    if (response->cancelled && response->finished && response->failed && (response->error_code == SFETCH_ERROR_CANCELLED)) {
+        load_file_cancel_before_dispatch_passed = true;
+    }
+}
+
+UTEST(sokol_fetch, load_file_cancel_before_dispatch) {
+    sfetch_setup(&(sfetch_desc_t){
+        .num_channels = 1,
+    });
+    sfetch_handle_t h = sfetch_send(&(sfetch_request_t){
+        .path = "comsi.s3m",
+        .callback = load_file_cancel_before_dispatch_callback,
+    });
+    sfetch_cancel(h);
+    sfetch_dowork();
+    T(load_file_cancel_before_dispatch_passed);
+    sfetch_shutdown();
+}
+
+static bool load_file_cancel_after_dispatch_passed = false;
+static void load_file_cancel_after_dispatch_callback(const sfetch_response_t* response) {
+    // when cancelled, then finished, failed and error code must all be set
+    if (response->cancelled && response->finished && response->failed && (response->error_code == SFETCH_ERROR_CANCELLED)) {
+        load_file_cancel_after_dispatch_passed = true;
+    }
+}
+
+UTEST(sokol_fetch, load_file_cancel_after_dispatch) {
+    sfetch_setup(&(sfetch_desc_t){
+        .num_channels = 1,
+    });
+    sfetch_handle_t h = sfetch_send(&(sfetch_request_t){
+        .path = "comsi.s3m",
+        .callback = load_file_cancel_after_dispatch_callback,
+        .buffer = SFETCH_RANGE(load_file_buf),
+    });
+    int frame_count = 0;
+    const int max_frames = 10000;
+    while (sfetch_handle_valid(h) && (frame_count++ < max_frames)) {
+        sfetch_dowork();
+        sfetch_cancel(h);
+        sleep_ms(1);
+    }
+    T(frame_count < max_frames);
+    T(load_file_cancel_after_dispatch_passed);
+    sfetch_shutdown();
+}