Skip to content

Commit

Permalink
Merge pull request #14804 from yang-g/too_many_pings
Browse files Browse the repository at this point in the history 
cherrypick PR14787 to 1.10.x
  • Loading branch information
@yang-g
yang-g committed Mar 26, 2018
2 parents 474c595 + 1f7e8cb commit 5065e35
Show file tree
Hide file tree
Showing 4 changed files with 172 additions and 12 deletions.
2 changes: 1 addition & 1 deletion src/core/ext/transport/chttp2/transport/chttp2_transport.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@

#define DEFAULT_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS 300000 /* 5 minutes */
#define DEFAULT_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS 300000 /* 5 minutes */
#define DEFAULT_MAX_PINGS_BETWEEN_DATA 0 /* unlimited */
#define DEFAULT_MAX_PINGS_BETWEEN_DATA 2
#define DEFAULT_MAX_PING_STRIKES 2

static int g_default_client_keepalive_time_ms =
Expand Down
4 changes: 0 additions & 4 deletions src/core/ext/transport/chttp2/transport/parsing.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,6 @@ static grpc_error* init_data_frame_parser(grpc_chttp2_transport* t) {
/* t->parser = grpc_chttp2_data_parser_parse;*/
t->parser = grpc_chttp2_data_parser_parse;
t->parser_data = &s->data_parser;
t->ping_state.pings_before_data_required =
t->ping_policy.max_pings_without_data;
t->ping_state.last_ping_sent_time = GRPC_MILLIS_INF_PAST;
return GRPC_ERROR_NONE;
} else if (grpc_error_get_int(err, GRPC_ERROR_INT_STREAM_ID, nullptr)) {
Expand Down Expand Up @@ -545,8 +543,6 @@ static grpc_error* init_header_frame_parser(grpc_chttp2_transport* t,
(t->incoming_frame_flags & GRPC_CHTTP2_DATA_FLAG_END_STREAM) != 0;
}

t->ping_state.pings_before_data_required =
t->ping_policy.max_pings_without_data;
t->ping_state.last_ping_sent_time = GRPC_MILLIS_INF_PAST;

/* could be a new grpc_chttp2_stream or an existing grpc_chttp2_stream */
Expand Down
14 changes: 8 additions & 6 deletions src/core/ext/transport/chttp2/transport/writing.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ class WriteContext {
grpc_slice_buffer_add(
&t_->outbuf, grpc_chttp2_window_update_create(0, transport_announce,
&throwaway_stats));
ResetPingRecvClock();
ResetPingClock();
}
}

Expand Down Expand Up @@ -267,11 +267,13 @@ class WriteContext {
return s;
}

void ResetPingRecvClock() {
void ResetPingClock() {
if (!t_->is_client) {
t_->ping_recv_state.last_ping_recv_time = GRPC_MILLIS_INF_PAST;
t_->ping_recv_state.ping_strikes = 0;
}
t_->ping_state.pings_before_data_required =
t_->ping_policy.max_pings_without_data;
}

void IncInitialMetadataWrites() { ++initial_metadata_writes_; }
Expand Down Expand Up @@ -433,7 +435,7 @@ class StreamWriteContext {
};
grpc_chttp2_encode_header(&t_->hpack_compressor, nullptr, 0,
s_->send_initial_metadata, &hopt, &t_->outbuf);
write_context_->ResetPingRecvClock();
write_context_->ResetPingClock();
write_context_->IncInitialMetadataWrites();
}

Expand All @@ -453,7 +455,7 @@ class StreamWriteContext {
grpc_slice_buffer_add(
&t_->outbuf, grpc_chttp2_window_update_create(s_->id, stream_announce,
&s_->stats.outgoing));
write_context_->ResetPingRecvClock();
write_context_->ResetPingClock();
write_context_->IncWindowUpdateWrites();
}

Expand Down Expand Up @@ -487,7 +489,7 @@ class StreamWriteContext {
data_send_context.CompressMoreBytes();
}
}
write_context_->ResetPingRecvClock();
write_context_->ResetPingClock();
if (data_send_context.is_last_frame()) {
SentLastFrame();
}
Expand Down Expand Up @@ -528,7 +530,7 @@ class StreamWriteContext {
s_->send_trailing_metadata, &hopt, &t_->outbuf);
}
write_context_->IncTrailingMetadataWrites();
write_context_->ResetPingRecvClock();
write_context_->ResetPingClock();
SentLastFrame();

write_context_->NoteScheduledResults();
Expand Down
164 changes: 163 additions & 1 deletion test/core/end2end/tests/bad_ping.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
#include "src/core/lib/gpr/useful.h"
#include "test/core/end2end/cq_verifier.h"

#define MAX_PING_STRIKES 1
#define MAX_PING_STRIKES 2

static void* tag(intptr_t t) { return (void*)t; }

Expand Down Expand Up @@ -63,6 +63,7 @@ static void end_test(grpc_end2end_test_fixture* f) {
grpc_completion_queue_destroy(f->shutdown_cq);
}

// Send more pings than server allows to trigger server's GOAWAY.
static void test_bad_ping(grpc_end2end_test_config config) {
grpc_end2end_test_fixture f = config.create_fixture(nullptr, nullptr);
cq_verifier* cqv = cq_verifier_create(f.cq);
Expand Down Expand Up @@ -221,9 +222,170 @@ static void test_bad_ping(grpc_end2end_test_config config) {
config.tear_down_data(&f);
}

// Try sending more pings than server allows, but server should be fine because
// max_pings_without_data should limit pings sent out on wire.
static void test_pings_without_data(grpc_end2end_test_config config) {
grpc_end2end_test_fixture f = config.create_fixture(nullptr, nullptr);
cq_verifier* cqv = cq_verifier_create(f.cq);
grpc_arg client_a[3];
client_a[0].type = GRPC_ARG_INTEGER;
client_a[0].key =
const_cast<char*>(GRPC_ARG_HTTP2_MIN_SENT_PING_INTERVAL_WITHOUT_DATA_MS);
client_a[0].value.integer = 10;
// Only allow MAX_PING_STRIKES pings without data (DATA/HEADERS/WINDOW_UPDATE)
// so that the transport will throttle the excess pings.
client_a[1].type = GRPC_ARG_INTEGER;
client_a[1].key = const_cast<char*>(GRPC_ARG_HTTP2_MAX_PINGS_WITHOUT_DATA);
client_a[1].value.integer = MAX_PING_STRIKES;
client_a[2].type = GRPC_ARG_INTEGER;
client_a[2].key = const_cast<char*>(GRPC_ARG_HTTP2_BDP_PROBE);
client_a[2].value.integer = 0;
grpc_arg server_a[3];
server_a[0].type = GRPC_ARG_INTEGER;
server_a[0].key =
const_cast<char*>(GRPC_ARG_HTTP2_MIN_RECV_PING_INTERVAL_WITHOUT_DATA_MS);
server_a[0].value.integer = 300000 /* 5 minutes */;
server_a[1].type = GRPC_ARG_INTEGER;
server_a[1].key = const_cast<char*>(GRPC_ARG_HTTP2_MAX_PING_STRIKES);
server_a[1].value.integer = MAX_PING_STRIKES;
server_a[2].type = GRPC_ARG_INTEGER;
server_a[2].key = const_cast<char*>(GRPC_ARG_HTTP2_BDP_PROBE);
server_a[2].value.integer = 0;
grpc_channel_args client_args = {GPR_ARRAY_SIZE(client_a), client_a};
grpc_channel_args server_args = {GPR_ARRAY_SIZE(server_a), server_a};

config.init_client(&f, &client_args);
config.init_server(&f, &server_args);

grpc_call* c;
grpc_call* s;
gpr_timespec deadline = grpc_timeout_seconds_to_deadline(10);
grpc_op ops[6];
grpc_op* op;
grpc_metadata_array initial_metadata_recv;
grpc_metadata_array trailing_metadata_recv;
grpc_metadata_array request_metadata_recv;
grpc_call_details call_details;
grpc_status_code status;
grpc_call_error error;
grpc_slice details;
int was_cancelled = 2;

c = grpc_channel_create_call(
f.client, nullptr, GRPC_PROPAGATE_DEFAULTS, f.cq,
grpc_slice_from_static_string("/foo"),
get_host_override_slice("foo.test.google.fr:1234", config), deadline,
nullptr);
GPR_ASSERT(c);

grpc_metadata_array_init(&initial_metadata_recv);
grpc_metadata_array_init(&trailing_metadata_recv);
grpc_metadata_array_init(&request_metadata_recv);
grpc_call_details_init(&call_details);

memset(ops, 0, sizeof(ops));
op = ops;
op->op = GRPC_OP_SEND_INITIAL_METADATA;
op->data.send_initial_metadata.count = 0;
op->data.send_initial_metadata.metadata = nullptr;
op->flags = 0;
op->reserved = nullptr;
op++;
op->op = GRPC_OP_SEND_CLOSE_FROM_CLIENT;
op->flags = 0;
op->reserved = nullptr;
op++;
op->op = GRPC_OP_RECV_INITIAL_METADATA;
op->data.recv_initial_metadata.recv_initial_metadata = &initial_metadata_recv;
op->flags = 0;
op->reserved = nullptr;
op++;
op->op = GRPC_OP_RECV_STATUS_ON_CLIENT;
op->data.recv_status_on_client.trailing_metadata = &trailing_metadata_recv;
op->data.recv_status_on_client.status = &status;
op->data.recv_status_on_client.status_details = &details;
op->flags = 0;
op->reserved = nullptr;
op++;
error = grpc_call_start_batch(c, ops, static_cast<size_t>(op - ops), tag(1),
nullptr);
GPR_ASSERT(GRPC_CALL_OK == error);

error =
grpc_server_request_call(f.server, &s, &call_details,
&request_metadata_recv, f.cq, f.cq, tag(101));
GPR_ASSERT(GRPC_CALL_OK == error);
CQ_EXPECT_COMPLETION(cqv, tag(101), 1);
cq_verify(cqv);

// Send too many pings to the server similar to the prevous test case.
// However, since we set the MAX_PINGS_WITHOUT_DATA at the client side, only
// MAX_PING_STRIKES will actually be sent and the rpc will still succeed.
int i;
for (i = 1; i <= MAX_PING_STRIKES + 2; i++) {
grpc_channel_ping(f.client, f.cq, tag(200 + i), nullptr);
if (i <= MAX_PING_STRIKES) {
CQ_EXPECT_COMPLETION(cqv, tag(200 + i), 1);
}
cq_verify(cqv);
}

memset(ops, 0, sizeof(ops));
op = ops;
op->op = GRPC_OP_SEND_INITIAL_METADATA;
op->data.send_initial_metadata.count = 0;
op->flags = 0;
op->reserved = nullptr;
op++;
op->op = GRPC_OP_SEND_STATUS_FROM_SERVER;
op->data.send_status_from_server.trailing_metadata_count = 0;
op->data.send_status_from_server.status = GRPC_STATUS_UNIMPLEMENTED;
grpc_slice status_details = grpc_slice_from_static_string("xyz");
op->data.send_status_from_server.status_details = &status_details;
op->flags = 0;
op->reserved = nullptr;
op++;
op->op = GRPC_OP_RECV_CLOSE_ON_SERVER;
op->data.recv_close_on_server.cancelled = &was_cancelled;
op->flags = 0;
op->reserved = nullptr;
op++;
error = grpc_call_start_batch(s, ops, static_cast<size_t>(op - ops), tag(102),
nullptr);
GPR_ASSERT(GRPC_CALL_OK == error);

CQ_EXPECT_COMPLETION(cqv, tag(102), 1);
// Client call should return.
CQ_EXPECT_COMPLETION(cqv, tag(1), 1);
cq_verify(cqv);

grpc_server_shutdown_and_notify(f.server, f.cq, tag(0xdead));
CQ_EXPECT_COMPLETION(cqv, tag(0xdead), 1);
cq_verify(cqv);

grpc_call_unref(s);

// The rpc should be successful.
GPR_ASSERT(status == GRPC_STATUS_UNIMPLEMENTED);
GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo"));
validate_host_override_string("foo.test.google.fr:1234", call_details.host,
config);

grpc_slice_unref(details);
grpc_metadata_array_destroy(&initial_metadata_recv);
grpc_metadata_array_destroy(&trailing_metadata_recv);
grpc_metadata_array_destroy(&request_metadata_recv);
grpc_call_details_destroy(&call_details);
grpc_call_unref(c);
cq_verifier_destroy(cqv);
end_test(&f);
config.tear_down_data(&f);
}

void bad_ping(grpc_end2end_test_config config) {
GPR_ASSERT(config.feature_mask & FEATURE_MASK_SUPPORTS_DELAYED_CONNECTION);
test_bad_ping(config);
test_pings_without_data(config);
}

void bad_ping_pre_init(void) {}

0 comments on commit 5065e35

Please sign in to comment.