Skip to content

Commit 0600662

Browse files
authored
Handle connection close corner case (#431)
1 parent 291b06c commit 0600662

File tree

3 files changed

+220
-31
lines changed

3 files changed

+220
-31
lines changed

source/h1_connection.c

+54
Original file line numberDiff line numberDiff line change
@@ -546,6 +546,33 @@ static void s_stream_complete(struct aws_h1_stream *stream, int error_code) {
546546
}
547547
}
548548

549+
if (error_code != AWS_ERROR_SUCCESS) {
550+
if (stream->base.client_data && stream->is_incoming_message_done) {
551+
/* As a request that finished receiving the response, we ignore error and
552+
* consider it finished successfully */
553+
AWS_LOGF_DEBUG(
554+
AWS_LS_HTTP_STREAM,
555+
"id=%p: Ignoring error code %d (%s). The response has been fully received,"
556+
"so the stream will complete successfully.",
557+
(void *)&stream->base,
558+
error_code,
559+
aws_error_name(error_code));
560+
error_code = AWS_ERROR_SUCCESS;
561+
}
562+
if (stream->base.server_data && stream->is_outgoing_message_done) {
563+
/* As a server finished sending the response, but still failed with the request was not finished receiving.
564+
* We ignore error and consider it finished successfully */
565+
AWS_LOGF_DEBUG(
566+
AWS_LS_HTTP_STREAM,
567+
"id=%p: Ignoring error code %d (%s). The response has been fully sent,"
568+
" so the stream will complete successfully",
569+
(void *)&stream->base,
570+
error_code,
571+
aws_error_name(error_code));
572+
error_code = AWS_ERROR_SUCCESS;
573+
}
574+
}
575+
549576
/* Remove stream from list. */
550577
aws_linked_list_remove(&stream->node);
551578

@@ -1066,6 +1093,33 @@ static int s_decoder_on_header(const struct aws_h1_decoded_header *header, void
10661093
connection->synced_data.new_stream_error_code = AWS_ERROR_HTTP_CONNECTION_CLOSED;
10671094
aws_h1_connection_unlock_synced_data(connection);
10681095
} /* END CRITICAL SECTION */
1096+
1097+
if (connection->base.client_data) {
1098+
/**
1099+
* RFC-9112 section 9.6.
1100+
* A client that receives a "close" connection option MUST cease sending
1101+
* requests on that connection and close the connection after reading the
1102+
* response message containing the "close" connection option.
1103+
*
1104+
* Mark the stream's outgoing message as complete,
1105+
* so that we stop sending, and stop waiting for it to finish sending.
1106+
**/
1107+
if (!incoming_stream->is_outgoing_message_done) {
1108+
AWS_LOGF_DEBUG(
1109+
AWS_LS_HTTP_STREAM,
1110+
"id=%p: Received 'Connection: close' header, no more request data will be sent.",
1111+
(void *)&incoming_stream->base);
1112+
incoming_stream->is_outgoing_message_done = true;
1113+
}
1114+
/* Stop writing right now.
1115+
* Shutdown will be scheduled after we finishing parsing the response */
1116+
s_stop(
1117+
connection,
1118+
false /*stop_reading*/,
1119+
true /*stop_writing*/,
1120+
false /*schedule_shutdown*/,
1121+
AWS_ERROR_SUCCESS);
1122+
}
10691123
}
10701124
}
10711125

tests/CMakeLists.txt

+22-14
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,8 @@ add_test_case(h1_client_midchannel_requires_switching_protocols)
142142
add_test_case(h1_client_switching_protocols_fails_pending_requests)
143143
add_test_case(h1_client_switching_protocols_fails_subsequent_requests)
144144
add_test_case(h1_client_switching_protocols_requires_downstream_handler)
145+
add_test_case(h1_client_connection_close_before_request_finishes)
146+
add_test_case(h1_client_response_close_connection_before_request_finishes)
145147

146148
add_test_case(strutil_trim_http_whitespace)
147149
add_test_case(strutil_is_http_token)
@@ -199,6 +201,7 @@ add_test_case(websocket_handler_delayed_write_completion)
199201
add_test_case(websocket_handler_send_halts_if_payload_fn_returns_false)
200202
add_test_case(websocket_handler_shutdown_automatically_sends_close_frame)
201203
add_test_case(websocket_handler_shutdown_handles_queued_close_frame)
204+
202205
# add_test_case(websocket_handler_shutdown_immediately_in_emergency) disabled until channel API exposes immediate shutdown
203206
add_test_case(websocket_handler_shutdown_handles_unexpected_write_error)
204207
add_test_case(websocket_handler_close_on_thread)
@@ -268,7 +271,7 @@ add_test_case(hpack_dynamic_table_empty_value)
268271
add_test_case(hpack_dynamic_table_with_empty_header)
269272
add_test_case(hpack_dynamic_table_size_update_from_setting)
270273

271-
if (ENABLE_LOCALHOST_INTEGRATION_TESTS)
274+
if(ENABLE_LOCALHOST_INTEGRATION_TESTS)
272275
# Tests should be named with localhost_integ_*
273276
add_net_test_case(localhost_integ_hpack_stress)
274277
add_net_test_case(localhost_integ_hpack_compression_stress)
@@ -401,7 +404,8 @@ add_test_case(h2_client_unactivated_stream_cleans_up)
401404
add_test_case(h2_client_connection_preface_sent)
402405
add_test_case(h2_client_auto_ping_ack)
403406
add_test_case(h2_client_auto_ping_ack_higher_priority)
404-
#TODO add_test_case(h2_client_auto_ping_ack_higher_priority_not_break_encoding_frame)
407+
408+
# TODO add_test_case(h2_client_auto_ping_ack_higher_priority_not_break_encoding_frame)
405409
add_test_case(h2_client_auto_settings_ack)
406410
add_test_case(h2_client_stream_complete)
407411
add_test_case(h2_client_close)
@@ -444,7 +448,8 @@ add_test_case(h2_client_push_promise_automatically_rejected)
444448
add_test_case(h2_client_conn_receive_goaway)
445449
add_test_case(h2_client_conn_receive_goaway_debug_data)
446450
add_test_case(h2_client_conn_err_invalid_last_stream_id_goaway)
447-
#TODO add_test_case(h2_client_send_goaway_with_push_promises) id of 1st should be in GOAWAY 2nd should be ignored
451+
452+
# TODO add_test_case(h2_client_send_goaway_with_push_promises) id of 1st should be in GOAWAY 2nd should be ignored
448453
add_test_case(h2_client_change_settings_succeed)
449454
add_test_case(h2_client_change_settings_failed_no_ack_received)
450455
add_test_case(h2_client_manual_window_management_disabled_auto_window_update)
@@ -454,10 +459,11 @@ add_test_case(h2_client_manual_window_management_user_send_stream_window_update_
454459
add_test_case(h2_client_manual_window_management_user_send_conn_window_update)
455460
add_test_case(h2_client_manual_window_management_user_send_conn_window_update_with_padding)
456461
add_test_case(h2_client_manual_window_management_user_send_connection_window_update_overflow)
462+
457463
# Build these when we address window_update() differences in H1 vs H2
458-
#TODO add_test_case(h2_client_manual_updated_window_ignored_when_automatical_on)
459-
#TODO add_test_case(h2_client_manual_stream_updated_window_ignored_invalid_state)
460-
#TODO add_test_case(h2_client_manual_window_management_window_overflow) #we cannot ensure the increment_size is safe or not, let our peer detect the maximum exceed or not. But we can test the obviously overflows here.
464+
# TODO add_test_case(h2_client_manual_updated_window_ignored_when_automatical_on)
465+
# TODO add_test_case(h2_client_manual_stream_updated_window_ignored_invalid_state)
466+
# TODO add_test_case(h2_client_manual_window_management_window_overflow) #we cannot ensure the increment_size is safe or not, let our peer detect the maximum exceed or not. But we can test the obviously overflows here.
461467
add_test_case(h2_client_send_ping_successfully_receive_ack)
462468
add_test_case(h2_client_send_ping_no_ack_received)
463469
add_test_case(h2_client_conn_err_extraneous_ping_ack_received)
@@ -496,10 +502,11 @@ add_test_case(connection_h2_prior_knowledge)
496502
add_test_case(connection_h2_prior_knowledge_not_work_with_tls)
497503
add_test_case(connection_customized_alpn)
498504
add_test_case(connection_customized_alpn_error_with_unknown_return_string)
505+
499506
# These server tests occasionally fail. Resurrect if/when we get back to work on HTTP server.
500-
#add_test_case(connection_destroy_server_with_connection_existing)
501-
#add_test_case(connection_destroy_server_with_multiple_connections_existing)
502-
#add_test_case(connection_server_shutting_down_new_connection_setup_fail)
507+
# add_test_case(connection_destroy_server_with_connection_existing)
508+
# add_test_case(connection_destroy_server_with_multiple_connections_existing)
509+
# add_test_case(connection_server_shutting_down_new_connection_setup_fail)
503510

504511
# connection manager tests
505512
# unit tests where connections are mocked
@@ -526,7 +533,7 @@ add_net_test_case(test_connection_manager_acquire_release_mix)
526533

527534
# Integration test that requires proxy envrionment in us-east-1 region.
528535
# TODO: test the server name validation properly
529-
if (ENABLE_PROXY_INTEGRATION_TESTS)
536+
if(ENABLE_PROXY_INTEGRATION_TESTS)
530537
add_net_test_case(connection_manager_proxy_integration_forwarding_proxy_no_auth)
531538
add_net_test_case(connection_manager_proxy_integration_forwarding_proxy_no_auth_env)
532539
add_net_test_case(connection_manager_proxy_integration_legacy_http_no_auth)
@@ -646,8 +653,9 @@ add_net_test_case(h2_sm_acquire_stream)
646653
add_net_test_case(h2_sm_acquire_stream_multiple_connections)
647654
add_net_test_case(h2_sm_closing_before_connection_acquired)
648655
add_net_test_case(h2_sm_close_connection_on_server_error)
656+
649657
# Tests against local server
650-
if (ENABLE_LOCALHOST_INTEGRATION_TESTS)
658+
if(ENABLE_LOCALHOST_INTEGRATION_TESTS)
651659
# Tests should be named with localhost_integ_*
652660
add_net_test_case(localhost_integ_h2_sm_prior_knowledge)
653661
add_net_test_case(localhost_integ_h2_sm_acquire_stream_stress)
@@ -669,7 +677,7 @@ generate_test_driver(${TEST_BINARY_NAME})
669677
file(GLOB FUZZ_TESTS "fuzz/*.c")
670678
aws_add_fuzz_tests("${FUZZ_TESTS}" "" "")
671679

672-
#SSL certificates to use for testing.
680+
# SSL certificates to use for testing.
673681
add_custom_command(TARGET ${TEST_BINARY_NAME} PRE_BUILD
674-
COMMAND ${CMAKE_COMMAND} -E copy_directory
675-
${CMAKE_CURRENT_SOURCE_DIR}/resources $<TARGET_FILE_DIR:${TEST_BINARY_NAME}>)
682+
COMMAND ${CMAKE_COMMAND} -E copy_directory
683+
${CMAKE_CURRENT_SOURCE_DIR}/resources $<TARGET_FILE_DIR:${TEST_BINARY_NAME}>)

tests/test_h1_client.c

+144-17
Original file line numberDiff line numberDiff line change
@@ -2260,30 +2260,33 @@ static struct aws_input_stream_vtable s_slow_stream_vtable = {
22602260
.get_length = s_slow_stream_get_length,
22612261
};
22622262

2263+
static void s_slow_body_sender_init(struct slow_body_sender *body_sender) {
2264+
/* set up request whose body won't send immediately */
2265+
struct aws_input_stream empty_stream_base;
2266+
AWS_ZERO_STRUCT(empty_stream_base);
2267+
body_sender->base = empty_stream_base;
2268+
body_sender->status.is_end_of_stream = false;
2269+
body_sender->status.is_valid = true;
2270+
struct aws_byte_cursor body = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("write more tests");
2271+
body_sender->cursor = body;
2272+
body_sender->delay_ticks = 5;
2273+
body_sender->bytes_per_tick = 1;
2274+
2275+
body_sender->base.vtable = &s_slow_stream_vtable;
2276+
aws_ref_count_init(
2277+
&body_sender->base.ref_count, &body_sender, (aws_simple_completion_callback *)s_slow_stream_destroy);
2278+
}
2279+
22632280
/* It should be fine to receive a response before the request has finished sending */
22642281
H1_CLIENT_TEST_CASE(h1_client_response_arrives_before_request_done_sending_is_ok) {
22652282
(void)ctx;
22662283
struct tester tester;
22672284
ASSERT_SUCCESS(s_tester_init(&tester, allocator));
22682285

22692286
/* set up request whose body won't send immediately */
2270-
struct aws_input_stream empty_stream_base;
2271-
AWS_ZERO_STRUCT(empty_stream_base);
2272-
struct slow_body_sender body_sender = {
2273-
.base = empty_stream_base,
2274-
.status =
2275-
{
2276-
.is_end_of_stream = false,
2277-
.is_valid = true,
2278-
},
2279-
.cursor = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("write more tests"),
2280-
.delay_ticks = 5,
2281-
.bytes_per_tick = 1,
2282-
};
2283-
body_sender.base.vtable = &s_slow_stream_vtable;
2284-
aws_ref_count_init(
2285-
&body_sender.base.ref_count, &body_sender, (aws_simple_completion_callback *)s_slow_stream_destroy);
2286-
2287+
struct slow_body_sender body_sender;
2288+
AWS_ZERO_STRUCT(body_sender);
2289+
s_slow_body_sender_init(&body_sender);
22872290
struct aws_input_stream *body_stream = &body_sender.base;
22882291

22892292
struct aws_http_header headers[] = {
@@ -4156,3 +4159,127 @@ H1_CLIENT_TEST_CASE(h1_client_switching_protocols_requires_downstream_handler) {
41564159
ASSERT_SUCCESS(s_tester_clean_up(&tester));
41574160
return AWS_OP_SUCCESS;
41584161
}
4162+
4163+
H1_CLIENT_TEST_CASE(h1_client_connection_close_before_request_finishes) {
4164+
(void)ctx;
4165+
struct tester tester;
4166+
ASSERT_SUCCESS(s_tester_init(&tester, allocator));
4167+
4168+
/* set up request whose body won't send immediately */
4169+
struct slow_body_sender body_sender;
4170+
AWS_ZERO_STRUCT(body_sender);
4171+
s_slow_body_sender_init(&body_sender);
4172+
struct aws_input_stream *body_stream = &body_sender.base;
4173+
4174+
struct aws_http_header headers[] = {
4175+
{
4176+
.name = aws_byte_cursor_from_c_str("Content-Length"),
4177+
.value = aws_byte_cursor_from_c_str("16"),
4178+
},
4179+
};
4180+
4181+
struct aws_http_message *request = aws_http_message_new_request(allocator);
4182+
ASSERT_NOT_NULL(request);
4183+
ASSERT_SUCCESS(aws_http_message_set_request_method(request, aws_byte_cursor_from_c_str("PUT")));
4184+
ASSERT_SUCCESS(aws_http_message_set_request_path(request, aws_byte_cursor_from_c_str("/plan.txt")));
4185+
ASSERT_SUCCESS(aws_http_message_add_header_array(request, headers, AWS_ARRAY_SIZE(headers)));
4186+
aws_http_message_set_body_stream(request, body_stream);
4187+
4188+
struct client_stream_tester stream_tester;
4189+
ASSERT_SUCCESS(s_stream_tester_init(&stream_tester, &tester, request));
4190+
4191+
/* send head of request */
4192+
testing_channel_run_currently_queued_tasks(&tester.testing_channel);
4193+
4194+
/* Ensure the request can be destroyed after request is sent */
4195+
aws_http_message_destroy(request);
4196+
aws_input_stream_release(body_stream);
4197+
4198+
/* send close connection response */
4199+
ASSERT_SUCCESS(testing_channel_push_read_str(
4200+
&tester.testing_channel,
4201+
"HTTP/1.1 404 Not Found\r\n"
4202+
"Date: Fri, 01 Mar 2019 17:18:55 GMT\r\n"
4203+
"\r\n"));
4204+
4205+
testing_channel_run_currently_queued_tasks(&tester.testing_channel);
4206+
4207+
aws_channel_shutdown(tester.testing_channel.channel, AWS_ERROR_SUCCESS);
4208+
/* Wait for channel to finish shutdown */
4209+
testing_channel_drain_queued_tasks(&tester.testing_channel);
4210+
/* check result, should not receive any body */
4211+
const char *expected = "PUT /plan.txt HTTP/1.1\r\n"
4212+
"Content-Length: 16\r\n"
4213+
"\r\n";
4214+
ASSERT_SUCCESS(testing_channel_check_written_messages_str(&tester.testing_channel, allocator, expected));
4215+
4216+
ASSERT_TRUE(stream_tester.complete);
4217+
ASSERT_INT_EQUALS(AWS_ERROR_SUCCESS, stream_tester.on_complete_error_code);
4218+
4219+
/* clean up */
4220+
client_stream_tester_clean_up(&stream_tester);
4221+
ASSERT_SUCCESS(s_tester_clean_up(&tester));
4222+
return AWS_OP_SUCCESS;
4223+
}
4224+
4225+
/* When response has `connection: close` any further request body should not be sent. */
4226+
H1_CLIENT_TEST_CASE(h1_client_response_close_connection_before_request_finishes) {
4227+
(void)ctx;
4228+
struct tester tester;
4229+
ASSERT_SUCCESS(s_tester_init(&tester, allocator));
4230+
4231+
/* set up request whose body won't send immediately */
4232+
struct slow_body_sender body_sender;
4233+
AWS_ZERO_STRUCT(body_sender);
4234+
s_slow_body_sender_init(&body_sender);
4235+
struct aws_input_stream *body_stream = &body_sender.base;
4236+
4237+
struct aws_http_header headers[] = {
4238+
{
4239+
.name = aws_byte_cursor_from_c_str("Content-Length"),
4240+
.value = aws_byte_cursor_from_c_str("16"),
4241+
},
4242+
};
4243+
4244+
struct aws_http_message *request = aws_http_message_new_request(allocator);
4245+
ASSERT_NOT_NULL(request);
4246+
ASSERT_SUCCESS(aws_http_message_set_request_method(request, aws_byte_cursor_from_c_str("PUT")));
4247+
ASSERT_SUCCESS(aws_http_message_set_request_path(request, aws_byte_cursor_from_c_str("/plan.txt")));
4248+
ASSERT_SUCCESS(aws_http_message_add_header_array(request, headers, AWS_ARRAY_SIZE(headers)));
4249+
aws_http_message_set_body_stream(request, body_stream);
4250+
4251+
struct client_stream_tester stream_tester;
4252+
ASSERT_SUCCESS(s_stream_tester_init(&stream_tester, &tester, request));
4253+
4254+
/* send head of request */
4255+
testing_channel_run_currently_queued_tasks(&tester.testing_channel);
4256+
4257+
/* Ensure the request can be destroyed after request is sent */
4258+
aws_http_message_destroy(request);
4259+
aws_input_stream_release(body_stream);
4260+
4261+
/* send close connection response */
4262+
ASSERT_SUCCESS(testing_channel_push_read_str(
4263+
&tester.testing_channel,
4264+
"HTTP/1.1 404 Not Found\r\n"
4265+
"Date: Fri, 01 Mar 2019 17:18:55 GMT\r\n"
4266+
"Connection: close\r\n"
4267+
"\r\n"));
4268+
4269+
testing_channel_drain_queued_tasks(&tester.testing_channel);
4270+
/* check result, should not receive any body */
4271+
const char *expected = "PUT /plan.txt HTTP/1.1\r\n"
4272+
"Content-Length: 16\r\n"
4273+
"\r\n";
4274+
ASSERT_SUCCESS(testing_channel_check_written_messages_str(&tester.testing_channel, allocator, expected));
4275+
/* Check if the testing channel has shut down. */
4276+
ASSERT_TRUE(testing_channel_is_shutdown_completed(&tester.testing_channel));
4277+
4278+
ASSERT_TRUE(stream_tester.complete);
4279+
ASSERT_INT_EQUALS(AWS_ERROR_SUCCESS, stream_tester.on_complete_error_code);
4280+
4281+
/* clean up */
4282+
client_stream_tester_clean_up(&stream_tester);
4283+
ASSERT_SUCCESS(s_tester_clean_up(&tester));
4284+
return AWS_OP_SUCCESS;
4285+
}

0 commit comments

Comments
 (0)