Throw better errors for h2 manual write instead of invalid state error (#430)

Author: waahm7

include/aws/http/http.h

@@ -55,6 +55,8 @@ enum aws_http_errors {
     AWS_ERROR_HTTP_STREAM_MANAGER_CONNECTION_ACQUIRE_FAILURE,
     AWS_ERROR_HTTP_STREAM_MANAGER_UNEXPECTED_HTTP_VERSION,
     AWS_ERROR_HTTP_WEBSOCKET_PROTOCOL_ERROR,
+    AWS_ERROR_HTTP_MANUAL_WRITE_NOT_ENABLED,
+    AWS_ERROR_HTTP_MANUAL_WRITE_HAS_COMPLETED,
 
     AWS_ERROR_HTTP_END_RANGE = AWS_ERROR_ENUM_END_RANGE(AWS_C_HTTP_PACKAGE_ID)
 };

source/h2_stream.c

@@ -1254,6 +1254,14 @@ static int s_stream_write_data(
     struct aws_http_stream *stream_base,
     const struct aws_http2_stream_write_data_options *options) {
     struct aws_h2_stream *stream = AWS_CONTAINER_OF(stream_base, struct aws_h2_stream, base);
+    if (!stream->manual_write) {
+        AWS_H2_STREAM_LOG(
+            ERROR,
+            stream,
+            "Manual writes are not enabled. You need to enable manual writes using by setting "
+            "'http2_use_manual_data_writes' to true in 'aws_http_make_request_options'");
+        return aws_raise_error(AWS_ERROR_HTTP_MANUAL_WRITE_NOT_ENABLED);
+    }
     struct aws_h2_connection *connection = s_get_h2_connection(stream);
 
     /* queue this new write into the pending write list for the stream */
@@ -1272,23 +1280,20 @@ static int s_stream_write_data(
         {
             if (stream->synced_data.api_state != AWS_H2_STREAM_API_STATE_ACTIVE) {
                 s_unlock_synced_data(stream);
-                s_stream_data_write_destroy(stream, pending_write, AWS_ERROR_INVALID_STATE);
-                AWS_LOGF_ERROR(
-                    AWS_LS_HTTP_STREAM,
-                    "Cannot write DATA frames to an inactive or closed stream, stream=%p",
-                    (void *)stream_base);
-                return aws_raise_error(AWS_ERROR_INVALID_STATE);
+                int error_code = stream->synced_data.api_state == AWS_H2_STREAM_API_STATE_INIT
+                                     ? AWS_ERROR_HTTP_STREAM_NOT_ACTIVATED
+                                     : AWS_ERROR_HTTP_STREAM_HAS_COMPLETED;
+                s_stream_data_write_destroy(stream, pending_write, error_code);
+                AWS_H2_STREAM_LOG(ERROR, stream, "Cannot write DATA frames to an inactive or closed stream");
+                return aws_raise_error(error_code);
             }
 
             if (stream->synced_data.manual_write_ended) {
                 s_unlock_synced_data(stream);
-                s_stream_data_write_destroy(stream, pending_write, AWS_ERROR_INVALID_STATE);
-                AWS_LOGF_ERROR(
-                    AWS_LS_HTTP_STREAM,
-                    "Cannot write DATA frames to a stream after end, stream=%p",
-                    (void *)stream_base);
+                s_stream_data_write_destroy(stream, pending_write, AWS_ERROR_HTTP_MANUAL_WRITE_HAS_COMPLETED);
+                AWS_H2_STREAM_LOG(ERROR, stream, "Cannot write DATA frames to a stream after manual write ended");
                 /* Fail with error, otherwise, people can wait for on_complete callback that will never be invoked. */
-                return aws_raise_error(AWS_ERROR_INVALID_STATE);
+                return aws_raise_error(AWS_ERROR_HTTP_MANUAL_WRITE_HAS_COMPLETED);
             }
             /* Not setting this until we're sure we succeeded, so that callback doesn't fire on cleanup if we fail */
             if (options->end_stream) {

source/http.c

@@ -142,6 +142,12 @@ static struct aws_error_info s_errors[] = {
     AWS_DEFINE_ERROR_INFO_HTTP(
         AWS_ERROR_HTTP_WEBSOCKET_PROTOCOL_ERROR,
         "Websocket protocol rules violated by peer"),
+    AWS_DEFINE_ERROR_INFO_HTTP(
+        AWS_ERROR_HTTP_MANUAL_WRITE_NOT_ENABLED,
+        "Manual write failed because manual writes are not enabled."),
+    AWS_DEFINE_ERROR_INFO_HTTP(
+        AWS_ERROR_HTTP_MANUAL_WRITE_HAS_COMPLETED,
+        "Manual write failed because manual writes are already completed."),
 };
 /* clang-format on */
 

tests/CMakeLists.txt

@@ -482,6 +482,7 @@ add_test_case(h2_client_error_from_incoming_headers_callback_reset_stream)
 add_test_case(h2_client_error_from_incoming_headers_done_callback_reset_stream)
 add_test_case(h2_client_error_from_incoming_body_callback_reset_stream)
 add_test_case(h2_client_manual_data_write)
+add_test_case(h2_client_manual_data_write_not_enabled)
 add_test_case(h2_client_manual_data_write_with_body)
 add_test_case(h2_client_manual_data_write_no_data)
 add_test_case(h2_client_manual_data_write_connection_close)

tests/test_h2_client.c

@@ -5336,6 +5336,58 @@ TEST_CASE(h2_client_manual_data_write) {
     return s_tester_clean_up();
 }
 
+TEST_CASE(h2_client_manual_data_write_not_enabled) {
+
+    ASSERT_SUCCESS(s_tester_init(allocator, ctx));
+
+    struct aws_http_message *request = aws_http2_message_new_request(allocator);
+    ASSERT_NOT_NULL(request);
+
+    struct aws_http_header request_headers_src[] = {
+        DEFINE_HEADER(":method", "GET"),
+        DEFINE_HEADER(":scheme", "https"),
+        DEFINE_HEADER(":path", "/"),
+    };
+    aws_http_message_add_header_array(request, request_headers_src, AWS_ARRAY_SIZE(request_headers_src));
+    struct aws_http_make_request_options request_options = {
+        .self_size = sizeof(request_options),
+        .request = request,
+        .http2_use_manual_data_writes = false,
+    };
+    struct aws_http_stream *stream = aws_http_connection_make_request(s_tester.connection, &request_options);
+    ASSERT_NOT_NULL(stream);
+
+    aws_http_stream_activate(stream);
+
+    struct aws_byte_buf payload;
+    aws_byte_buf_init(&payload, allocator, 1024);
+
+    struct h2_client_manual_data_write_ctx test_ctx = {
+        .allocator = allocator,
+        .data = payload,
+    };
+
+    /* Try writing the data */
+    struct aws_input_stream *data_stream = s_h2_client_manual_data_write_generate_data(&test_ctx);
+    int64_t stream_length = 0;
+    ASSERT_SUCCESS(aws_input_stream_get_length(data_stream, &stream_length));
+    struct aws_http2_stream_write_data_options write_options = {
+        .data = data_stream,
+    };
+    ASSERT_ERROR(AWS_ERROR_HTTP_MANUAL_WRITE_NOT_ENABLED, aws_http2_stream_write_data(stream, &write_options));
+    aws_input_stream_release(data_stream);
+    aws_http_message_release(request);
+    aws_http_stream_release(stream);
+
+    /* close the connection */
+    aws_http_connection_close(s_tester.connection);
+
+    aws_byte_buf_clean_up(&test_ctx.data);
+
+    /* clean up */
+    return s_tester_clean_up();
+}
+
 TEST_CASE(h2_client_manual_data_write_with_body) {
 
     ASSERT_SUCCESS(s_tester_init(allocator, ctx));