Address TODOs

include/aws/http/http.h

@@ -54,6 +54,7 @@ enum aws_http_errors {
     AWS_ERROR_HTTP_STREAM_MANAGER_SHUTTING_DOWN,
     AWS_ERROR_HTTP_STREAM_MANAGER_CONNECTION_ACQUIRE_FAILURE,
     AWS_ERROR_HTTP_STREAM_MANAGER_UNEXPECTED_HTTP_VERSION,
+    AWS_ERROR_HTTP_REQUIRED_PSEUDO_HEADER_MISSING,
 
     AWS_ERROR_HTTP_END_RANGE = AWS_ERROR_ENUM_END_RANGE(AWS_C_HTTP_PACKAGE_ID)
 };

source/connection.c

@@ -815,9 +815,7 @@ static void s_client_bootstrap_on_channel_setup(
         struct aws_crt_statistics_handler *http_connection_monitor =
             aws_crt_statistics_handler_new_http_connection_monitor(
                 http_bootstrap->alloc, &http_bootstrap->monitoring_options);
-        if (http_connection_monitor == NULL) {
-            goto error;
-        }
+        AWS_ASSERT(http_connection_monitor);
 
         aws_channel_set_statistics_handler(channel, http_connection_monitor);
     }
@@ -1030,17 +1028,15 @@ int aws_http_client_connect_internal(
 
     struct aws_http2_setting *setting_array = NULL;
     struct aws_hash_table *alpn_string_map = NULL;
-    if (!aws_mem_acquire_many(
-            options.allocator,
-            3,
-            &http_bootstrap,
-            sizeof(struct aws_http_client_bootstrap),
-            &setting_array,
-            options.http2_options->num_initial_settings * sizeof(struct aws_http2_setting),
-            &alpn_string_map,
-            sizeof(struct aws_hash_table))) {
-        goto error;
-    }
+    aws_mem_acquire_many(
+        options.allocator,
+        3,
+        &http_bootstrap,
+        sizeof(struct aws_http_client_bootstrap),
+        &setting_array,
+        options.http2_options->num_initial_settings * sizeof(struct aws_http2_setting),
+        &alpn_string_map,
+        sizeof(struct aws_hash_table));
 
     AWS_ZERO_STRUCT(*http_bootstrap);
 

source/connection_manager.c

@@ -124,17 +124,16 @@ enum aws_http_connection_manager_count_type {
  * READY - connections may be acquired and released.  When the external ref count for the manager
  * drops to zero, the manager moves to:
  *
- * TODO: Seems like connections can still be release while shutting down.
  * SHUTTING_DOWN - connections may no longer be acquired and released (how could they if the external
- * ref count was accurate?) but in case of user ref errors, we simply fail attempts to do so rather
- * than crash or underflow.  While in this state, we wait for a set of tracking counters to all fall to zero:
+ * ref count was accurate?)
+ * While in this state, we wait for a set of tracking counters to all fall to zero:
  *
  *   pending_connect_count - the # of unresolved calls to the http layer's connect logic
  *   open_connection_count - the # of connections for whom the shutdown callback (from http) has not been invoked
  *   vended_connection_count - the # of connections held by external users that haven't been released.  Under correct
  *      usage this should be zero before SHUTTING_DOWN is entered, but we attempt to handle incorrect usage gracefully.
  *
- * While all the counter fall to zero and no outlife transition, connection manager will detroy itself.
+ * While all the counter fall to zero and no outlive transition, connection manager will detroy itself.
  *
  * While shutting down, as pending connects resolve, we immediately release new incoming (from http) connections
  *
@@ -821,9 +820,6 @@ struct aws_http_connection_manager *aws_http_connection_manager_new(
 
     struct aws_http_connection_manager *manager =
         aws_mem_calloc(allocator, 1, sizeof(struct aws_http_connection_manager));
-    if (manager == NULL) {
-        return NULL;
-    }
 
     manager->allocator = allocator;
 
@@ -1232,6 +1228,7 @@ int aws_http_connection_manager_release_connection(
         (void *)connection);
 
     aws_mutex_lock(&manager->lock);
+    AWS_FATAL_ASSERT(manager->state == AWS_HCMST_READY);
 
     /* We're probably hosed in this case, but let's not underflow */
     if (manager->internal_ref[AWS_HCMCT_VENDED_CONNECTION] == 0) {

source/connection_monitor.c

@@ -204,15 +204,13 @@ struct aws_crt_statistics_handler *aws_crt_statistics_handler_new_http_connectio
     struct aws_crt_statistics_handler *handler = NULL;
     struct aws_statistics_handler_http_connection_monitor_impl *impl = NULL;
 
-    if (!aws_mem_acquire_many(
-            allocator,
-            2,
-            &handler,
-            sizeof(struct aws_crt_statistics_handler),
-            &impl,
-            sizeof(struct aws_statistics_handler_http_connection_monitor_impl))) {
-        return NULL;
-    }
+    aws_mem_acquire_many(
+        allocator,
+        2,
+        &handler,
+        sizeof(struct aws_crt_statistics_handler),
+        &impl,
+        sizeof(struct aws_statistics_handler_http_connection_monitor_impl));
 
     AWS_ZERO_STRUCT(*handler);
     AWS_ZERO_STRUCT(*impl);

source/h1_decoder.c

@@ -108,9 +108,6 @@ static int s_cat(struct aws_h1_decoder *decoder, struct aws_byte_cursor to_appen
         } while (new_size < (buffer->len + to_append.len));
 
         uint8_t *new_data = aws_mem_acquire(buffer->allocator, new_size);
-        if (!new_data) {
-            return AWS_OP_ERR;
-        }
 
         if (buffer->buffer != NULL) {
             memcpy(new_data, buffer->buffer, buffer->len);
@@ -726,11 +723,7 @@ static int s_linestate_response(struct aws_h1_decoder *decoder, struct aws_byte_
 struct aws_h1_decoder *aws_h1_decoder_new(struct aws_h1_decoder_params *params) {
     AWS_ASSERT(params);
 
-    struct aws_h1_decoder *decoder = aws_mem_acquire(params->alloc, sizeof(struct aws_h1_decoder));
-    if (!decoder) {
-        return NULL;
-    }
-    AWS_ZERO_STRUCT(*decoder);
+    struct aws_h1_decoder *decoder = aws_mem_calloc(params->alloc, 1, sizeof(struct aws_h1_decoder));
 
     decoder->alloc = params->alloc;
     decoder->user_data = params->user_data;

source/h1_encoder.c

@@ -542,10 +542,7 @@ struct aws_h1_chunk *aws_h1_chunk_new(struct aws_allocator *allocator, const str
     struct aws_h1_chunk *chunk;
     size_t chunk_line_size = s_calculate_chunk_line_size(options);
     void *chunk_line_storage;
-    if (!aws_mem_acquire_many(
-            allocator, 2, &chunk, sizeof(struct aws_h1_chunk), &chunk_line_storage, chunk_line_size)) {
-        return NULL;
-    }
+    aws_mem_acquire_many(allocator, 2, &chunk, sizeof(struct aws_h1_chunk), &chunk_line_storage, chunk_line_size);
 
     chunk->allocator = allocator;
     chunk->data = aws_input_stream_acquire(options->chunk_data);

source/h2_connection.c

@@ -304,9 +304,6 @@ static struct aws_h2_connection *s_connection_new(
     AWS_PRECONDITION(http2_options);
 
     struct aws_h2_connection *connection = aws_mem_calloc(alloc, 1, sizeof(struct aws_h2_connection));
-    if (!connection) {
-        return NULL;
-    }
     connection->base.vtable = &s_h2_connection_vtable;
     connection->base.alloc = alloc;
     connection->base.channel_handler.vtable = &s_h2_connection_vtable.channel_handler_vtable;
@@ -420,9 +417,7 @@ static struct aws_h2_connection *s_connection_new(
         http2_options->num_initial_settings,
         http2_options->on_initial_settings_completed,
         NULL /* user_data is set later... */);
-    if (!connection->thread_data.init_pending_settings) {
-        goto error;
-    }
+    AWS_ASSERT(connection->thread_data.init_pending_settings);
     /* We enqueue the inital settings when handler get installed */
     return connection;
 
@@ -511,15 +506,13 @@ static struct aws_h2_pending_settings *s_new_pending_settings(
     size_t settings_storage_size = sizeof(struct aws_http2_setting) * num_settings;
     struct aws_h2_pending_settings *pending_settings;
     void *settings_storage;
-    if (!aws_mem_acquire_many(
-            allocator,
-            2,
-            &pending_settings,
-            sizeof(struct aws_h2_pending_settings),
-            &settings_storage,
-            settings_storage_size)) {
-        return NULL;
-    }
+    aws_mem_acquire_many(
+        allocator,
+        2,
+        &pending_settings,
+        sizeof(struct aws_h2_pending_settings),
+        &settings_storage,
+        settings_storage_size);
 
     AWS_ZERO_STRUCT(*pending_settings);
     /* We buffer the settings up, incase the caller has freed them when the ACK arrives */
@@ -542,9 +535,7 @@ static struct aws_h2_pending_ping *s_new_pending_ping(
     aws_http2_on_ping_complete_fn *on_completed) {
 
     struct aws_h2_pending_ping *pending_ping = aws_mem_calloc(allocator, 1, sizeof(struct aws_h2_pending_ping));
-    if (!pending_ping) {
-        return NULL;
-    }
+
     if (optional_opaque_data) {
         memcpy(pending_ping->opaque_data, optional_opaque_data->ptr, AWS_HTTP2_PING_DATA_SIZE);
     }
@@ -1410,9 +1401,6 @@ static struct aws_h2err s_decoder_on_settings(
     struct aws_http2_setting *callback_array = NULL;
     if (num_settings) {
         callback_array = aws_mem_acquire(connection->base.alloc, num_settings * sizeof(struct aws_http2_setting));
-        if (!callback_array) {
-            return aws_h2err_from_last_error();
-        }
     }
     size_t callback_array_num = 0;
 
@@ -2082,9 +2070,6 @@ static struct aws_http_stream *s_connection_make_request(
 
     struct aws_h2_connection *connection = AWS_CONTAINER_OF(client_connection, struct aws_h2_connection, base);
 
-    /* #TODO: http/2-ify the request (ex: add ":method" header). Should we mutate a copy or the original? Validate?
-     *  Or just pass pointer to headers struct and let encoder transform it while encoding? */
-
     struct aws_h2_stream *stream = aws_h2_stream_new_request(client_connection, options);
     if (!stream) {
         CONNECTION_LOGF(
@@ -2112,8 +2097,24 @@ static struct aws_http_stream *s_connection_make_request(
             aws_error_name(aws_last_error()));
         goto error;
     }
+    struct aws_byte_cursor method;
+    AWS_ZERO_STRUCT(method);
+    struct aws_byte_cursor path;
+    AWS_ZERO_STRUCT(path);
 
-    AWS_H2_STREAM_LOG(DEBUG, stream, "Created HTTP/2 request stream"); /* #TODO: print method & path */
+    if (aws_http_message_get_request_method(stream->thread_data.outgoing_message, &method) ||
+        aws_http_message_get_request_path(stream->thread_data.outgoing_message, &path)) {
+        aws_raise_error(AWS_ERROR_HTTP_REQUIRED_PSEUDO_HEADER_MISSING);
+        CONNECTION_LOG(ERROR, connection, "Cannot create request stream, the :path or :method header is missing.");
+        goto error;
+    }
+
+    AWS_H2_STREAM_LOGF(
+        DEBUG,
+        stream,
+        "Created HTTP/2 request stream, method: " PRInSTR ". path: " PRInSTR "",
+        AWS_BYTE_CURSOR_PRI(method),
+        AWS_BYTE_CURSOR_PRI(path));
     return &stream->base;
 
 error:
@@ -2263,9 +2264,7 @@ static int s_connection_change_settings(
 
     struct aws_h2_pending_settings *pending_settings =
         s_new_pending_settings(connection->base.alloc, settings_array, num_settings, on_completed, user_data);
-    if (!pending_settings) {
-        return AWS_OP_ERR;
-    }
+    AWS_ASSERT(pending_settings);
     struct aws_h2_frame *settings_frame =
         aws_h2_frame_new_settings(connection->base.alloc, settings_array, num_settings, false /*ACK*/);
     if (!settings_frame) {
@@ -2818,22 +2817,26 @@ static void s_gather_statistics(struct aws_channel_handler *handler, struct aws_
     struct aws_h2_connection *connection = handler->impl;
     AWS_PRECONDITION(aws_channel_thread_is_callers_thread(connection->base.channel_slot->channel));
 
-    /* TODO: Need update the way we calculate statistics, to account for user-controlled pauses.
-     * If user is adding chunks 1 by 1, there can naturally be a gap in the upload.
-     * If the user lets the stream-window go to zero, there can naturally be a gap in the download. */
     uint64_t now_ns = 0;
     if (aws_channel_current_clock_time(connection->base.channel_slot->channel, &now_ns)) {
         return;
     }
 
     if (!aws_linked_list_empty(&connection->thread_data.outgoing_streams_list)) {
+        /**
+         * For stream flow control stall and writing to stream over time, the stream will be removed from the
+         * outgoing_streams_list.
+         * For connection level flow control, as there could be streams waiting for response, we cannot mark the
+         * connection is inactive.
+         */
         s_add_time_measurement_to_stats(
             connection->thread_data.outgoing_timestamp_ns,
             now_ns,
             &connection->thread_data.stats.pending_outgoing_stream_ms);
 
         connection->thread_data.outgoing_timestamp_ns = now_ns;
     }
+
     if (aws_hash_table_get_entry_count(&connection->thread_data.active_streams_map) != 0) {
         s_add_time_measurement_to_stats(
             connection->thread_data.incoming_timestamp_ns,
@@ -2842,6 +2845,9 @@ static void s_gather_statistics(struct aws_channel_handler *handler, struct aws_
 
         connection->thread_data.incoming_timestamp_ns = now_ns;
     } else {
+        /**
+         * was inactive as no stream has data to write or waiting for data from the other side.
+         */
         connection->thread_data.stats.was_inactive = true;
     }