fix two bugs

awslabs · Dec 21, 2023 · fea7cdc · fea7cdc
1 parent c91f4c3
commit fea7cdc
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 21 deletions.
diff --git a/source/s3_meta_request.c b/source/s3_meta_request.c
@@ -1385,14 +1385,16 @@ static void s_s3_meta_request_stream_complete(struct aws_http_stream *stream, in
     if (meta_request->checksum_config.validate_response_checksum) {
         s_get_response_part_finish_checksum_helper(connection, error_code);
     }
-    if (error_code != AWS_ERROR_S3_CANCELED && error_code != AWS_ERROR_S3_PAUSED) {
-        /* BEGIN CRITICAL SECTION */
+    /* BEGIN CRITICAL SECTION */
+    {
         aws_s3_meta_request_lock_synced_data(meta_request);
-        AWS_ASSERT(request->synced_data.http_stream != NULL);
-        aws_linked_list_remove(&request->ongoing_http_requests_list_node);
+        if (request->synced_data.http_stream) {
+            /* The request has been cancelled, and the node has been removed. */
+            aws_linked_list_remove(&request->ongoing_http_requests_list_node);
+        }
         aws_s3_meta_request_unlock_synced_data(meta_request);
-        /* END CRITICAL SECTION */
     }
+    /* END CRITICAL SECTION */
     s_s3_meta_request_send_request_finish(connection, stream, error_code);
 }
 
@@ -1683,6 +1685,23 @@ void aws_s3_meta_request_cancel_ongoing_http_requests_synced(struct aws_s3_meta_
     }
 }
 
+static void s_s3_request_finish_up_metrics(struct aws_s3_request *request, struct aws_s3_meta_request *meta_request) {
+
+    if (request->send_data.metrics != NULL) {
+        /* Request is done streaming the body, complete the metrics for the request now. */
+        struct aws_s3_request_metrics *metrics = request->send_data.metrics;
+        aws_high_res_clock_get_ticks((uint64_t *)&metrics->time_metrics.end_timestamp_ns);
+        metrics->time_metrics.total_duration_ns =
+            metrics->time_metrics.end_timestamp_ns - metrics->time_metrics.start_timestamp_ns;
+
+        if (meta_request->telemetry_callback != NULL) {
+            /* We already in the meta request event thread, invoke the telemetry callback directly */
+            meta_request->telemetry_callback(meta_request, metrics, meta_request->user_data);
+        }
+        request->send_data.metrics = aws_s3_request_metrics_release(metrics);
+    }
+}
+
 /* Deliver events in event_delivery_array.
  * This task runs on the meta-request's io_event_loop thread. */
 static void s_s3_meta_request_event_delivery_task(struct aws_task *task, void *arg, enum aws_task_status task_status) {
@@ -1750,21 +1769,7 @@ static void s_s3_meta_request_event_delivery_task(struct aws_task *task, void *a
                 aws_atomic_fetch_sub(&client->stats.num_requests_streaming_response, 1);
 
                 ++num_parts_delivered;
-
-                if (request->send_data.metrics != NULL) {
-                    /* Request is done streaming the body, complete the metrics for the request now. */
-                    struct aws_s3_request_metrics *metrics = request->send_data.metrics;
-                    metrics->crt_info_metrics.error_code = error_code;
-                    aws_high_res_clock_get_ticks((uint64_t *)&metrics->time_metrics.end_timestamp_ns);
-                    metrics->time_metrics.total_duration_ns =
-                        metrics->time_metrics.end_timestamp_ns - metrics->time_metrics.start_timestamp_ns;
-
-                    if (meta_request->telemetry_callback != NULL) {
-                        /* We already in the meta request event thread, invoke the telemetry callback directly */
-                        meta_request->telemetry_callback(meta_request, metrics, meta_request->user_data);
-                    }
-                    request->send_data.metrics = aws_s3_request_metrics_release(metrics);
-                }
+                s_s3_request_finish_up_metrics(request, meta_request);
 
                 aws_s3_request_release(request);
             } break;
@@ -1935,6 +1940,8 @@ void aws_s3_meta_request_finish_default(struct aws_s3_meta_request *meta_request
         struct aws_linked_list_node *request_node = aws_linked_list_pop_front(&release_request_list);
         struct aws_s3_request *release_request = AWS_CONTAINER_OF(request_node, struct aws_s3_request, node);
         AWS_FATAL_ASSERT(release_request != NULL);
+        /* The pending body streaming requests cleaned up here. Finish the metrics for those requests. */
+        s_s3_request_finish_up_metrics(release_request, meta_request);
         aws_s3_request_release(release_request);
     }
 

diff --git a/source/s3_util.c b/source/s3_util.c
@@ -636,6 +636,7 @@ int aws_s3_crt_error_code_from_server_error_code_string(struct aws_byte_cursor e
 void aws_s3_request_finish_up_metrics_synced(struct aws_s3_request *request, struct aws_s3_meta_request *meta_request) {
     AWS_PRECONDITION(meta_request);
     AWS_PRECONDITION(request);
+    ASSERT_SYNCED_DATA_LOCK_HELD(meta_request);
 
     if (request->send_data.metrics != NULL) {
         /* Request is done, complete the metrics for the request now. */

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -59,6 +59,7 @@ add_net_test_case(test_s3_cancel_mpd_head_object_sent)
 add_net_test_case(test_s3_cancel_mpd_head_object_completed)
 add_net_test_case(test_s3_cancel_mpd_get_without_range_sent)
 add_net_test_case(test_s3_cancel_mpd_get_without_range_completed)
+add_net_test_case(test_s3_cancel_mpd_pending_streaming)
 add_net_test_case(test_s3_cancel_prepare)
 
 add_net_test_case(test_s3_get_object_tls_disabled)

diff --git a/tests/s3_cancel_tests.c b/tests/s3_cancel_tests.c
@@ -30,6 +30,7 @@ enum s3_update_cancel_type {
     S3_UPDATE_CANCEL_TYPE_MPD_ONE_PART_SENT,
     S3_UPDATE_CANCEL_TYPE_MPD_ONE_PART_COMPLETED,
     S3_UPDATE_CANCEL_TYPE_MPD_TWO_PARTS_COMPLETED,
+    S3_UPDATE_CANCEL_TYPE_MPD_PENDING_STREAMING,
 };
 
 struct s3_cancel_test_user_data {
@@ -122,6 +123,11 @@ static bool s_s3_meta_request_update_cancel_test(
             /* Prevent other parts from being queued while we wait for these two to complete. */
             block_update = !call_cancel_or_pause && auto_ranged_get->synced_data.num_parts_requested == 2;
             break;
+
+        case S3_UPDATE_CANCEL_TYPE_MPD_PENDING_STREAMING:
+            call_cancel_or_pause =
+                aws_priority_queue_size(&meta_request->synced_data.pending_body_streaming_requests) > 0;
+            break;
     }
 
     aws_s3_meta_request_unlock_synced_data(meta_request);
@@ -299,7 +305,7 @@ static int s3_cancel_test_helper_ex(
             .validate_type = AWS_S3_TESTER_VALIDATE_TYPE_EXPECT_FAILURE,
             .get_options =
                 {
-                    .object_path = g_pre_existing_object_1MB,
+                    .object_path = g_pre_existing_object_10MB,
                 },
         };
 
@@ -618,6 +624,15 @@ static int s_test_s3_cancel_mpd_get_without_range_completed(struct aws_allocator
     return 0;
 }
 
+AWS_TEST_CASE(test_s3_cancel_mpd_pending_streaming, s_test_s3_cancel_mpd_pending_streaming)
+static int s_test_s3_cancel_mpd_pending_streaming(struct aws_allocator *allocator, void *ctx) {
+    (void)ctx;
+
+    ASSERT_SUCCESS(s3_cancel_test_helper(allocator, S3_UPDATE_CANCEL_TYPE_MPD_PENDING_STREAMING));
+
+    return 0;
+}
+
 struct test_s3_cancel_prepare_user_data {
     uint32_t request_prepare_counters[AWS_S3_AUTO_RANGED_PUT_REQUEST_TAG_MAX];
 };