Skip to content

Commit

Permalink
Fixing counter bug with the number of prepared requests. (#111)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rccarper
rccarper authored Apr 13, 2021
1 parent 53870cd commit e0ad63b
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 32 deletions.
12 changes: 8 additions & 4 deletions include/aws/s3/private/s3_client_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,8 @@ struct aws_s3_client_vtable {

void (
*setup_vip_connection_retry_token)(struct aws_s3_client *client, struct aws_s3_vip_connection *vip_connection);

void (*finish_destroy)(struct aws_s3_client *client);
};

/* Represents the state of the S3 client. */
Expand Down Expand Up @@ -209,15 +211,18 @@ struct aws_s3_client {
struct {
struct aws_mutex lock;

/* Endpoint to use for the bucket. */
struct aws_string *endpoint;

/* How many vips are being actively used. */
uint32_t active_vip_count;

/* How many vips are allocated. (This number includes vips that are in the process of cleaning up) */
uint32_t allocated_vip_count;

/* How many requests failed to be prepared. */
uint32_t num_failed_prepare_requests;

/* Endpoint to use for the bucket. */
struct aws_string *endpoint;

/* Linked list of active VIP's. */
struct aws_linked_list vips;

Expand Down Expand Up @@ -377,7 +382,6 @@ void aws_s3_client_unlock_synced_data(struct aws_s3_client *client);

AWS_S3_API
extern const uint32_t g_max_num_connections_per_vip;

AWS_EXTERN_C_END

#endif /* AWS_S3_CLIENT_IMPL_H */
40 changes: 29 additions & 11 deletions source/s3_client.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ AWS_STATIC_STRING_FROM_LITERAL(s_http_proxy_env_var, "HTTP_PROXY");
static void s_s3_client_start_destroy(void *user_data);

/* Called by s_s3_client_process_work_default when all shutdown criteria has been met. */
static void s_s3_client_finish_destroy(void *user_data);
static void s_s3_client_finish_destroy_default(struct aws_s3_client *client);

/* Called when the body streaming elg shutdown has completed. */
static void s_s3_client_body_streaming_elg_shutdown(void *user_data);
Expand Down Expand Up @@ -158,6 +158,7 @@ static struct aws_s3_client_vtable s_s3_client_default_vtable = {
.vip_connection_destroy = s_s3_vip_connection_destroy_default,
.schedule_process_work_synced = s_s3_client_schedule_process_work_synced_default,
.process_work = s_s3_client_process_work_default,
.finish_destroy = s_s3_client_finish_destroy_default,
};

void aws_s3_set_dns_ttl(size_t ttl) {
Expand Down Expand Up @@ -449,9 +450,8 @@ static void s_s3_client_start_destroy(void *user_data) {
aws_s3_client_unlock_synced_data(client);
}

static void s_s3_client_finish_destroy(void *user_data) {
AWS_PRECONDITION(user_data);
struct aws_s3_client *client = user_data;
static void s_s3_client_finish_destroy_default(struct aws_s3_client *client) {
AWS_PRECONDITION(client);

AWS_LOGF_DEBUG(AWS_LS_S3_CLIENT, "id=%p Client finishing destruction.", (void *)client);

Expand Down Expand Up @@ -1277,6 +1277,8 @@ static void s_s3_client_process_work_task(struct aws_task *task, void *arg, enum

static void s_s3_client_process_work_default(struct aws_s3_client *client) {
AWS_PRECONDITION(client);
AWS_PRECONDITION(client->vtable);
AWS_PRECONDITION(client->vtable->finish_destroy);

bool client_active = false;
bool invalid_endpoint = false;
Expand Down Expand Up @@ -1308,13 +1310,27 @@ static void s_s3_client_process_work_default(struct aws_s3_client *client) {
uint32_t num_requests_queued =
aws_s3_client_queue_requests_threaded(client, &client->synced_data.prepared_requests, false);

int sub_result = aws_sub_u32_checked(
client->threaded_data.num_requests_being_prepared,
num_requests_queued,
&client->threaded_data.num_requests_being_prepared);
{
int sub_result = aws_sub_u32_checked(
client->threaded_data.num_requests_being_prepared,
num_requests_queued,
&client->threaded_data.num_requests_being_prepared);

AWS_ASSERT(sub_result == AWS_OP_SUCCESS);
(void)sub_result;
}

{
int sub_result = aws_sub_u32_checked(
client->threaded_data.num_requests_being_prepared,
client->synced_data.num_failed_prepare_requests,
&client->threaded_data.num_requests_being_prepared);

AWS_ASSERT(sub_result == AWS_OP_SUCCESS);
(void)sub_result;
client->synced_data.num_failed_prepare_requests = 0;

AWS_ASSERT(sub_result == AWS_OP_SUCCESS);
(void)sub_result;
}

aws_s3_client_unlock_synced_data(client);

Expand Down Expand Up @@ -1464,7 +1480,7 @@ static void s_s3_client_process_work_default(struct aws_s3_client *client) {
aws_s3_client_unlock_synced_data(client);

if (finish_destroy) {
s_s3_client_finish_destroy(client);
client->vtable->finish_destroy(client);
}
}
}
Expand Down Expand Up @@ -1593,6 +1609,8 @@ static void s_s3_client_prepare_callback_queue_request(

if (error_code == AWS_ERROR_SUCCESS) {
aws_linked_list_push_back(&client->synced_data.prepared_requests, &request->node);
} else {
++client->synced_data.num_failed_prepare_requests;
}

s_s3_client_schedule_process_work_synced(client);
Expand Down
53 changes: 36 additions & 17 deletions tests/s3_retry_tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,25 +114,18 @@ static int s_test_s3_client_acquire_connection_fail(struct aws_allocator *alloca
return 0;
}

static int s_s3_fail_first_prepare_request(struct aws_s3_meta_request *meta_request, struct aws_s3_request *request) {
struct s3_fail_prepare_test_data {
uint32_t num_requests_being_prepared_is_correct : 1;
};

static int s_s3_fail_prepare_request(struct aws_s3_meta_request *meta_request, struct aws_s3_request *request) {
(void)meta_request;
(void)request;
AWS_ASSERT(meta_request != NULL);
AWS_ASSERT(request != NULL);

struct aws_s3_client *client = meta_request->client;
AWS_ASSERT(client != NULL);

struct aws_s3_tester *tester = client->shutdown_callback_user_data;
AWS_ASSERT(tester != NULL);

if (aws_s3_tester_inc_counter1(tester) == 1) {
aws_raise_error(AWS_ERROR_UNKNOWN);
return AWS_OP_ERR;
}

struct aws_s3_meta_request_vtable *original_meta_request_vtable =
aws_s3_tester_get_meta_request_vtable_patch(tester, 0)->original_vtable;

return original_meta_request_vtable->prepare_request(meta_request, request);
aws_raise_error(AWS_ERROR_UNKNOWN);
return AWS_OP_ERR;
}

static struct aws_s3_meta_request *s_meta_request_factory_patch_prepare_request(
Expand All @@ -151,11 +144,28 @@ static struct aws_s3_meta_request *s_meta_request_factory_patch_prepare_request(

struct aws_s3_meta_request_vtable *patched_meta_request_vtable =
aws_s3_tester_patch_meta_request_vtable(tester, meta_request, NULL);
patched_meta_request_vtable->prepare_request = s_s3_fail_first_prepare_request;
patched_meta_request_vtable->prepare_request = s_s3_fail_prepare_request;

return meta_request;
}

static void s_s3_fail_prepare_finish_destroy(struct aws_s3_client *client) {
AWS_ASSERT(client);

struct aws_s3_tester *tester = client->shutdown_callback_user_data;
AWS_ASSERT(tester != NULL);

struct s3_fail_prepare_test_data *test_data = tester->user_data;
AWS_ASSERT(test_data != NULL);

test_data->num_requests_being_prepared_is_correct = client->threaded_data.num_requests_being_prepared == 0;

struct aws_s3_client_vtable *original_client_vtable =
aws_s3_tester_get_client_vtable_patch(tester, 0)->original_vtable;

original_client_vtable->finish_destroy(client);
}

/* Test recovery when prepare request fails. */
AWS_TEST_CASE(test_s3_meta_request_fail_prepare_request, s_test_s3_meta_request_fail_prepare_request)
static int s_test_s3_meta_request_fail_prepare_request(struct aws_allocator *allocator, void *ctx) {
Expand All @@ -164,6 +174,10 @@ static int s_test_s3_meta_request_fail_prepare_request(struct aws_allocator *all
struct aws_s3_tester tester;
ASSERT_SUCCESS(aws_s3_tester_init(allocator, &tester));

struct s3_fail_prepare_test_data test_data;
AWS_ZERO_STRUCT(test_data);
tester.user_data = &test_data;

struct aws_s3_client_config client_config;
AWS_ZERO_STRUCT(client_config);

Expand All @@ -173,14 +187,19 @@ static int s_test_s3_meta_request_fail_prepare_request(struct aws_allocator *all
struct aws_s3_client *client = aws_s3_client_new(allocator, &client_config);
struct aws_s3_client_vtable *patched_client_vtable = aws_s3_tester_patch_client_vtable(&tester, client, NULL);
patched_client_vtable->meta_request_factory = s_meta_request_factory_patch_prepare_request;
patched_client_vtable->finish_destroy = s_s3_fail_prepare_finish_destroy;

ASSERT_SUCCESS(aws_s3_tester_send_get_object_meta_request(&tester, client, g_s3_path_get_object_test_1MB, 0, NULL));

aws_s3_tester_wait_for_counters(&tester);

aws_s3_client_release(client);
client = NULL;

aws_s3_tester_clean_up(&tester);

ASSERT_TRUE(test_data.num_requests_being_prepared_is_correct);

return 0;
}

Expand Down

0 comments on commit e0ad63b

Please sign in to comment.