From 33aa40ab2b493e0dd40e3a7363e2f62d7d125727 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 9 Sep 2016 14:16:51 -0700 Subject: [PATCH 01/51] Add an extra Cronet read after a message read is complete and when trailing metadata is received --- .../cronet/transport/cronet_transport.c | 32 ++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 25ad40b935a..5154fdf3de6 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -58,7 +58,7 @@ } while (0) /* TODO (makdharma): Hook up into the wider tracing mechanism */ -int grpc_cronet_trace = 0; +int grpc_cronet_trace = 1; enum e_op_result { ACTION_TAKEN_WITH_CALLBACK, @@ -509,8 +509,20 @@ static void on_response_trailers_received( s->state.rs.trailing_metadata_valid = true; } s->state.state_callback_received[OP_RECV_TRAILING_METADATA] = true; - gpr_mu_unlock(&s->mu); - execute_from_storage(s); + if (!s->state.state_op_done[OP_READ_REQ_MADE]) { + /* Do an extra read to trigger on_succeeded() callback in case connection + is closed */ + s->state.rs.received_bytes = 0; + s->state.rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; + s->state.rs.length_field_received = false; + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); + cronet_bidirectional_stream_read(s->cbs, s->state.rs.read_buffer, + s->state.rs.remaining_bytes); + gpr_mu_unlock(&s->mu); + } else { + gpr_mu_unlock(&s->mu); + execute_from_storage(s); + } } /* @@ -935,11 +947,15 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; oas->state.state_op_done[OP_RECV_MESSAGE] = true; - /* Clear read state of the stream, so next read op (if it were to come) - * will work */ - stream_state->rs.received_bytes = stream_state->rs.remaining_bytes = - stream_state->rs.length_field_received = 0; - result = ACTION_TAKEN_NO_CALLBACK; + /* Do an extra read to trigger on_succeeded() callback in case connection + is closed */ + stream_state->rs.received_bytes = 0; + stream_state->rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; + stream_state->rs.length_field_received = false; + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); + cronet_bidirectional_stream_read(s->cbs, stream_state->rs.read_buffer, + stream_state->rs.remaining_bytes); + result = ACTION_TAKEN_WITH_CALLBACK; } } else if (stream_op->recv_trailing_metadata && op_can_be_run(stream_op, stream_state, &oas->state, From 6e0d5676ea4559fc23b4a0d865b8072502b012ec Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 9 Sep 2016 17:35:34 -0700 Subject: [PATCH 02/51] Minor bug fix --- src/core/ext/transport/cronet/transport/cronet_transport.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 5154fdf3de6..444873988b3 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -955,7 +955,7 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); cronet_bidirectional_stream_read(s->cbs, stream_state->rs.read_buffer, stream_state->rs.remaining_bytes); - result = ACTION_TAKEN_WITH_CALLBACK; + result = ACTION_TAKEN_NO_CALLBACK; } } else if (stream_op->recv_trailing_metadata && op_can_be_run(stream_op, stream_state, &oas->state, From f2f3bb89514e4453b5c26458fac333d58a07e85d Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Thu, 15 Sep 2016 16:00:54 -0700 Subject: [PATCH 03/51] Add an extra Cronet read after response header is received --- .../cronet/transport/cronet_transport.c | 30 +++++++++---------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 444873988b3..8b074f4ea85 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -432,6 +432,15 @@ static void on_response_headers_received( grpc_mdstr_from_string(headers->headers[i].value))); } s->state.state_callback_received[OP_RECV_INITIAL_METADATA] = true; + /* Do an extra read to trigger on_succeeded() callback in case connection + is closed */ + GPR_ASSERT(s->state.rs.length_field_received == false); + s->state.rs.read_buffer = s->state.rs.grpc_header_bytes; + s->state.rs.received_bytes = 0; + s->state.rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); + cronet_bidirectional_stream_read(s->cbs, s->state.rs.read_buffer, + s->state.rs.remaining_bytes); gpr_mu_unlock(&s->mu); execute_from_storage(s); } @@ -509,20 +518,8 @@ static void on_response_trailers_received( s->state.rs.trailing_metadata_valid = true; } s->state.state_callback_received[OP_RECV_TRAILING_METADATA] = true; - if (!s->state.state_op_done[OP_READ_REQ_MADE]) { - /* Do an extra read to trigger on_succeeded() callback in case connection - is closed */ - s->state.rs.received_bytes = 0; - s->state.rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; - s->state.rs.length_field_received = false; - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); - cronet_bidirectional_stream_read(s->cbs, s->state.rs.read_buffer, - s->state.rs.remaining_bytes); - gpr_mu_unlock(&s->mu); - } else { - gpr_mu_unlock(&s->mu); - execute_from_storage(s); - } + gpr_mu_unlock(&s->mu); + execute_from_storage(s); } /* @@ -633,9 +630,9 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, /* When call is canceled, every op can be run, except under following conditions */ - bool is_canceled_of_failed = stream_state->state_op_done[OP_CANCEL_ERROR] || + bool is_canceled_or_failed = stream_state->state_op_done[OP_CANCEL_ERROR] || stream_state->state_callback_received[OP_FAILED]; - if (is_canceled_of_failed) { + if (is_canceled_or_failed) { if (op_id == OP_SEND_INITIAL_METADATA) result = false; if (op_id == OP_SEND_MESSAGE) result = false; if (op_id == OP_SEND_TRAILING_METADATA) result = false; @@ -949,6 +946,7 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, oas->state.state_op_done[OP_RECV_MESSAGE] = true; /* Do an extra read to trigger on_succeeded() callback in case connection is closed */ + stream_state->rs.read_buffer = stream_state->rs.grpc_header_bytes; stream_state->rs.received_bytes = 0; stream_state->rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; stream_state->rs.length_field_received = false; From 3ec218ba2620fdfd8e8f5c12c84edad9c909e8fc Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 20 Sep 2016 18:36:57 -0700 Subject: [PATCH 04/51] Fix a bug that causes cronet read when cancelled or failed --- .../cronet/transport/cronet_transport.c | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 8b074f4ea85..eefe1a889cb 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -432,15 +432,18 @@ static void on_response_headers_received( grpc_mdstr_from_string(headers->headers[i].value))); } s->state.state_callback_received[OP_RECV_INITIAL_METADATA] = true; - /* Do an extra read to trigger on_succeeded() callback in case connection - is closed */ - GPR_ASSERT(s->state.rs.length_field_received == false); - s->state.rs.read_buffer = s->state.rs.grpc_header_bytes; - s->state.rs.received_bytes = 0; - s->state.rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); - cronet_bidirectional_stream_read(s->cbs, s->state.rs.read_buffer, - s->state.rs.remaining_bytes); + if (!(s->state.state_op_done[OP_CANCEL_ERROR] || + s->state.state_callback_received[OP_FAILED])) { + /* Do an extra read to trigger on_succeeded() callback in case connection + is closed */ + GPR_ASSERT(s->state.rs.length_field_received == false); + s->state.rs.read_buffer = s->state.rs.grpc_header_bytes; + s->state.rs.received_bytes = 0; + s->state.rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); + cronet_bidirectional_stream_read(s->cbs, s->state.rs.read_buffer, + s->state.rs.remaining_bytes); + } gpr_mu_unlock(&s->mu); execute_from_storage(s); } From 106af4995f936944c581ce8026913fe452ea830a Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Thu, 22 Sep 2016 16:21:31 -0700 Subject: [PATCH 05/51] Fix a bug that causes cronet stuck when server initiate termination of stream --- .../transport/cronet/transport/cronet_transport.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index eefe1a889cb..7457b30d25f 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -521,8 +521,18 @@ static void on_response_trailers_received( s->state.rs.trailing_metadata_valid = true; } s->state.state_callback_received[OP_RECV_TRAILING_METADATA] = true; - gpr_mu_unlock(&s->mu); - execute_from_storage(s); + /* Send a EOS when server terminates the stream to trigger on_succeeded */ + if (!s->state.state_op_done[OP_SEND_TRAILING_METADATA]) { + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p, 0)", s->cbs); + s->state.state_callback_received[OP_SEND_MESSAGE] = false; + cronet_bidirectional_stream_write(s->cbs, "", 0, true); + s->state.state_op_done[OP_SEND_TRAILING_METADATA] = true; + + gpr_mu_unlock(&s->mu); + } else { + gpr_mu_unlock(&s->mu); + execute_from_storage(s); + } } /* From 7faef2188d46a8db60cafc5751712227867e826e Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 23 Sep 2016 16:31:25 -0700 Subject: [PATCH 06/51] Add cronet read buffer flush when error grpc-status is received --- .../cronet/transport/cronet_transport.c | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 7457b30d25f..5e202d0ee28 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -148,6 +148,8 @@ struct write_state { struct op_state { bool state_op_done[OP_NUM_OPS]; bool state_callback_received[OP_NUM_OPS]; + bool fail_state; + bool flush_read; /* data structure for storing data coming from server */ struct read_state rs; /* data structure for storing data going to the server */ @@ -475,7 +477,11 @@ static void on_read_completed(cronet_bidirectional_stream *stream, char *data, count); gpr_mu_lock(&s->mu); s->state.state_callback_received[OP_RECV_MESSAGE] = true; - if (count > 0) { + if (count > 0 && s->state.flush_read) { + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); + cronet_bidirectional_stream_read(s->cbs, s->state.rs.read_buffer, 4096); + gpr_mu_unlock(&s->mu); + } else if (count > 0) { s->state.rs.received_bytes += count; s->state.rs.remaining_bytes -= count; if (s->state.rs.remaining_bytes > 0) { @@ -490,6 +496,10 @@ static void on_read_completed(cronet_bidirectional_stream *stream, char *data, execute_from_storage(s); } } else { + if (s->state.flush_read) { + gpr_free(s->state.rs.read_buffer); + s->state.rs.read_buffer = NULL; + } s->state.rs.read_stream_closed = true; gpr_mu_unlock(&s->mu); execute_from_storage(s); @@ -519,6 +529,10 @@ static void on_response_trailers_received( grpc_mdstr_from_string(trailers->headers[i].key), grpc_mdstr_from_string(trailers->headers[i].value))); s->state.rs.trailing_metadata_valid = true; + if (0 == strcmp(trailers->headers[i].key, "grpc-status") && + 0 != strcmp(trailers->headers[i].value, "0")) { + s->state.fail_state = true; + } } s->state.state_callback_received[OP_RECV_TRAILING_METADATA] = true; /* Send a EOS when server terminates the stream to trigger on_succeeded */ @@ -790,6 +804,7 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, OP_SEND_INITIAL_METADATA)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_SEND_INITIAL_METADATA", oas); /* This OP is the beginning. Reset various states */ + stream_state->fail_state = stream_state->flush_read = false; memset(&s->header_array, 0, sizeof(s->header_array)); memset(&stream_state->rs, 0, sizeof(stream_state->rs)); memset(&stream_state->ws, 0, sizeof(stream_state->ws)); @@ -1026,6 +1041,15 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, make a note */ if (stream_op->recv_message) stream_state->state_op_done[OP_RECV_MESSAGE_AND_ON_COMPLETE] = true; + } else if (stream_state->fail_state && !stream_state->flush_read) { + CRONET_LOG(GPR_DEBUG, "running: %p flush read", oas); + if (stream_state->rs.read_buffer && + stream_state->rs.read_buffer != stream_state->rs.grpc_header_bytes) { + gpr_free(stream_state->rs.read_buffer); + stream_state->rs.read_buffer = NULL; + } + stream_state->rs.read_buffer = gpr_malloc(4096); + stream_state->flush_read = true; } else { result = NO_ACTION_POSSIBLE; } From 638d03b197c562291024b5f8a1ed9ad08db1b922 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Fri, 21 Oct 2016 18:07:48 -0700 Subject: [PATCH 07/51] Minor bug fix --- src/core/ext/transport/cronet/transport/cronet_transport.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 5e202d0ee28..71481c05398 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -535,8 +535,9 @@ static void on_response_trailers_received( } } s->state.state_callback_received[OP_RECV_TRAILING_METADATA] = true; - /* Send a EOS when server terminates the stream to trigger on_succeeded */ - if (!s->state.state_op_done[OP_SEND_TRAILING_METADATA]) { + /* Send a EOS when server terminates the stream (testServerFinishesRequest) to trigger on_succeeded */ + if (!s->state.state_op_done[OP_SEND_TRAILING_METADATA] && + !(s->state.state_op_done[OP_CANCEL_ERROR] || s->state.state_callback_received[OP_FAILED])) { CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p, 0)", s->cbs); s->state.state_callback_received[OP_SEND_MESSAGE] = false; cronet_bidirectional_stream_write(s->cbs, "", 0, true); From c08f53ab353912ea41c9e342cc597d7604c99816 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 24 Oct 2016 10:25:15 -0700 Subject: [PATCH 08/51] Turn off cronet_transport logs and clang-format --- .../ext/transport/cronet/transport/cronet_transport.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 71481c05398..5d012921ee2 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -58,7 +58,7 @@ } while (0) /* TODO (makdharma): Hook up into the wider tracing mechanism */ -int grpc_cronet_trace = 1; +int grpc_cronet_trace = 0; enum e_op_result { ACTION_TAKEN_WITH_CALLBACK, @@ -535,9 +535,11 @@ static void on_response_trailers_received( } } s->state.state_callback_received[OP_RECV_TRAILING_METADATA] = true; - /* Send a EOS when server terminates the stream (testServerFinishesRequest) to trigger on_succeeded */ + /* Send a EOS when server terminates the stream (testServerFinishesRequest) to + * trigger on_succeeded */ if (!s->state.state_op_done[OP_SEND_TRAILING_METADATA] && - !(s->state.state_op_done[OP_CANCEL_ERROR] || s->state.state_callback_received[OP_FAILED])) { + !(s->state.state_op_done[OP_CANCEL_ERROR] || + s->state.state_callback_received[OP_FAILED])) { CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p, 0)", s->cbs); s->state.state_callback_received[OP_SEND_MESSAGE] = false; cronet_bidirectional_stream_write(s->cbs, "", 0, true); From ea0d61f80666b92cf4dc5fa3b2fc6fc460e48f9e Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 28 Nov 2016 15:07:28 -0800 Subject: [PATCH 09/51] Return correct status on cancel --- .../cronet/transport/cronet_transport.c | 52 ++++++++++++------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 961e0a5acc7..4ac2ebf04e5 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -151,6 +151,7 @@ struct op_state { bool state_callback_received[OP_NUM_OPS]; bool fail_state; bool flush_read; + grpc_error *cancel_error; /* data structure for storing data coming from server */ struct read_state rs; /* data structure for storing data going to the server */ @@ -250,6 +251,12 @@ static void free_read_buffer(stream_obj *s) { } } +static grpc_error* make_error_with_desc(int error_code, const char *desc) { + grpc_error *error = GRPC_ERROR_CREATE(desc); + error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS, error_code); + return error; +} + /* Add a new stream op to op storage. */ @@ -817,17 +824,10 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, op_can_be_run(stream_op, stream_state, &oas->state, OP_SEND_INITIAL_METADATA)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_SEND_INITIAL_METADATA", oas); - /* This OP is the beginning. Reset various states */ - stream_state->fail_state = stream_state->flush_read = false; - memset(&s->header_array, 0, sizeof(s->header_array)); - memset(&stream_state->rs, 0, sizeof(stream_state->rs)); - memset(&stream_state->ws, 0, sizeof(stream_state->ws)); - memset(stream_state->state_op_done, 0, sizeof(stream_state->state_op_done)); - memset(stream_state->state_callback_received, 0, - sizeof(stream_state->state_callback_received)); /* Start new cronet stream. It is destroyed in on_succeeded, on_canceled, * on_failed */ GPR_ASSERT(s->cbs == NULL); + GPR_ASSERT(!stream_state->state_op_done[OP_SEND_INITIAL_METADATA]); s->cbs = cronet_bidirectional_stream_create(s->curr_ct.engine, s->curr_gs, &cronet_callbacks); CRONET_LOG(GPR_DEBUG, "%p = cronet_bidirectional_stream_create()", s->cbs); @@ -848,10 +848,11 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, op_can_be_run(stream_op, stream_state, &oas->state, OP_RECV_INITIAL_METADATA)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_RECV_INITIAL_METADATA", oas); - if (stream_state->state_op_done[OP_CANCEL_ERROR] || - stream_state->state_callback_received[OP_FAILED]) { + if (stream_state->state_op_done[OP_CANCEL_ERROR]) { grpc_exec_ctx_sched(exec_ctx, stream_op->recv_initial_metadata_ready, GRPC_ERROR_CANCELLED, NULL); + } else if (stream_state->state_callback_received[OP_FAILED]) { + grpc_exec_ctx_sched(exec_ctx, stream_op->recv_initial_metadata_ready, make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); } else { grpc_chttp2_incoming_metadata_buffer_publish( &oas->s->state.rs.initial_metadata, stream_op->recv_initial_metadata); @@ -905,12 +906,16 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, op_can_be_run(stream_op, stream_state, &oas->state, OP_RECV_MESSAGE)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_RECV_MESSAGE", oas); - if (stream_state->state_op_done[OP_CANCEL_ERROR] || - stream_state->state_callback_received[OP_FAILED]) { - CRONET_LOG(GPR_DEBUG, "Stream is either cancelled or failed."); + if (stream_state->state_op_done[OP_CANCEL_ERROR]) { + CRONET_LOG(GPR_DEBUG, "Stream is cancelled."); grpc_exec_ctx_sched(exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_CANCELLED, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; + } else if (stream_state->state_callback_received[OP_FAILED]) { + CRONET_LOG(GPR_DEBUG, "Stream failed."); + grpc_exec_ctx_sched(exec_ctx, stream_op->recv_message_ready, + make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); + stream_state->state_op_done[OP_RECV_MESSAGE] = true; } else if (stream_state->rs.read_stream_closed == true) { /* No more data will be received */ CRONET_LOG(GPR_DEBUG, "read stream closed"); @@ -1031,17 +1036,23 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, CRONET_LOG(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); if (s->cbs) { cronet_bidirectional_stream_cancel(s->cbs); + result = ACTION_TAKEN_WITH_CALLBACK; + } else { + result = ACTION_TAKEN_NO_CALLBACK; } stream_state->state_op_done[OP_CANCEL_ERROR] = true; - result = ACTION_TAKEN_WITH_CALLBACK; + if (!stream_state->cancel_error) { + stream_state->cancel_error = GRPC_ERROR_REF(stream_op->cancel_error); + } } else if (stream_op->on_complete && op_can_be_run(stream_op, stream_state, &oas->state, OP_ON_COMPLETE)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_ON_COMPLETE", oas); - if (stream_state->state_op_done[OP_CANCEL_ERROR] || - stream_state->state_callback_received[OP_FAILED]) { + if (stream_state->state_op_done[OP_CANCEL_ERROR]) { grpc_exec_ctx_sched(exec_ctx, stream_op->on_complete, - GRPC_ERROR_CANCELLED, NULL); + GRPC_ERROR_REF(stream_state->cancel_error), NULL); + } else if (stream_state->state_callback_received[OP_FAILED]) { + grpc_exec_ctx_sched(exec_ctx, stream_op->on_complete, make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); } else { /* All actions in this stream_op are complete. Call the on_complete * callback @@ -1096,6 +1107,8 @@ static int init_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, memset(s->state.state_op_done, 0, sizeof(s->state.state_op_done)); memset(s->state.state_callback_received, 0, sizeof(s->state.state_callback_received)); + s->state.fail_state = s->state.flush_read = false; + s->state.cancel_error = NULL; gpr_mu_init(&s->mu); return 0; } @@ -1142,7 +1155,10 @@ static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, } static void destroy_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, - grpc_stream *gs, void *and_free_memory) {} + grpc_stream *gs, void *and_free_memory) { + stream_obj *s = (stream_obj *)gs; + GRPC_ERROR_UNREF(s->state.cancel_error); +} static void destroy_transport(grpc_exec_ctx *exec_ctx, grpc_transport *gt) {} From a6b88dfcd642784d6566b342d086f862c717d138 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 28 Nov 2016 16:19:28 -0800 Subject: [PATCH 10/51] Change behavior of RECV_MESSAGE --- src/core/ext/transport/cronet/transport/cronet_transport.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 4ac2ebf04e5..69e2e27c087 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -59,7 +59,7 @@ } while (0) /* TODO (makdharma): Hook up into the wider tracing mechanism */ -int grpc_cronet_trace = 0; +int grpc_cronet_trace = 1; enum e_op_result { ACTION_TAKEN_WITH_CALLBACK, @@ -911,11 +911,13 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, grpc_exec_ctx_sched(exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_CANCELLED, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; + result = ACTION_TAKEN_NO_CALLBACK; } else if (stream_state->state_callback_received[OP_FAILED]) { CRONET_LOG(GPR_DEBUG, "Stream failed."); grpc_exec_ctx_sched(exec_ctx, stream_op->recv_message_ready, make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; + result = ACTION_TAKEN_NO_CALLBACK; } else if (stream_state->rs.read_stream_closed == true) { /* No more data will be received */ CRONET_LOG(GPR_DEBUG, "read stream closed"); @@ -923,6 +925,7 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; oas->state.state_op_done[OP_RECV_MESSAGE] = true; + result = ACTION_TAKEN_NO_CALLBACK; } else if (stream_state->rs.length_field_received == false) { if (stream_state->rs.received_bytes == GRPC_HEADER_SIZE_IN_BYTES && stream_state->rs.remaining_bytes == 0) { From fdbca15194a3e1d70b54d90931c39cf771923301 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 28 Nov 2016 16:44:57 -0800 Subject: [PATCH 11/51] Mark unsupported tests --- .../tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m b/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m index 4a92cc8e0d3..4ba7badd866 100644 --- a/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m +++ b/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m @@ -316,7 +316,8 @@ static char *roots_filename; } - (void)testInvokeLargeRequest { - [self testIndividualCase:"invoke_large_request"]; + // NOT SUPPORTED (frame size) + // [self testIndividualCase:"invoke_large_request"]; } - (void)testLargeMetadata { @@ -329,7 +330,8 @@ static char *roots_filename; } - (void)testMaxMessageLength { - [self testIndividualCase:"max_message_length"]; + // NOT SUPPORTED (close_error) + // [self testIndividualCase:"max_message_length"]; } - (void)testNegativeDeadline { From 72e409655ece13b84f8b110a7bcb904187737254 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Tue, 29 Nov 2016 10:07:43 -0800 Subject: [PATCH 12/51] clang-format --- .../transport/cronet/transport/cronet_transport.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 69e2e27c087..c3abb47735a 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -251,7 +251,7 @@ static void free_read_buffer(stream_obj *s) { } } -static grpc_error* make_error_with_desc(int error_code, const char *desc) { +static grpc_error *make_error_with_desc(int error_code, const char *desc) { grpc_error *error = GRPC_ERROR_CREATE(desc); error = grpc_error_set_int(error, GRPC_ERROR_INT_GRPC_STATUS, error_code); return error; @@ -852,7 +852,9 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, grpc_exec_ctx_sched(exec_ctx, stream_op->recv_initial_metadata_ready, GRPC_ERROR_CANCELLED, NULL); } else if (stream_state->state_callback_received[OP_FAILED]) { - grpc_exec_ctx_sched(exec_ctx, stream_op->recv_initial_metadata_ready, make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); + grpc_exec_ctx_sched( + exec_ctx, stream_op->recv_initial_metadata_ready, + make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); } else { grpc_chttp2_incoming_metadata_buffer_publish( &oas->s->state.rs.initial_metadata, stream_op->recv_initial_metadata); @@ -914,8 +916,9 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, result = ACTION_TAKEN_NO_CALLBACK; } else if (stream_state->state_callback_received[OP_FAILED]) { CRONET_LOG(GPR_DEBUG, "Stream failed."); - grpc_exec_ctx_sched(exec_ctx, stream_op->recv_message_ready, - make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); + grpc_exec_ctx_sched( + exec_ctx, stream_op->recv_message_ready, + make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; result = ACTION_TAKEN_NO_CALLBACK; } else if (stream_state->rs.read_stream_closed == true) { @@ -1055,7 +1058,9 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, grpc_exec_ctx_sched(exec_ctx, stream_op->on_complete, GRPC_ERROR_REF(stream_state->cancel_error), NULL); } else if (stream_state->state_callback_received[OP_FAILED]) { - grpc_exec_ctx_sched(exec_ctx, stream_op->on_complete, make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); + grpc_exec_ctx_sched( + exec_ctx, stream_op->on_complete, + make_error_with_desc(GRPC_STATUS_UNAVAILABLE, "Unavailable."), NULL); } else { /* All actions in this stream_op are complete. Call the on_complete * callback From 99080d1488ab1d286d2019a15b3f18f0a6e630fb Mon Sep 17 00:00:00 2001 From: Robbie Shade Date: Tue, 29 Nov 2016 14:59:57 -0500 Subject: [PATCH 13/51] Fix TSAN failure when running DEBUG mode. --- src/core/lib/surface/completion_queue.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/core/lib/surface/completion_queue.c b/src/core/lib/surface/completion_queue.c index 4e0feb56acc..184c1a1a16a 100644 --- a/src/core/lib/surface/completion_queue.c +++ b/src/core/lib/surface/completion_queue.c @@ -354,11 +354,13 @@ static void dump_pending_tags(grpc_completion_queue *cc) { gpr_strvec v; gpr_strvec_init(&v); gpr_strvec_add(&v, gpr_strdup("PENDING TAGS:")); + gpr_mu_lock(cc->mu); for (size_t i = 0; i < cc->outstanding_tag_count; i++) { char *s; gpr_asprintf(&s, " %p", cc->outstanding_tags[i]); gpr_strvec_add(&v, s); } + gpr_mu_unlock(cc->mu); char *out = gpr_strvec_flatten(&v, NULL); gpr_strvec_destroy(&v); gpr_log(GPR_DEBUG, "%s", out); From db25f083c2ec0e2912aecaed4ae2826865305320 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 2 Dec 2016 12:45:24 -0800 Subject: [PATCH 14/51] Make TCP error messages more descriptive --- src/core/lib/iomgr/tcp_posix.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/core/lib/iomgr/tcp_posix.c b/src/core/lib/iomgr/tcp_posix.c index 12a4797e6f4..fd807795199 100644 --- a/src/core/lib/iomgr/tcp_posix.c +++ b/src/core/lib/iomgr/tcp_posix.c @@ -107,6 +107,12 @@ typedef struct { grpc_resource_user_slice_allocator slice_allocator; } grpc_tcp; +static grpc_error *tcp_annotate_error(grpc_error *src_error, grpc_tcp *tcp) { + return grpc_error_set_str( + grpc_error_set_int(src_error, GRPC_ERROR_INT_FD, tcp->fd), + GRPC_ERROR_STR_TARGET_ADDRESS, tcp->peer_string); +} + static void tcp_handle_read(grpc_exec_ctx *exec_ctx, void *arg /* grpc_tcp */, grpc_error *error); static void tcp_handle_write(grpc_exec_ctx *exec_ctx, void *arg /* grpc_tcp */, @@ -230,13 +236,15 @@ static void tcp_do_read(grpc_exec_ctx *exec_ctx, grpc_tcp *tcp) { grpc_fd_notify_on_read(exec_ctx, tcp->em_fd, &tcp->read_closure); } else { grpc_slice_buffer_reset_and_unref(tcp->incoming_buffer); - call_read_cb(exec_ctx, tcp, GRPC_OS_ERROR(errno, "recvmsg")); + call_read_cb(exec_ctx, tcp, + tcp_annotate_error(GRPC_OS_ERROR(errno, "recvmsg"), tcp)); TCP_UNREF(exec_ctx, tcp, "read"); } } else if (read_bytes == 0) { /* 0 read size ==> end of stream */ grpc_slice_buffer_reset_and_unref(tcp->incoming_buffer); - call_read_cb(exec_ctx, tcp, GRPC_ERROR_CREATE("Socket closed")); + call_read_cb(exec_ctx, tcp, + tcp_annotate_error(GRPC_ERROR_CREATE("Socket closed"), tcp)); TCP_UNREF(exec_ctx, tcp, "read"); } else { GPR_ASSERT((size_t)read_bytes <= tcp->incoming_buffer->length); @@ -366,7 +374,7 @@ static bool tcp_flush(grpc_tcp *tcp, grpc_error **error) { tcp->outgoing_byte_idx = unwind_byte_idx; return false; } else { - *error = GRPC_OS_ERROR(errno, "sendmsg"); + *error = tcp_annotate_error(GRPC_OS_ERROR(errno, "sendmsg"), tcp); return true; } } @@ -447,9 +455,10 @@ static void tcp_write(grpc_exec_ctx *exec_ctx, grpc_endpoint *ep, if (buf->length == 0) { GPR_TIMER_END("tcp_write", 0); - grpc_exec_ctx_sched(exec_ctx, cb, grpc_fd_is_shutdown(tcp->em_fd) - ? GRPC_ERROR_CREATE("EOF") - : GRPC_ERROR_NONE, + grpc_exec_ctx_sched(exec_ctx, cb, + grpc_fd_is_shutdown(tcp->em_fd) + ? tcp_annotate_error(GRPC_ERROR_CREATE("EOF"), tcp) + : GRPC_ERROR_NONE, NULL); return; } From c494c7b104faf393839ff33dc02f0994dec70ba1 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 2 Dec 2016 17:10:06 -0800 Subject: [PATCH 15/51] Fix some things: - use after free for resource quota - stuck-in-combiner-lock bug for End2endTest.ClientCancelsBidi/1 under asan/epoll - fixes clang-format stuff for a few files --- .../transport/chttp2/server/chttp2_server.c | 16 +++-- .../transport/chttp2/server/chttp2_server.h | 3 +- .../chttp2/server/insecure/server_chttp2.c | 2 +- .../server/secure/server_secure_chttp2.c | 6 +- .../chttp2/transport/chttp2_transport.c | 61 +++++++------------ .../ext/transport/chttp2/transport/internal.h | 9 +-- .../ext/transport/chttp2/transport/writing.c | 3 +- src/core/lib/iomgr/resource_quota.c | 3 + 8 files changed, 41 insertions(+), 62 deletions(-) diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.c b/src/core/ext/transport/chttp2/server/chttp2_server.c index 7795606e737..8ee7e29316c 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.c +++ b/src/core/ext/transport/chttp2/server/chttp2_server.c @@ -58,8 +58,8 @@ void grpc_chttp2_server_handshaker_factory_create_handshakers( grpc_chttp2_server_handshaker_factory *handshaker_factory, grpc_handshake_manager *handshake_mgr) { if (handshaker_factory != NULL) { - handshaker_factory->vtable->create_handshakers( - exec_ctx, handshaker_factory, handshake_mgr); + handshaker_factory->vtable->create_handshakers(exec_ctx, handshaker_factory, + handshake_mgr); } } @@ -71,7 +71,6 @@ void grpc_chttp2_server_handshaker_factory_destroy( } } - typedef struct pending_handshake_manager_node { grpc_handshake_manager *handshake_mgr; struct pending_handshake_manager_node *next; @@ -196,9 +195,9 @@ static void on_accept(grpc_exec_ctx *exec_ctx, void *arg, grpc_endpoint *tcp, // args instead of hard-coding it. const gpr_timespec deadline = gpr_time_add( gpr_now(GPR_CLOCK_MONOTONIC), gpr_time_from_seconds(120, GPR_TIMESPAN)); - grpc_handshake_manager_do_handshake( - exec_ctx, connection_state->handshake_mgr, tcp, state->args, deadline, - acceptor, on_handshake_done, connection_state); + grpc_handshake_manager_do_handshake(exec_ctx, connection_state->handshake_mgr, + tcp, state->args, deadline, acceptor, + on_handshake_done, connection_state); } /* Server callback: start listening on our ports */ @@ -275,9 +274,8 @@ grpc_error *grpc_chttp2_server_add_port( memset(state, 0, sizeof(*state)); grpc_closure_init(&state->tcp_server_shutdown_complete, tcp_server_shutdown_complete, state); - err = - grpc_tcp_server_create(exec_ctx, &state->tcp_server_shutdown_complete, - args, &tcp_server); + err = grpc_tcp_server_create(exec_ctx, &state->tcp_server_shutdown_complete, + args, &tcp_server); if (err != GRPC_ERROR_NONE) { goto error; } diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.h b/src/core/ext/transport/chttp2/server/chttp2_server.h index b1ff04bcbb8..30733992676 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.h +++ b/src/core/ext/transport/chttp2/server/chttp2_server.h @@ -73,7 +73,6 @@ void grpc_chttp2_server_handshaker_factory_destroy( grpc_error *grpc_chttp2_server_add_port( grpc_exec_ctx *exec_ctx, grpc_server *server, const char *addr, grpc_channel_args *args, - grpc_chttp2_server_handshaker_factory *handshaker_factory, - int *port_num); + grpc_chttp2_server_handshaker_factory *handshaker_factory, int *port_num); #endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_SERVER_CHTTP2_SERVER_H */ diff --git a/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c b/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c index 366312bd728..7e286d4e46d 100644 --- a/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c +++ b/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c @@ -45,7 +45,7 @@ int grpc_server_add_insecure_http2_port(grpc_server *server, const char *addr) { int port_num = 0; GRPC_API_TRACE("grpc_server_add_insecure_http2_port(server=%p, addr=%s)", 2, (server, addr)); - grpc_error* err = grpc_chttp2_server_add_port( + grpc_error *err = grpc_chttp2_server_add_port( &exec_ctx, server, addr, grpc_channel_args_copy(grpc_server_get_channel_args(server)), NULL /* handshaker_factory */, &port_num); diff --git a/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c b/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c index 5f417281321..85c21f0ca21 100644 --- a/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c +++ b/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c @@ -64,7 +64,7 @@ static void server_security_handshaker_factory_create_handshakers( } static void server_security_handshaker_factory_destroy( - grpc_exec_ctx* exec_ctx, grpc_chttp2_server_handshaker_factory *hf) { + grpc_exec_ctx *exec_ctx, grpc_chttp2_server_handshaker_factory *hf) { server_security_handshaker_factory *handshaker_factory = (server_security_handshaker_factory *)hf; GRPC_SECURITY_CONNECTOR_UNREF(&handshaker_factory->security_connector->base, @@ -106,8 +106,8 @@ int grpc_server_add_secure_http2_port(grpc_server *server, const char *addr, goto done; } // Create handshaker factory. - server_security_handshaker_factory* handshaker_factory = - gpr_malloc(sizeof(*handshaker_factory)); + server_security_handshaker_factory *handshaker_factory = + gpr_malloc(sizeof(*handshaker_factory)); memset(handshaker_factory, 0, sizeof(*handshaker_factory)); handshaker_factory->base.vtable = &server_security_handshaker_factory_vtable; handshaker_factory->security_connector = sc; diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 3e7c078d3ce..d4493009695 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -425,7 +425,6 @@ static void close_transport_locked(grpc_exec_ctx *exec_ctx, /* flush writable stream list to avoid dangling references */ grpc_chttp2_stream *s; while (grpc_chttp2_list_pop_writable_stream(t, &s)) { - grpc_chttp2_leave_writing_lists(exec_ctx, t, s); GRPC_CHTTP2_STREAM_UNREF(exec_ctx, s, "chttp2_writing:close"); } end_all_the_calls(exec_ctx, t, GRPC_ERROR_REF(error)); @@ -521,10 +520,6 @@ static void destroy_stream_locked(grpc_exec_ctx *exec_ctx, void *sp, } } - if (s->fail_pending_writes_on_writes_finished_error != NULL) { - GRPC_ERROR_UNREF(s->fail_pending_writes_on_writes_finished_error); - } - GPR_ASSERT(s->send_initial_metadata_finished == NULL); GPR_ASSERT(s->fetching_send_message == NULL); GPR_ASSERT(s->send_trailing_metadata_finished == NULL); @@ -826,6 +821,7 @@ static void maybe_start_some_streams(grpc_exec_ctx *exec_ctx, } #define CLOSURE_BARRIER_STATS_BIT (1 << 0) +#define CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE (1 << 1) #define CLOSURE_BARRIER_FIRST_REF_BIT (1 << 16) static grpc_closure *add_closure_barrier(grpc_closure *closure) { @@ -852,6 +848,16 @@ void grpc_chttp2_complete_closure_step(grpc_exec_ctx *exec_ctx, return; } closure->next_data.scratch -= CLOSURE_BARRIER_FIRST_REF_BIT; + if (grpc_http_trace) { + const char *errstr = grpc_error_string(error); + gpr_log(GPR_DEBUG, + "complete_closure_step: %p refs=%d flags=0x%04x desc=%s err=%s", + closure, + (int)(closure->next_data.scratch / CLOSURE_BARRIER_FIRST_REF_BIT), + (int)(closure->next_data.scratch % CLOSURE_BARRIER_FIRST_REF_BIT), + desc, errstr); + grpc_error_free_string(errstr); + } if (error != GRPC_ERROR_NONE) { if (closure->error_data.error == GRPC_ERROR_NONE) { closure->error_data.error = @@ -868,7 +874,14 @@ void grpc_chttp2_complete_closure_step(grpc_exec_ctx *exec_ctx, grpc_transport_move_stats(&s->stats, s->collecting_stats); s->collecting_stats = NULL; } - grpc_closure_run(exec_ctx, closure, closure->error_data.error); + if (t->write_state == GRPC_CHTTP2_WRITE_STATE_IDLE || + (closure->next_data.scratch & CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE) == + 0) { + grpc_closure_run(exec_ctx, closure, closure->error_data.error); + } else { + grpc_closure_list_append(&t->run_after_write, closure, + closure->error_data.error); + } } } @@ -1013,6 +1026,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, if (op->send_initial_metadata != NULL) { GPR_ASSERT(s->send_initial_metadata_finished == NULL); + on_complete->next_data.scratch |= CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE; s->send_initial_metadata_finished = add_closure_barrier(on_complete); s->send_initial_metadata = op->send_initial_metadata; const size_t metadata_size = @@ -1066,6 +1080,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, } if (op->send_message != NULL) { + on_complete->next_data.scratch |= CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE; s->fetching_send_message_finished = add_closure_barrier(op->on_complete); if (s->write_closed) { grpc_chttp2_complete_closure_step( @@ -1103,6 +1118,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, if (op->send_trailing_metadata != NULL) { GPR_ASSERT(s->send_trailing_metadata_finished == NULL); + on_complete->next_data.scratch |= CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE; s->send_trailing_metadata_finished = add_closure_barrier(on_complete); s->send_trailing_metadata = op->send_trailing_metadata; const size_t metadata_size = @@ -1406,7 +1422,6 @@ static void remove_stream(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, } } if (grpc_chttp2_list_remove_writable_stream(t, s)) { - grpc_chttp2_leave_writing_lists(exec_ctx, t, s); GRPC_CHTTP2_STREAM_UNREF(exec_ctx, s, "chttp2_writing:remove_stream"); } @@ -1537,41 +1552,9 @@ static grpc_error *removal_error(grpc_error *extra_error, grpc_chttp2_stream *s, return error; } -void grpc_chttp2_leave_writing_lists(grpc_exec_ctx *exec_ctx, - grpc_chttp2_transport *t, - grpc_chttp2_stream *s) { - if (s->need_fail_pending_writes_on_writes_finished) { - grpc_error *error = s->fail_pending_writes_on_writes_finished_error; - s->fail_pending_writes_on_writes_finished_error = NULL; - s->need_fail_pending_writes_on_writes_finished = false; - grpc_chttp2_fail_pending_writes(exec_ctx, t, s, error); - } -} - void grpc_chttp2_fail_pending_writes(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_chttp2_stream *s, grpc_error *error) { - if (s->need_fail_pending_writes_on_writes_finished || - (t->write_state != GRPC_CHTTP2_WRITE_STATE_IDLE && - (s->included[GRPC_CHTTP2_LIST_WRITABLE] || - s->included[GRPC_CHTTP2_LIST_WRITING]))) { - /* If a write is in progress, and it involves this stream, wait for the - * write to complete before cancelling things out. If we don't do this, then - * our combiner lock might think that some operation on its queue might be - * covering a completion even though there is none, in which case we might - * offload to another thread, which isn't guarateed to exist */ - if (error != GRPC_ERROR_NONE) { - if (s->fail_pending_writes_on_writes_finished_error == GRPC_ERROR_NONE) { - s->fail_pending_writes_on_writes_finished_error = GRPC_ERROR_CREATE( - "Post-poned fail writes due to in-progress write"); - } - s->fail_pending_writes_on_writes_finished_error = grpc_error_add_child( - s->fail_pending_writes_on_writes_finished_error, error); - } - s->need_fail_pending_writes_on_writes_finished = true; - return; /* early out */ - } - error = removal_error(error, s, "Pending writes failed due to stream closure"); s->send_initial_metadata = NULL; diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 6cba1e7fd20..31eb1e01ac0 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -327,6 +327,9 @@ struct grpc_chttp2_transport { */ grpc_error *close_transport_on_writes_finished; + /* a list of closures to run after writes are finished */ + grpc_closure_list run_after_write; + /* buffer pool state */ /** have we scheduled a benign cleanup? */ bool benign_reclaimer_registered; @@ -409,9 +412,6 @@ struct grpc_chttp2_stream { grpc_error *read_closed_error; /** the error that resulted in this stream being write-closed */ grpc_error *write_closed_error; - /** should any writes be cleared once this stream becomes non-writable */ - bool need_fail_pending_writes_on_writes_finished; - grpc_error *fail_pending_writes_on_writes_finished_error; grpc_published_metadata_method published_metadata[2]; bool final_metadata_requested; @@ -692,9 +692,6 @@ void grpc_chttp2_maybe_complete_recv_trailing_metadata(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_chttp2_stream *s); -void grpc_chttp2_leave_writing_lists(grpc_exec_ctx *exec_ctx, - grpc_chttp2_transport *t, - grpc_chttp2_stream *s); void grpc_chttp2_fail_pending_writes(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_chttp2_stream *s, grpc_error *error); diff --git a/src/core/ext/transport/chttp2/transport/writing.c b/src/core/ext/transport/chttp2/transport/writing.c index 769b229a0da..ddd75ee574f 100644 --- a/src/core/ext/transport/chttp2/transport/writing.c +++ b/src/core/ext/transport/chttp2/transport/writing.c @@ -208,7 +208,6 @@ bool grpc_chttp2_begin_write(grpc_exec_ctx *exec_ctx, GRPC_CHTTP2_STREAM_UNREF(exec_ctx, s, "chttp2_writing:already_writing"); } } else { - grpc_chttp2_leave_writing_lists(exec_ctx, t, s); GRPC_CHTTP2_STREAM_UNREF(exec_ctx, s, "chttp2_writing:no_write"); } } @@ -253,9 +252,9 @@ void grpc_chttp2_end_write(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, grpc_chttp2_mark_stream_closed(exec_ctx, t, s, !t->is_client, 1, GRPC_ERROR_REF(error)); } - grpc_chttp2_leave_writing_lists(exec_ctx, t, s); GRPC_CHTTP2_STREAM_UNREF(exec_ctx, s, "chttp2_writing:end"); } + grpc_exec_ctx_enqueue_list(exec_ctx, &t->run_after_write, NULL); grpc_slice_buffer_reset_and_unref(&t->outbuf); GRPC_ERROR_UNREF(error); GPR_TIMER_END("grpc_chttp2_end_write", 0); diff --git a/src/core/lib/iomgr/resource_quota.c b/src/core/lib/iomgr/resource_quota.c index 16392020f46..cc1840d36b2 100644 --- a/src/core/lib/iomgr/resource_quota.c +++ b/src/core/lib/iomgr/resource_quota.c @@ -231,6 +231,7 @@ static void rulist_remove(grpc_resource_user *resource_user, grpc_rulist list) { resource_user->links[list].prev; resource_user->links[list].prev->links[list].next = resource_user->links[list].next; + resource_user->links[list].next = resource_user->links[list].prev = NULL; } /******************************************************************************* @@ -703,6 +704,8 @@ static void ru_unref_by(grpc_exec_ctx *exec_ctx, grpc_resource_user *resource_user, gpr_atm amount) { GPR_ASSERT(amount > 0); gpr_atm old = gpr_atm_full_fetch_add(&resource_user->refs, -amount); + gpr_log(GPR_DEBUG, "%p unref_by %d, old=%d", resource_user, (int)amount, + (int)old); GPR_ASSERT(old >= amount); if (old == amount) { grpc_combiner_execute(exec_ctx, resource_user->resource_quota->combiner, From 1af5401e734d9dd12845b50951f199d6b4caaa92 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 2 Dec 2016 17:17:33 -0800 Subject: [PATCH 16/51] Fix spam --- src/core/lib/iomgr/resource_quota.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/core/lib/iomgr/resource_quota.c b/src/core/lib/iomgr/resource_quota.c index cc1840d36b2..213d29600c9 100644 --- a/src/core/lib/iomgr/resource_quota.c +++ b/src/core/lib/iomgr/resource_quota.c @@ -704,8 +704,6 @@ static void ru_unref_by(grpc_exec_ctx *exec_ctx, grpc_resource_user *resource_user, gpr_atm amount) { GPR_ASSERT(amount > 0); gpr_atm old = gpr_atm_full_fetch_add(&resource_user->refs, -amount); - gpr_log(GPR_DEBUG, "%p unref_by %d, old=%d", resource_user, (int)amount, - (int)old); GPR_ASSERT(old >= amount); if (old == amount) { grpc_combiner_execute(exec_ctx, resource_user->resource_quota->combiner, From 7fa08e24b0d46090cc6c9bb09bef940805f9daff Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sun, 4 Dec 2016 22:11:57 -0800 Subject: [PATCH 17/51] Fix data race with atomic for interop c++ --- test/cpp/interop/interop_server.cc | 2 +- test/cpp/interop/interop_server_bootstrap.cc | 4 ++-- test/cpp/interop/server_helper.h | 6 ++++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/test/cpp/interop/interop_server.cc b/test/cpp/interop/interop_server.cc index 8b50ae8c05a..67456ce18bb 100644 --- a/test/cpp/interop/interop_server.cc +++ b/test/cpp/interop/interop_server.cc @@ -344,7 +344,7 @@ void grpc::testing::interop::RunServer( } std::unique_ptr server(builder.BuildAndStart()); gpr_log(GPR_INFO, "Server listening on %s", server_address.str().c_str()); - while (!g_got_sigint) { + while (!gpr_atm_no_barrier_load(&g_got_sigint)) { sleep(5); } } diff --git a/test/cpp/interop/interop_server_bootstrap.cc b/test/cpp/interop/interop_server_bootstrap.cc index 424f7ca7f0d..99518c6943f 100644 --- a/test/cpp/interop/interop_server_bootstrap.cc +++ b/test/cpp/interop/interop_server_bootstrap.cc @@ -37,10 +37,10 @@ #include "test/cpp/interop/server_helper.h" #include "test/cpp/util/test_config.h" -bool grpc::testing::interop::g_got_sigint = false; +gpr_atm grpc::testing::interop::g_got_sigint; static void sigint_handler(int x) { - grpc::testing::interop::g_got_sigint = true; + gpr_atm_no_barrier_store(&grpc::testing::interop::g_got_sigint, true); } int main(int argc, char** argv) { diff --git a/test/cpp/interop/server_helper.h b/test/cpp/interop/server_helper.h index fc4ea8b3e81..99539adee55 100644 --- a/test/cpp/interop/server_helper.h +++ b/test/cpp/interop/server_helper.h @@ -36,9 +36,11 @@ #include +#include +#include + #include #include -#include namespace grpc { namespace testing { @@ -62,7 +64,7 @@ class InteropServerContextInspector { namespace interop { -extern bool g_got_sigint; +extern gpr_atm g_got_sigint; void RunServer(std::shared_ptr creds); } // namespace interop From 17db9022f153bdd79b4201d9841fe6c44d824e29 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 5 Dec 2016 11:38:01 +0100 Subject: [PATCH 18/51] add grpc_fuzzer_client config --- .../internal_ci/linux/grpc_fuzzer_client.cfg | 34 ++++++++++++++++ tools/internal_ci/linux/grpc_fuzzer_client.sh | 40 +++++++++++++++++++ tools/internal_ci/linux/grpc_master.cfg | 2 +- .../linux/{run_tests.sh => grpc_master.sh} | 0 4 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 tools/internal_ci/linux/grpc_fuzzer_client.cfg create mode 100755 tools/internal_ci/linux/grpc_fuzzer_client.sh rename tools/internal_ci/linux/{run_tests.sh => grpc_master.sh} (100%) diff --git a/tools/internal_ci/linux/grpc_fuzzer_client.cfg b/tools/internal_ci/linux/grpc_fuzzer_client.cfg new file mode 100644 index 00000000000..8c3c7878d30 --- /dev/null +++ b/tools/internal_ci/linux/grpc_fuzzer_client.cfg @@ -0,0 +1,34 @@ +#!/bin/bash +# Copyright 2016, Google Inc. +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +# Config file for the internal CI (in protobuf text format) + +# Location of the continuous shell script in repository. +build_file: "grpc/tools/internal_ci/linux/grpc_fuzzer_client.sh" diff --git a/tools/internal_ci/linux/grpc_fuzzer_client.sh b/tools/internal_ci/linux/grpc_fuzzer_client.sh new file mode 100755 index 00000000000..81fd4c4114e --- /dev/null +++ b/tools/internal_ci/linux/grpc_fuzzer_client.sh @@ -0,0 +1,40 @@ +#!/bin/bash +# Copyright 2016, Google Inc. +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +set -ex + +# change to grpc repo root +cd $(dirname $0)/../../.. + +git submodule update --init + +# download fuzzer docker image from dockerhub +export DOCKERHUB_ORGANIZATION=grpctesting +config=asan-trace-cmp runtime=3600 tools/jenkins/run_fuzzer.sh client_fuzzer diff --git a/tools/internal_ci/linux/grpc_master.cfg b/tools/internal_ci/linux/grpc_master.cfg index 1f816600780..acaba608aa2 100644 --- a/tools/internal_ci/linux/grpc_master.cfg +++ b/tools/internal_ci/linux/grpc_master.cfg @@ -31,4 +31,4 @@ # Config file for the internal CI (in protobuf text format) # Location of the continuous shell script in repository. -build_file: "grpc/tools/internal_ci/linux/run_tests.sh" +build_file: "grpc/tools/internal_ci/linux/grpc_master.sh" diff --git a/tools/internal_ci/linux/run_tests.sh b/tools/internal_ci/linux/grpc_master.sh similarity index 100% rename from tools/internal_ci/linux/run_tests.sh rename to tools/internal_ci/linux/grpc_master.sh From a8ac4a21a0424bb57b78cd046c1b6e72466877fd Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 5 Dec 2016 12:21:17 +0100 Subject: [PATCH 19/51] fuzzer settings --- tools/internal_ci/linux/grpc_fuzzer_client.cfg | 1 + tools/internal_ci/linux/grpc_fuzzer_client.sh | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/internal_ci/linux/grpc_fuzzer_client.cfg b/tools/internal_ci/linux/grpc_fuzzer_client.cfg index 8c3c7878d30..557dc0b390d 100644 --- a/tools/internal_ci/linux/grpc_fuzzer_client.cfg +++ b/tools/internal_ci/linux/grpc_fuzzer_client.cfg @@ -32,3 +32,4 @@ # Location of the continuous shell script in repository. build_file: "grpc/tools/internal_ci/linux/grpc_fuzzer_client.sh" +timeout_mins: 1440 # 24 hours is the maximum allowed value diff --git a/tools/internal_ci/linux/grpc_fuzzer_client.sh b/tools/internal_ci/linux/grpc_fuzzer_client.sh index 81fd4c4114e..f9ff13d303e 100755 --- a/tools/internal_ci/linux/grpc_fuzzer_client.sh +++ b/tools/internal_ci/linux/grpc_fuzzer_client.sh @@ -37,4 +37,5 @@ git submodule update --init # download fuzzer docker image from dockerhub export DOCKERHUB_ORGANIZATION=grpctesting -config=asan-trace-cmp runtime=3600 tools/jenkins/run_fuzzer.sh client_fuzzer +# runtime 23 * 60 mins +config=asan-trace-cmp runtime=82800 tools/jenkins/run_fuzzer.sh client_fuzzer From 2c1801ff94eae33999f78c29c77c704acb252bc8 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 5 Dec 2016 12:58:14 +0100 Subject: [PATCH 20/51] dont just build C core --- tools/internal_ci/linux/grpc_master.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/internal_ci/linux/grpc_master.sh b/tools/internal_ci/linux/grpc_master.sh index be477c1271d..c706b1c7460 100755 --- a/tools/internal_ci/linux/grpc_master.sh +++ b/tools/internal_ci/linux/grpc_master.sh @@ -42,4 +42,12 @@ docker --version || true git submodule update --init -tools/run_tests/run_tests.py -l c --build_only +tools/run_tests/run_tests.py -l c || FAILED="true" + +# kill port_server.py to prevent the build from hanging +ps aux | grep port_server\\.py | awk '{print $2}' | xargs kill -9 + +if [ "$FAILED" != "" ] +then + exit 1 +fi \ No newline at end of file From d4c33b9bfe2914ca220346861b15b96582e1262d Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 5 Dec 2016 13:01:38 +0100 Subject: [PATCH 21/51] increase master timeout --- tools/internal_ci/linux/grpc_master.cfg | 1 + tools/internal_ci/linux/grpc_master.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/internal_ci/linux/grpc_master.cfg b/tools/internal_ci/linux/grpc_master.cfg index acaba608aa2..3addb47bbd9 100644 --- a/tools/internal_ci/linux/grpc_master.cfg +++ b/tools/internal_ci/linux/grpc_master.cfg @@ -32,3 +32,4 @@ # Location of the continuous shell script in repository. build_file: "grpc/tools/internal_ci/linux/grpc_master.sh" +timeout_mins: 60 diff --git a/tools/internal_ci/linux/grpc_master.sh b/tools/internal_ci/linux/grpc_master.sh index c706b1c7460..6c7e2c2cd94 100755 --- a/tools/internal_ci/linux/grpc_master.sh +++ b/tools/internal_ci/linux/grpc_master.sh @@ -50,4 +50,4 @@ ps aux | grep port_server\\.py | awk '{print $2}' | xargs kill -9 if [ "$FAILED" != "" ] then exit 1 -fi \ No newline at end of file +fi From c939446a74f93d3e93b9e7804653f30d68600e75 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 5 Dec 2016 13:26:36 +0100 Subject: [PATCH 22/51] artifact uploading and test report --- tools/internal_ci/linux/grpc_fuzzer_client.cfg | 5 +++++ tools/internal_ci/linux/grpc_master.cfg | 5 +++++ tools/internal_ci/linux/grpc_master.sh | 2 +- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/tools/internal_ci/linux/grpc_fuzzer_client.cfg b/tools/internal_ci/linux/grpc_fuzzer_client.cfg index 557dc0b390d..8251ff8ef29 100644 --- a/tools/internal_ci/linux/grpc_fuzzer_client.cfg +++ b/tools/internal_ci/linux/grpc_fuzzer_client.cfg @@ -33,3 +33,8 @@ # Location of the continuous shell script in repository. build_file: "grpc/tools/internal_ci/linux/grpc_fuzzer_client.sh" timeout_mins: 1440 # 24 hours is the maximum allowed value +action { + define_artifacts { + regex: "grpc/fuzzer_output/**" + } +} diff --git a/tools/internal_ci/linux/grpc_master.cfg b/tools/internal_ci/linux/grpc_master.cfg index 3addb47bbd9..d2edbdab930 100644 --- a/tools/internal_ci/linux/grpc_master.cfg +++ b/tools/internal_ci/linux/grpc_master.cfg @@ -33,3 +33,8 @@ # Location of the continuous shell script in repository. build_file: "grpc/tools/internal_ci/linux/grpc_master.sh" timeout_mins: 60 +action { + define_artifacts { + regex: "grpc/*sponge_log.xml" + } +} diff --git a/tools/internal_ci/linux/grpc_master.sh b/tools/internal_ci/linux/grpc_master.sh index 6c7e2c2cd94..ea77d113058 100755 --- a/tools/internal_ci/linux/grpc_master.sh +++ b/tools/internal_ci/linux/grpc_master.sh @@ -42,7 +42,7 @@ docker --version || true git submodule update --init -tools/run_tests/run_tests.py -l c || FAILED="true" +tools/run_tests/run_tests.py -l c -t -x sponge_log.xml || FAILED="true" # kill port_server.py to prevent the build from hanging ps aux | grep port_server\\.py | awk '{print $2}' | xargs kill -9 From 3e6bd8564e575a48a76065e6ab2fe3eaa61485bc Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 5 Dec 2016 13:49:39 +0100 Subject: [PATCH 23/51] fixup --- tools/internal_ci/linux/grpc_master.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/internal_ci/linux/grpc_master.cfg b/tools/internal_ci/linux/grpc_master.cfg index d2edbdab930..8ce2ef11a20 100644 --- a/tools/internal_ci/linux/grpc_master.cfg +++ b/tools/internal_ci/linux/grpc_master.cfg @@ -35,6 +35,6 @@ build_file: "grpc/tools/internal_ci/linux/grpc_master.sh" timeout_mins: 60 action { define_artifacts { - regex: "grpc/*sponge_log.xml" + regex: "**/sponge_log.xml" } } From 90741ec95b5514f607a7a431747727e6cb11b5db Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Mon, 5 Dec 2016 14:09:09 +0100 Subject: [PATCH 24/51] fixup --- tools/internal_ci/linux/grpc_fuzzer_client.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/internal_ci/linux/grpc_fuzzer_client.cfg b/tools/internal_ci/linux/grpc_fuzzer_client.cfg index 8251ff8ef29..b1bce022823 100644 --- a/tools/internal_ci/linux/grpc_fuzzer_client.cfg +++ b/tools/internal_ci/linux/grpc_fuzzer_client.cfg @@ -35,6 +35,6 @@ build_file: "grpc/tools/internal_ci/linux/grpc_fuzzer_client.sh" timeout_mins: 1440 # 24 hours is the maximum allowed value action { define_artifacts { - regex: "grpc/fuzzer_output/**" + regex: "git/grpc/fuzzer_output/**" } } From 73122ba8f56b94252b5e550c74657f26d8257e6d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 5 Dec 2016 08:16:58 -0800 Subject: [PATCH 25/51] clang-format --- .../ext/transport/chttp2/server/chttp2_server.c | 16 +++++++--------- .../ext/transport/chttp2/server/chttp2_server.h | 3 +-- .../chttp2/server/insecure/server_chttp2.c | 2 +- .../chttp2/server/secure/server_secure_chttp2.c | 6 +++--- 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.c b/src/core/ext/transport/chttp2/server/chttp2_server.c index 7795606e737..8ee7e29316c 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.c +++ b/src/core/ext/transport/chttp2/server/chttp2_server.c @@ -58,8 +58,8 @@ void grpc_chttp2_server_handshaker_factory_create_handshakers( grpc_chttp2_server_handshaker_factory *handshaker_factory, grpc_handshake_manager *handshake_mgr) { if (handshaker_factory != NULL) { - handshaker_factory->vtable->create_handshakers( - exec_ctx, handshaker_factory, handshake_mgr); + handshaker_factory->vtable->create_handshakers(exec_ctx, handshaker_factory, + handshake_mgr); } } @@ -71,7 +71,6 @@ void grpc_chttp2_server_handshaker_factory_destroy( } } - typedef struct pending_handshake_manager_node { grpc_handshake_manager *handshake_mgr; struct pending_handshake_manager_node *next; @@ -196,9 +195,9 @@ static void on_accept(grpc_exec_ctx *exec_ctx, void *arg, grpc_endpoint *tcp, // args instead of hard-coding it. const gpr_timespec deadline = gpr_time_add( gpr_now(GPR_CLOCK_MONOTONIC), gpr_time_from_seconds(120, GPR_TIMESPAN)); - grpc_handshake_manager_do_handshake( - exec_ctx, connection_state->handshake_mgr, tcp, state->args, deadline, - acceptor, on_handshake_done, connection_state); + grpc_handshake_manager_do_handshake(exec_ctx, connection_state->handshake_mgr, + tcp, state->args, deadline, acceptor, + on_handshake_done, connection_state); } /* Server callback: start listening on our ports */ @@ -275,9 +274,8 @@ grpc_error *grpc_chttp2_server_add_port( memset(state, 0, sizeof(*state)); grpc_closure_init(&state->tcp_server_shutdown_complete, tcp_server_shutdown_complete, state); - err = - grpc_tcp_server_create(exec_ctx, &state->tcp_server_shutdown_complete, - args, &tcp_server); + err = grpc_tcp_server_create(exec_ctx, &state->tcp_server_shutdown_complete, + args, &tcp_server); if (err != GRPC_ERROR_NONE) { goto error; } diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.h b/src/core/ext/transport/chttp2/server/chttp2_server.h index b1ff04bcbb8..30733992676 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.h +++ b/src/core/ext/transport/chttp2/server/chttp2_server.h @@ -73,7 +73,6 @@ void grpc_chttp2_server_handshaker_factory_destroy( grpc_error *grpc_chttp2_server_add_port( grpc_exec_ctx *exec_ctx, grpc_server *server, const char *addr, grpc_channel_args *args, - grpc_chttp2_server_handshaker_factory *handshaker_factory, - int *port_num); + grpc_chttp2_server_handshaker_factory *handshaker_factory, int *port_num); #endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_SERVER_CHTTP2_SERVER_H */ diff --git a/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c b/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c index 366312bd728..7e286d4e46d 100644 --- a/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c +++ b/src/core/ext/transport/chttp2/server/insecure/server_chttp2.c @@ -45,7 +45,7 @@ int grpc_server_add_insecure_http2_port(grpc_server *server, const char *addr) { int port_num = 0; GRPC_API_TRACE("grpc_server_add_insecure_http2_port(server=%p, addr=%s)", 2, (server, addr)); - grpc_error* err = grpc_chttp2_server_add_port( + grpc_error *err = grpc_chttp2_server_add_port( &exec_ctx, server, addr, grpc_channel_args_copy(grpc_server_get_channel_args(server)), NULL /* handshaker_factory */, &port_num); diff --git a/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c b/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c index 5f417281321..85c21f0ca21 100644 --- a/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c +++ b/src/core/ext/transport/chttp2/server/secure/server_secure_chttp2.c @@ -64,7 +64,7 @@ static void server_security_handshaker_factory_create_handshakers( } static void server_security_handshaker_factory_destroy( - grpc_exec_ctx* exec_ctx, grpc_chttp2_server_handshaker_factory *hf) { + grpc_exec_ctx *exec_ctx, grpc_chttp2_server_handshaker_factory *hf) { server_security_handshaker_factory *handshaker_factory = (server_security_handshaker_factory *)hf; GRPC_SECURITY_CONNECTOR_UNREF(&handshaker_factory->security_connector->base, @@ -106,8 +106,8 @@ int grpc_server_add_secure_http2_port(grpc_server *server, const char *addr, goto done; } // Create handshaker factory. - server_security_handshaker_factory* handshaker_factory = - gpr_malloc(sizeof(*handshaker_factory)); + server_security_handshaker_factory *handshaker_factory = + gpr_malloc(sizeof(*handshaker_factory)); memset(handshaker_factory, 0, sizeof(*handshaker_factory)); handshaker_factory->base.vtable = &server_security_handshaker_factory_vtable; handshaker_factory->security_connector = sc; From 909935087b7875dd46507cb87934911edfb507db Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 5 Dec 2016 08:32:53 -0800 Subject: [PATCH 26/51] Get idle edge reliably --- .../transport/chttp2/transport/chttp2_transport.c | 12 +++++++----- src/core/ext/transport/chttp2/transport/writing.c | 1 - 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index d4493009695..22e63d7682b 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -599,11 +599,13 @@ static void set_write_state(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, write_state_name(t->write_state), write_state_name(st), reason)); t->write_state = st; - if (st == GRPC_CHTTP2_WRITE_STATE_IDLE && - t->close_transport_on_writes_finished != NULL) { - grpc_error *err = t->close_transport_on_writes_finished; - t->close_transport_on_writes_finished = NULL; - close_transport_locked(exec_ctx, t, err); + if (st == GRPC_CHTTP2_WRITE_STATE_IDLE) { + grpc_exec_ctx_enqueue_list(exec_ctx, &t->run_after_write, NULL); + if (t->close_transport_on_writes_finished != NULL) { + grpc_error *err = t->close_transport_on_writes_finished; + t->close_transport_on_writes_finished = NULL; + close_transport_locked(exec_ctx, t, err); + } } } diff --git a/src/core/ext/transport/chttp2/transport/writing.c b/src/core/ext/transport/chttp2/transport/writing.c index ddd75ee574f..139e7387c4d 100644 --- a/src/core/ext/transport/chttp2/transport/writing.c +++ b/src/core/ext/transport/chttp2/transport/writing.c @@ -254,7 +254,6 @@ void grpc_chttp2_end_write(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, } GRPC_CHTTP2_STREAM_UNREF(exec_ctx, s, "chttp2_writing:end"); } - grpc_exec_ctx_enqueue_list(exec_ctx, &t->run_after_write, NULL); grpc_slice_buffer_reset_and_unref(&t->outbuf); GRPC_ERROR_UNREF(error); GPR_TIMER_END("grpc_chttp2_end_write", 0); From d907370fc82a7991a24bb0dcbaf962acd319428a Mon Sep 17 00:00:00 2001 From: Paul Querna Date: Thu, 17 Nov 2016 23:16:41 -0800 Subject: [PATCH 27/51] GPR Buffers need to be destroyed, not directly freed, otherwise it leaks reference counted backing slices. --- src/objective-c/GRPCClient/private/GRPCWrappedCall.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m index 627b6aa86dd..38fcae0299d 100644 --- a/src/objective-c/GRPCClient/private/GRPCWrappedCall.m +++ b/src/objective-c/GRPCClient/private/GRPCWrappedCall.m @@ -112,7 +112,7 @@ } - (void)dealloc { - gpr_free(_op.data.send_message); + grpc_byte_buffer_destroy(_op.data.send_message); } @end From fbcd9a7344098a7c8b7c1e1aea8c7414caf91ece Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 5 Dec 2016 08:47:02 -0800 Subject: [PATCH 28/51] Update server_crash_test to account for longer reconnect timeouts --- test/cpp/end2end/server_crash_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cpp/end2end/server_crash_test.cc b/test/cpp/end2end/server_crash_test.cc index 8cee1403dcf..b1f9216055f 100644 --- a/test/cpp/end2end/server_crash_test.cc +++ b/test/cpp/end2end/server_crash_test.cc @@ -138,7 +138,7 @@ TEST_F(CrashTest, ResponseStream) { auto server = CreateServerAndClient("response"); gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), - gpr_time_from_seconds(5, GPR_TIMESPAN))); + gpr_time_from_seconds(60, GPR_TIMESPAN))); KillClient(); server->Shutdown(); GPR_ASSERT(HadOneResponseStream()); @@ -148,7 +148,7 @@ TEST_F(CrashTest, BidiStream) { auto server = CreateServerAndClient("bidi"); gpr_sleep_until(gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), - gpr_time_from_seconds(5, GPR_TIMESPAN))); + gpr_time_from_seconds(60, GPR_TIMESPAN))); KillClient(); server->Shutdown(); GPR_ASSERT(HadOneBidiStream()); From bf25e240bccf5b4622fa59eb66cdb6263290969f Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 5 Dec 2016 09:39:01 -0800 Subject: [PATCH 29/51] Fix test name conflict in import. Also use low-thread-count variant for asan. --- test/cpp/qps/gen_build_yaml.py | 6 +- tools/run_tests/tests.json | 204 ++++++++++++++++----------------- 2 files changed, 105 insertions(+), 105 deletions(-) diff --git a/test/cpp/qps/gen_build_yaml.py b/test/cpp/qps/gen_build_yaml.py index 4aa58d2737c..188d6196e5a 100755 --- a/test/cpp/qps/gen_build_yaml.py +++ b/test/cpp/qps/gen_build_yaml.py @@ -91,7 +91,7 @@ print yaml.dump({ 'boringssl': True, 'defaults': 'boringssl', 'cpu_cost': guess_cpu(scenario_json, False), - 'exclude_configs': ['tsan'], + 'exclude_configs': ['tsan', 'asan'], 'timeout_seconds': 6*60 } for scenario_json in scenario_config.CXXLanguage().scenarios() @@ -99,7 +99,7 @@ print yaml.dump({ ] + [ { 'name': 'json_run_localhost', - 'shortname': 'json_run_localhost:%s' % scenario_json['name'], + 'shortname': 'json_run_localhost:%s_low_thread_count' % scenario_json['name'], 'args': ['--scenarios_json', _scenario_json_string(scenario_json, True)], 'ci_platforms': ['linux'], 'platforms': ['linux'], @@ -108,7 +108,7 @@ print yaml.dump({ 'boringssl': True, 'defaults': 'boringssl', 'cpu_cost': guess_cpu(scenario_json, True), - 'exclude_configs': sorted(c for c in configs_from_yaml if c != 'tsan'), + 'exclude_configs': sorted(c for c in configs_from_yaml if c not in ('tsan', 'asan')), 'timeout_seconds': 6*60 } for scenario_json in scenario_config.CXXLanguage().scenarios() diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index c4bfd0a9a74..b76263b8b98 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -36765,7 +36765,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36788,7 +36789,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36811,7 +36813,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36834,7 +36837,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36857,7 +36861,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36880,7 +36885,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36903,7 +36909,8 @@ "cpu_cost": 1024, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36926,7 +36933,8 @@ "cpu_cost": 1024, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36949,7 +36957,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36972,7 +36981,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -36995,7 +37005,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37018,7 +37029,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37041,7 +37053,8 @@ "cpu_cost": 1024, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37064,7 +37077,8 @@ "cpu_cost": 1024, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37087,7 +37101,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37110,7 +37125,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37133,7 +37149,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37156,7 +37173,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37179,7 +37197,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37202,7 +37221,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37225,7 +37245,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37248,7 +37269,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37271,7 +37293,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37294,7 +37317,8 @@ "cpu_cost": 1024, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37317,7 +37341,8 @@ "cpu_cost": 1024, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37340,7 +37365,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37363,7 +37389,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37386,7 +37413,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37409,7 +37437,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37432,7 +37461,8 @@ "cpu_cost": 1024, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37455,7 +37485,8 @@ "cpu_cost": 1024, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37478,7 +37509,8 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37501,7 +37533,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37524,7 +37557,8 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "tsan" + "tsan", + "asan" ], "flaky": false, "language": "c++", @@ -37547,7 +37581,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37570,7 +37603,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_generic_async_streaming_ping_pong_secure", + "shortname": "json_run_localhost:cpp_generic_async_streaming_ping_pong_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37585,7 +37618,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37608,7 +37640,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_generic_async_streaming_qps_unconstrained_secure", + "shortname": "json_run_localhost:cpp_generic_async_streaming_qps_unconstrained_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37623,7 +37655,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37646,7 +37677,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_generic_async_streaming_qps_one_server_core_secure", + "shortname": "json_run_localhost:cpp_generic_async_streaming_qps_one_server_core_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37661,7 +37692,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37684,7 +37714,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_client_sync_server_unary_qps_unconstrained_secure", + "shortname": "json_run_localhost:cpp_protobuf_async_client_sync_server_unary_qps_unconstrained_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37699,7 +37729,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37722,7 +37751,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_client_sync_server_streaming_qps_unconstrained_secure", + "shortname": "json_run_localhost:cpp_protobuf_async_client_sync_server_streaming_qps_unconstrained_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37737,7 +37766,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37760,7 +37788,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_unary_ping_pong_secure", + "shortname": "json_run_localhost:cpp_protobuf_sync_unary_ping_pong_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37775,7 +37803,6 @@ "cpu_cost": 64, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37798,7 +37825,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_unary_qps_unconstrained_secure", + "shortname": "json_run_localhost:cpp_protobuf_sync_unary_qps_unconstrained_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37813,7 +37840,6 @@ "cpu_cost": 64, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37836,7 +37862,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_unary_qps_unconstrained_secure_500kib_resource_quota", + "shortname": "json_run_localhost:cpp_protobuf_sync_unary_qps_unconstrained_secure_500kib_resource_quota_low_thread_count", "timeout_seconds": 360 }, { @@ -37851,7 +37877,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37874,7 +37899,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_unary_ping_pong_secure", + "shortname": "json_run_localhost:cpp_protobuf_async_unary_ping_pong_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37889,7 +37914,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37912,7 +37936,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_unary_qps_unconstrained_secure", + "shortname": "json_run_localhost:cpp_protobuf_async_unary_qps_unconstrained_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -37927,7 +37951,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37950,7 +37973,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_unary_qps_unconstrained_secure_500kib_resource_quota", + "shortname": "json_run_localhost:cpp_protobuf_async_unary_qps_unconstrained_secure_500kib_resource_quota_low_thread_count", "timeout_seconds": 360 }, { @@ -37965,7 +37988,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -37988,7 +38010,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_ping_pong_secure", + "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_ping_pong_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -38003,7 +38025,6 @@ "cpu_cost": 64, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38026,7 +38047,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_qps_unconstrained_secure", + "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_qps_unconstrained_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -38041,7 +38062,6 @@ "cpu_cost": 64, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38064,7 +38084,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_qps_unconstrained_secure_500kib_resource_quota", + "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_qps_unconstrained_secure_500kib_resource_quota_low_thread_count", "timeout_seconds": 360 }, { @@ -38079,7 +38099,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38102,7 +38121,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_streaming_ping_pong_secure", + "shortname": "json_run_localhost:cpp_protobuf_async_streaming_ping_pong_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -38117,7 +38136,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38140,7 +38158,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_streaming_qps_unconstrained_secure", + "shortname": "json_run_localhost:cpp_protobuf_async_streaming_qps_unconstrained_secure_low_thread_count", "timeout_seconds": 360 }, { @@ -38155,7 +38173,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38178,7 +38195,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_streaming_qps_unconstrained_secure_500kib_resource_quota", + "shortname": "json_run_localhost:cpp_protobuf_async_streaming_qps_unconstrained_secure_500kib_resource_quota_low_thread_count", "timeout_seconds": 360 }, { @@ -38193,7 +38210,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38216,7 +38232,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_generic_async_streaming_ping_pong_insecure", + "shortname": "json_run_localhost:cpp_generic_async_streaming_ping_pong_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38231,7 +38247,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38254,7 +38269,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_generic_async_streaming_qps_unconstrained_insecure", + "shortname": "json_run_localhost:cpp_generic_async_streaming_qps_unconstrained_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38269,7 +38284,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38292,7 +38306,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_generic_async_streaming_qps_one_server_core_insecure", + "shortname": "json_run_localhost:cpp_generic_async_streaming_qps_one_server_core_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38307,7 +38321,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38330,7 +38343,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_client_sync_server_unary_qps_unconstrained_insecure", + "shortname": "json_run_localhost:cpp_protobuf_async_client_sync_server_unary_qps_unconstrained_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38345,7 +38358,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38368,7 +38380,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_client_sync_server_streaming_qps_unconstrained_insecure", + "shortname": "json_run_localhost:cpp_protobuf_async_client_sync_server_streaming_qps_unconstrained_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38383,7 +38395,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38406,7 +38417,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_unary_ping_pong_insecure", + "shortname": "json_run_localhost:cpp_protobuf_sync_unary_ping_pong_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38421,7 +38432,6 @@ "cpu_cost": 64, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38444,7 +38454,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_unary_qps_unconstrained_insecure", + "shortname": "json_run_localhost:cpp_protobuf_sync_unary_qps_unconstrained_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38459,7 +38469,6 @@ "cpu_cost": 64, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38482,7 +38491,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_unary_qps_unconstrained_insecure_500kib_resource_quota", + "shortname": "json_run_localhost:cpp_protobuf_sync_unary_qps_unconstrained_insecure_500kib_resource_quota_low_thread_count", "timeout_seconds": 360 }, { @@ -38497,7 +38506,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38520,7 +38528,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_unary_ping_pong_insecure", + "shortname": "json_run_localhost:cpp_protobuf_async_unary_ping_pong_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38535,7 +38543,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38558,7 +38565,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_unary_qps_unconstrained_insecure", + "shortname": "json_run_localhost:cpp_protobuf_async_unary_qps_unconstrained_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38573,7 +38580,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38596,7 +38602,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_unary_qps_unconstrained_insecure_500kib_resource_quota", + "shortname": "json_run_localhost:cpp_protobuf_async_unary_qps_unconstrained_insecure_500kib_resource_quota_low_thread_count", "timeout_seconds": 360 }, { @@ -38611,7 +38617,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38634,7 +38639,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_ping_pong_insecure", + "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_ping_pong_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38649,7 +38654,6 @@ "cpu_cost": 64, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38672,7 +38676,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_qps_unconstrained_insecure", + "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_qps_unconstrained_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38687,7 +38691,6 @@ "cpu_cost": 64, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38710,7 +38713,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_qps_unconstrained_insecure_500kib_resource_quota", + "shortname": "json_run_localhost:cpp_protobuf_sync_streaming_qps_unconstrained_insecure_500kib_resource_quota_low_thread_count", "timeout_seconds": 360 }, { @@ -38725,7 +38728,6 @@ "cpu_cost": 2, "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38748,7 +38750,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_streaming_ping_pong_insecure", + "shortname": "json_run_localhost:cpp_protobuf_async_streaming_ping_pong_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38763,7 +38765,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38786,7 +38787,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_streaming_qps_unconstrained_insecure", + "shortname": "json_run_localhost:cpp_protobuf_async_streaming_qps_unconstrained_insecure_low_thread_count", "timeout_seconds": 360 }, { @@ -38801,7 +38802,6 @@ "cpu_cost": "capacity", "defaults": "boringssl", "exclude_configs": [ - "asan", "asan-noleaks", "asan-trace-cmp", "basicprof", @@ -38824,7 +38824,7 @@ "platforms": [ "linux" ], - "shortname": "json_run_localhost:cpp_protobuf_async_streaming_qps_unconstrained_insecure_500kib_resource_quota", + "shortname": "json_run_localhost:cpp_protobuf_async_streaming_qps_unconstrained_insecure_500kib_resource_quota_low_thread_count", "timeout_seconds": 360 }, { From 504ae4347bee3589f6695dff4e28a1d7e40b8f59 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 5 Dec 2016 11:07:58 -0800 Subject: [PATCH 30/51] Add parens --- src/core/ext/transport/chttp2/transport/chttp2_transport.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 22e63d7682b..f995d2180eb 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -876,9 +876,9 @@ void grpc_chttp2_complete_closure_step(grpc_exec_ctx *exec_ctx, grpc_transport_move_stats(&s->stats, s->collecting_stats); s->collecting_stats = NULL; } - if (t->write_state == GRPC_CHTTP2_WRITE_STATE_IDLE || - (closure->next_data.scratch & CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE) == - 0) { + if ((t->write_state == GRPC_CHTTP2_WRITE_STATE_IDLE) || + ((closure->next_data.scratch & CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE) == + 0)) { grpc_closure_run(exec_ctx, closure, closure->error_data.error); } else { grpc_closure_list_append(&t->run_after_write, closure, From 73c4f8fce1eec86638d989e395ca2f004024b33d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 5 Dec 2016 11:11:59 -0800 Subject: [PATCH 31/51] Readability improvements --- .../chttp2/transport/chttp2_transport.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index f995d2180eb..3b84898fee0 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -822,8 +822,14 @@ static void maybe_start_some_streams(grpc_exec_ctx *exec_ctx, } } +/* Flag that this closure barrier wants stats to be updated before finishing */ #define CLOSURE_BARRIER_STATS_BIT (1 << 0) -#define CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE (1 << 1) +/* Flag that this closure barrier may be covering a write in a pollset, and so + we should not complete this closure until we can prove that the write got + scheduled */ +#define CLOSURE_BARRIER_MAY_COVER_WRITE (1 << 1) +/* First bit of the reference count, stored in the high order bits (with the low + bits being used for flags defined above) */ #define CLOSURE_BARRIER_FIRST_REF_BIT (1 << 16) static grpc_closure *add_closure_barrier(grpc_closure *closure) { @@ -877,8 +883,7 @@ void grpc_chttp2_complete_closure_step(grpc_exec_ctx *exec_ctx, s->collecting_stats = NULL; } if ((t->write_state == GRPC_CHTTP2_WRITE_STATE_IDLE) || - ((closure->next_data.scratch & CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE) == - 0)) { + !(closure->next_data.scratch & CLOSURE_BARRIER_MAY_COVER_WRITE)) { grpc_closure_run(exec_ctx, closure, closure->error_data.error); } else { grpc_closure_list_append(&t->run_after_write, closure, @@ -1028,7 +1033,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, if (op->send_initial_metadata != NULL) { GPR_ASSERT(s->send_initial_metadata_finished == NULL); - on_complete->next_data.scratch |= CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE; + on_complete->next_data.scratch |= CLOSURE_BARRIER_MAY_COVER_WRITE; s->send_initial_metadata_finished = add_closure_barrier(on_complete); s->send_initial_metadata = op->send_initial_metadata; const size_t metadata_size = @@ -1082,7 +1087,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, } if (op->send_message != NULL) { - on_complete->next_data.scratch |= CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE; + on_complete->next_data.scratch |= CLOSURE_BARRIER_MAY_COVER_WRITE; s->fetching_send_message_finished = add_closure_barrier(op->on_complete); if (s->write_closed) { grpc_chttp2_complete_closure_step( @@ -1120,7 +1125,7 @@ static void perform_stream_op_locked(grpc_exec_ctx *exec_ctx, void *stream_op, if (op->send_trailing_metadata != NULL) { GPR_ASSERT(s->send_trailing_metadata_finished == NULL); - on_complete->next_data.scratch |= CLOSURE_BARRIER_CANNOT_RUN_WITH_WRITE; + on_complete->next_data.scratch |= CLOSURE_BARRIER_MAY_COVER_WRITE; s->send_trailing_metadata_finished = add_closure_barrier(on_complete); s->send_trailing_metadata = op->send_trailing_metadata; const size_t metadata_size = From cbe159925005d6f71e891792d593b51263ac6b76 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Mon, 5 Dec 2016 13:56:29 -0800 Subject: [PATCH 32/51] Track requests that could cause other requests to be created, and don't do a real core shutdown of a CQ until such requests are done --- .../grpc++/impl/codegen/completion_queue.h | 19 ++++++++++++++++++- .../grpc++/impl/codegen/server_interface.h | 2 +- src/cpp/common/completion_queue_cc.cc | 15 +++++++++++---- src/cpp/server/server_cc.cc | 5 +++++ 4 files changed, 35 insertions(+), 6 deletions(-) diff --git a/include/grpc++/impl/codegen/completion_queue.h b/include/grpc++/impl/codegen/completion_queue.h index ef00163b7ec..a9bb98edd99 100644 --- a/include/grpc++/impl/codegen/completion_queue.h +++ b/include/grpc++/impl/codegen/completion_queue.h @@ -52,6 +52,7 @@ #include #include #include +#include struct grpc_completion_queue; @@ -101,6 +102,7 @@ class CompletionQueue : private GrpcLibraryCodegen { /// instance. CompletionQueue() { cq_ = g_core_codegen_interface->grpc_completion_queue_create(nullptr); + RegisterAvalanching(); // reserve this for the future shutdown } /// Wrap \a take, taking ownership of the instance. @@ -151,7 +153,8 @@ class CompletionQueue : private GrpcLibraryCodegen { /// Request the shutdown of the queue. /// - /// \warning This method must be called at some point. Once invoked, \a Next + /// \warning This method must be called at some point if this completion queue + /// is accessed with Next or AsyncNext. Once invoked, \a Next /// will start to return false and \a AsyncNext will return \a /// NextStatus::SHUTDOWN. Only once either one of these methods does that /// (that is, once the queue has been \em drained) can an instance of this @@ -165,6 +168,18 @@ class CompletionQueue : private GrpcLibraryCodegen { /// owership is performed. grpc_completion_queue* cq() { return cq_; } + /// Manage state of avalanching operations : completion queue tags that + /// trigger other completion queue operations. The underlying core completion + /// queue should not really shutdown until all avalanching operations have + /// been finalized. Note that we maintain the requirement that an avalanche + /// registration must take place before CQ shutdown (which must be maintained + /// elsehwere) + void RegisterAvalanching() { + gpr_atm_no_barrier_fetch_add(&avalanches_in_flight_, + static_cast(1)); + }; + void CompleteAvalanching(); + private: // Friend synchronous wrappers so that they can access Pluck(), which is // a semi-private API geared towards the synchronous implementation. @@ -229,6 +244,8 @@ class CompletionQueue : private GrpcLibraryCodegen { } grpc_completion_queue* cq_; // owned + + gpr_atm avalanches_in_flight_; }; /// A specific type of completion queue used by the processing of notifications diff --git a/include/grpc++/impl/codegen/server_interface.h b/include/grpc++/impl/codegen/server_interface.h index 41a64bead03..666b9ff66eb 100644 --- a/include/grpc++/impl/codegen/server_interface.h +++ b/include/grpc++/impl/codegen/server_interface.h @@ -140,7 +140,7 @@ class ServerInterface : public CallHook { ServerAsyncStreamingInterface* stream, CompletionQueue* call_cq, void* tag, bool delete_on_finalize); - virtual ~BaseAsyncRequest() {} + virtual ~BaseAsyncRequest(); bool FinalizeResult(void** tag, bool* status) override; diff --git a/src/cpp/common/completion_queue_cc.cc b/src/cpp/common/completion_queue_cc.cc index 00cc102f928..558f47af842 100644 --- a/src/cpp/common/completion_queue_cc.cc +++ b/src/cpp/common/completion_queue_cc.cc @@ -43,11 +43,18 @@ namespace grpc { static internal::GrpcLibraryInitializer g_gli_initializer; -CompletionQueue::CompletionQueue(grpc_completion_queue* take) : cq_(take) {} +CompletionQueue::CompletionQueue(grpc_completion_queue* take) : cq_(take) { + RegisterAvalanching(); +} + +void CompletionQueue::Shutdown() { CompleteAvalanching(); } -void CompletionQueue::Shutdown() { - g_gli_initializer.summon(); - grpc_completion_queue_shutdown(cq_); +void CompletionQueue::CompleteAvalanching() { + // Check if this was the last avalanching operation + if (gpr_atm_no_barrier_fetch_add(&avalanches_in_flight_, + static_cast(-1)) == 1) { + grpc_completion_queue_shutdown(cq_); + } } CompletionQueue::NextStatus CompletionQueue::AsyncNextInternal( diff --git a/src/cpp/server/server_cc.cc b/src/cpp/server/server_cc.cc index b7cfd6dbf11..96820ff9633 100644 --- a/src/cpp/server/server_cc.cc +++ b/src/cpp/server/server_cc.cc @@ -575,9 +575,14 @@ ServerInterface::BaseAsyncRequest::BaseAsyncRequest( tag_(tag), delete_on_finalize_(delete_on_finalize), call_(nullptr) { + call_cq_->RegisterAvalanching(); // This op will trigger more ops memset(&initial_metadata_array_, 0, sizeof(initial_metadata_array_)); } +ServerInterface::BaseAsyncRequest::~BaseAsyncRequest() { + call_cq_->CompleteAvalanching(); +} + bool ServerInterface::BaseAsyncRequest::FinalizeResult(void** tag, bool* status) { if (*status) { From bf24dd9e51f433aec60fbef24ee68496829277b1 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Mon, 5 Dec 2016 13:59:09 -0800 Subject: [PATCH 33/51] clang-format --- include/grpc++/impl/codegen/completion_queue.h | 2 +- src/cpp/server/server_cc.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/grpc++/impl/codegen/completion_queue.h b/include/grpc++/impl/codegen/completion_queue.h index a9bb98edd99..75e73ee1f44 100644 --- a/include/grpc++/impl/codegen/completion_queue.h +++ b/include/grpc++/impl/codegen/completion_queue.h @@ -102,7 +102,7 @@ class CompletionQueue : private GrpcLibraryCodegen { /// instance. CompletionQueue() { cq_ = g_core_codegen_interface->grpc_completion_queue_create(nullptr); - RegisterAvalanching(); // reserve this for the future shutdown + RegisterAvalanching(); // reserve this for the future shutdown } /// Wrap \a take, taking ownership of the instance. diff --git a/src/cpp/server/server_cc.cc b/src/cpp/server/server_cc.cc index 96820ff9633..1063c5d7f24 100644 --- a/src/cpp/server/server_cc.cc +++ b/src/cpp/server/server_cc.cc @@ -575,7 +575,7 @@ ServerInterface::BaseAsyncRequest::BaseAsyncRequest( tag_(tag), delete_on_finalize_(delete_on_finalize), call_(nullptr) { - call_cq_->RegisterAvalanching(); // This op will trigger more ops + call_cq_->RegisterAvalanching(); // This op will trigger more ops memset(&initial_metadata_array_, 0, sizeof(initial_metadata_array_)); } From 6510d47c8117d357dfaa289683aa5feca50d7c72 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Mon, 5 Dec 2016 14:16:20 -0800 Subject: [PATCH 34/51] gpr_atm isn't automatically initialized to 0. Thanks Obama. --- include/grpc++/impl/codegen/completion_queue.h | 5 ++++- src/cpp/common/completion_queue_cc.cc | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/include/grpc++/impl/codegen/completion_queue.h b/include/grpc++/impl/codegen/completion_queue.h index 75e73ee1f44..944f2c39198 100644 --- a/include/grpc++/impl/codegen/completion_queue.h +++ b/include/grpc++/impl/codegen/completion_queue.h @@ -102,7 +102,7 @@ class CompletionQueue : private GrpcLibraryCodegen { /// instance. CompletionQueue() { cq_ = g_core_codegen_interface->grpc_completion_queue_create(nullptr); - RegisterAvalanching(); // reserve this for the future shutdown + InitialAvalanching(); // reserve this for the future shutdown } /// Wrap \a take, taking ownership of the instance. @@ -174,6 +174,9 @@ class CompletionQueue : private GrpcLibraryCodegen { /// been finalized. Note that we maintain the requirement that an avalanche /// registration must take place before CQ shutdown (which must be maintained /// elsehwere) + void InitialAvalanching() { + gpr_atm_rel_store(&avalanches_in_flight_, static_cast(1)); + } void RegisterAvalanching() { gpr_atm_no_barrier_fetch_add(&avalanches_in_flight_, static_cast(1)); diff --git a/src/cpp/common/completion_queue_cc.cc b/src/cpp/common/completion_queue_cc.cc index 558f47af842..418baac5b2b 100644 --- a/src/cpp/common/completion_queue_cc.cc +++ b/src/cpp/common/completion_queue_cc.cc @@ -44,7 +44,7 @@ namespace grpc { static internal::GrpcLibraryInitializer g_gli_initializer; CompletionQueue::CompletionQueue(grpc_completion_queue* take) : cq_(take) { - RegisterAvalanching(); + InitialAvalanching(); } void CompletionQueue::Shutdown() { CompleteAvalanching(); } From 949309104ac01353700e2b0954a8b7727f894372 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 5 Dec 2016 14:46:55 -0800 Subject: [PATCH 35/51] Increase test timeout of testMetadata to 8s to alleviate flakiness under overloaded Jenkins mac machine --- src/objective-c/tests/GRPCClientTests.m | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/objective-c/tests/GRPCClientTests.m b/src/objective-c/tests/GRPCClientTests.m index 77640525d58..07d53cd495f 100644 --- a/src/objective-c/tests/GRPCClientTests.m +++ b/src/objective-c/tests/GRPCClientTests.m @@ -225,7 +225,7 @@ static GRPCProtoMethod *kUnaryCallMethod; [call startWithWriteable:responsesWriteable]; - [self waitForExpectationsWithTimeout:4 handler:nil]; + [self waitForExpectationsWithTimeout:8 handler:nil]; } - (void)testResponseMetadataKVO { From 7e499aa3fbc9b7baa6571ed4a2c595f6c06e8faa Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Mon, 5 Dec 2016 14:50:14 -0800 Subject: [PATCH 36/51] Bring back gli initializer summoning --- src/cpp/common/completion_queue_cc.cc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cpp/common/completion_queue_cc.cc b/src/cpp/common/completion_queue_cc.cc index 418baac5b2b..0408a410853 100644 --- a/src/cpp/common/completion_queue_cc.cc +++ b/src/cpp/common/completion_queue_cc.cc @@ -47,7 +47,10 @@ CompletionQueue::CompletionQueue(grpc_completion_queue* take) : cq_(take) { InitialAvalanching(); } -void CompletionQueue::Shutdown() { CompleteAvalanching(); } +void CompletionQueue::Shutdown() { + g_gli_initializer.summon(); + CompleteAvalanching(); +} void CompletionQueue::CompleteAvalanching() { // Check if this was the last avalanching operation From 9c5318aba097dc8634626ba0b106fc8993ce32b5 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 5 Dec 2016 15:07:04 -0800 Subject: [PATCH 37/51] Fix shutdown problems with sync server --- src/core/lib/surface/call.c | 4 ++++ src/cpp/server/server_cc.cc | 12 ++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 1e0f3eeca52..8ca3cab9d57 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -1551,6 +1551,10 @@ static grpc_call_error call_start_batch(grpc_exec_ctx *exec_ctx, error = GRPC_CALL_ERROR_TOO_MANY_OPERATIONS; goto done_with_error; } + /* IF this is a server, then GRPC_OP_RECV_INITIAL_METADATA *must* come + from server.c. In that case, it's coming from accept_stream, and in + that case we're not necessarily covered by a poller. */ + stream_op->covered_by_poller = call->is_client; call->received_initial_metadata = 1; call->buffered_metadata[0] = op->data.recv_initial_metadata; grpc_closure_init(&call->receiving_initial_metadata_ready, diff --git a/src/cpp/server/server_cc.cc b/src/cpp/server/server_cc.cc index b7cfd6dbf11..3ec7faddada 100644 --- a/src/cpp/server/server_cc.cc +++ b/src/cpp/server/server_cc.cc @@ -510,12 +510,6 @@ void Server::ShutdownInternal(gpr_timespec deadline) { ShutdownTag shutdown_tag; // Dummy shutdown tag grpc_server_shutdown_and_notify(server_, shutdown_cq.cq(), &shutdown_tag); - // Shutdown all ThreadManagers. This will try to gracefully stop all the - // threads in the ThreadManagers (once they process any inflight requests) - for (auto it = sync_req_mgrs_.begin(); it != sync_req_mgrs_.end(); it++) { - (*it)->Shutdown(); // ThreadManager's Shutdown() - } - shutdown_cq.Shutdown(); void* tag; @@ -531,6 +525,12 @@ void Server::ShutdownInternal(gpr_timespec deadline) { // Else in case of SHUTDOWN or GOT_EVENT, it means that the server has // successfully shutdown + // Shutdown all ThreadManagers. This will try to gracefully stop all the + // threads in the ThreadManagers (once they process any inflight requests) + for (auto it = sync_req_mgrs_.begin(); it != sync_req_mgrs_.end(); it++) { + (*it)->Shutdown(); // ThreadManager's Shutdown() + } + // Wait for threads in all ThreadManagers to terminate for (auto it = sync_req_mgrs_.begin(); it != sync_req_mgrs_.end(); it++) { (*it)->Wait(); From 4ee9bb06b92a7cdd5f87240b0ec882c368d325dd Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 5 Dec 2016 15:37:47 -0800 Subject: [PATCH 38/51] Fix race if connector is shutdown while connecting --- .../ext/transport/chttp2/client/chttp2_connector.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/core/ext/transport/chttp2/client/chttp2_connector.c b/src/core/ext/transport/chttp2/client/chttp2_connector.c index 213395c20f1..568b114d641 100644 --- a/src/core/ext/transport/chttp2/client/chttp2_connector.c +++ b/src/core/ext/transport/chttp2/client/chttp2_connector.c @@ -56,6 +56,7 @@ typedef struct { gpr_refcount refs; bool shutdown; + bool connecting; char *server_name; grpc_chttp2_create_handshakers_func create_handshakers; @@ -103,7 +104,9 @@ static void chttp2_connector_shutdown(grpc_exec_ctx *exec_ctx, } // If handshaking is not yet in progress, shutdown the endpoint. // Otherwise, the handshaker will do this for us. - if (c->endpoint != NULL) grpc_endpoint_shutdown(exec_ctx, c->endpoint); + if (!c->connecting && c->endpoint != NULL) { + grpc_endpoint_shutdown(exec_ctx, c->endpoint); + } gpr_mu_unlock(&c->mu); } @@ -192,6 +195,8 @@ static void on_initial_connect_string_sent(grpc_exec_ctx *exec_ctx, void *arg, static void connected(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { chttp2_connector *c = arg; gpr_mu_lock(&c->mu); + GPR_ASSERT(c->connecting); + c->connecting = false; if (error != GRPC_ERROR_NONE || c->shutdown) { if (error == GRPC_ERROR_NONE) { error = GRPC_ERROR_CREATE("connector shutdown"); @@ -202,6 +207,7 @@ static void connected(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { grpc_closure *notify = c->notify; c->notify = NULL; grpc_exec_ctx_sched(exec_ctx, notify, error, NULL); + if (c->endpoint != NULL) grpc_endpoint_shutdown(exec_ctx, c->endpoint); gpr_mu_unlock(&c->mu); chttp2_connector_unref(exec_ctx, arg); } else { @@ -235,6 +241,8 @@ static void chttp2_connector_connect(grpc_exec_ctx *exec_ctx, GPR_ASSERT(c->endpoint == NULL); chttp2_connector_ref(con); // Ref taken for callback. grpc_closure_init(&c->connected, connected, c); + GPR_ASSERT(!c->connecting); + c->connecting = true; grpc_tcp_client_connect(exec_ctx, &c->connected, &c->endpoint, args->interested_parties, args->channel_args, args->addr, args->deadline); From c4f5a2eed244dcd8191383b9830f513dd20d6d40 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Mon, 5 Dec 2016 15:54:56 -0800 Subject: [PATCH 39/51] Fix test runner failures for Python on Windows --- tools/run_tests/run_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/run_tests/run_tests.py b/tools/run_tests/run_tests.py index 1544ff369d0..c49ee4a6cc6 100755 --- a/tools/run_tests/run_tests.py +++ b/tools/run_tests/run_tests.py @@ -511,7 +511,7 @@ class PythonLanguage(object): config.run, timeout_seconds=5*60, environ=dict(list(environment.items()) + - [('GRPC_PYTHON_TESTRUNNER_FILTER', suite_name)]), + [('GRPC_PYTHON_TESTRUNNER_FILTER', str(suite_name))]), shortname='%s.test.%s' % (config.name, suite_name),) for suite_name in tests_json for config in self.pythons] From 76b7ab431a2cee78294a4292d8a3d32b97820cc3 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 5 Dec 2016 15:55:18 -0800 Subject: [PATCH 40/51] Interop test might take a little longer to start up (esp. w/tsan) --- test/cpp/interop/interop_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cpp/interop/interop_test.cc b/test/cpp/interop/interop_test.cc index c066598d363..d4004740a41 100644 --- a/test/cpp/interop/interop_test.cc +++ b/test/cpp/interop/interop_test.cc @@ -126,7 +126,7 @@ int main(int argc, char** argv) { return 1; } /* wait a little */ - sleep(2); + sleep(10); /* start the clients */ ret = test_client(root, "127.0.0.1", port); if (ret != 0) return ret; From b1341a302b54959ba1b0fa6a4b7a70a88d4dcb55 Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Mon, 5 Dec 2016 16:38:02 -0800 Subject: [PATCH 41/51] Use unified test timeouts --- src/objective-c/tests/GRPCClientTests.m | 16 ++++++++------- src/objective-c/tests/InteropTests.m | 26 +++++++++++++------------ 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/objective-c/tests/GRPCClientTests.m b/src/objective-c/tests/GRPCClientTests.m index 07d53cd495f..0b72a75f3d5 100644 --- a/src/objective-c/tests/GRPCClientTests.m +++ b/src/objective-c/tests/GRPCClientTests.m @@ -43,6 +43,8 @@ #import #import +#define TEST_TIMEOUT 16 + static NSString * const kHostAddress = @"localhost:5050"; static NSString * const kPackage = @"grpc.testing"; static NSString * const kService = @"TestService"; @@ -137,7 +139,7 @@ static GRPCProtoMethod *kUnaryCallMethod; [call startWithWriteable:responsesWriteable]; - [self waitForExpectationsWithTimeout:4 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testEmptyRPC { @@ -159,7 +161,7 @@ static GRPCProtoMethod *kUnaryCallMethod; [call startWithWriteable:responsesWriteable]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testSimpleProtoRPC { @@ -191,7 +193,7 @@ static GRPCProtoMethod *kUnaryCallMethod; [call startWithWriteable:responsesWriteable]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testMetadata { @@ -225,7 +227,7 @@ static GRPCProtoMethod *kUnaryCallMethod; [call startWithWriteable:responsesWriteable]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testResponseMetadataKVO { @@ -256,7 +258,7 @@ static GRPCProtoMethod *kUnaryCallMethod; [call startWithWriteable:responsesWriteable]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testUserAgentPrefix { @@ -287,7 +289,7 @@ static GRPCProtoMethod *kUnaryCallMethod; [call startWithWriteable:responsesWriteable]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } // TODO(makarandd): Move to a different file that contains only unit tests @@ -347,7 +349,7 @@ static GRPCProtoMethod *kUnaryCallMethod; [call startWithWriteable:responsesWriteable]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } @end diff --git a/src/objective-c/tests/InteropTests.m b/src/objective-c/tests/InteropTests.m index 9804734d6ae..c3935ce1e09 100644 --- a/src/objective-c/tests/InteropTests.m +++ b/src/objective-c/tests/InteropTests.m @@ -46,6 +46,8 @@ #import #import +#define TEST_TIMEOUT 32 + // Convenience constructors for the generated proto messages: @interface RMTStreamingOutputCallRequest (Constructors) @@ -124,7 +126,7 @@ [expectation fulfill]; }]; - [self waitForExpectationsWithTimeout:4 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testLargeUnaryRPC { @@ -147,7 +149,7 @@ [expectation fulfill]; }]; - [self waitForExpectationsWithTimeout:16 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)test4MBResponsesAreAccepted { @@ -164,7 +166,7 @@ [expectation fulfill]; }]; - [self waitForExpectationsWithTimeout:16 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testResponsesOverMaxSizeFailWithActionableMessage { @@ -185,7 +187,7 @@ [expectation fulfill]; }]; - [self waitForExpectationsWithTimeout:16 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testResponsesOver4MBAreAcceptedIfOptedIn { @@ -205,7 +207,7 @@ [expectation fulfill]; }]; - [self waitForExpectationsWithTimeout:16 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testClientStreamingRPC { @@ -238,7 +240,7 @@ [expectation fulfill]; }]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testServerStreamingRPC { @@ -275,7 +277,7 @@ } }]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testPingPongRPC { @@ -319,7 +321,7 @@ [expectation fulfill]; } }]; - [self waitForExpectationsWithTimeout:4 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } #ifndef GRPC_COMPILE_WITH_CRONET @@ -335,7 +337,7 @@ XCTAssert(done, @"Unexpected response: %@", response); [expectation fulfill]; }]; - [self waitForExpectationsWithTimeout:2 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } #endif @@ -361,7 +363,7 @@ [call cancel]; XCTAssertEqual(call.state, GRXWriterStateFinished); - [self waitForExpectationsWithTimeout:1 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testCancelAfterFirstResponseRPC { @@ -396,7 +398,7 @@ } }]; [call start]; - [self waitForExpectationsWithTimeout:8 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } - (void)testRPCAfterClosingOpenConnections { @@ -420,7 +422,7 @@ }]; }]; - [self waitForExpectationsWithTimeout:4 handler:nil]; + [self waitForExpectationsWithTimeout:TEST_TIMEOUT handler:nil]; } @end From fcf09ea42e0a4216292152b12514329ce49e7706 Mon Sep 17 00:00:00 2001 From: Alex Polcyn Date: Tue, 6 Dec 2016 04:00:05 +0000 Subject: [PATCH 42/51] handle empty string for qps workers in driver and dont quit them on netperf --- test/cpp/qps/driver.cc | 2 +- tools/run_tests/run_performance_tests.py | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/test/cpp/qps/driver.cc b/test/cpp/qps/driver.cc index ea0b38e8ad0..22b2cd080d8 100644 --- a/test/cpp/qps/driver.cc +++ b/test/cpp/qps/driver.cc @@ -101,7 +101,7 @@ static std::unordered_map> get_hosts_and_cores( static deque get_workers(const string& name) { char* env = gpr_getenv(name.c_str()); - if (!env) return deque(); + if (!env || strlen(env) == 0) return deque(); deque out; char* p = env; diff --git a/tools/run_tests/run_performance_tests.py b/tools/run_tests/run_performance_tests.py index 1d0c98fb690..5e6ff44f4fa 100755 --- a/tools/run_tests/run_performance_tests.py +++ b/tools/run_tests/run_performance_tests.py @@ -464,9 +464,10 @@ for scenario in scenarios: try: for worker in scenario.workers: worker.start() - scenario_failures, resultset = jobset.run([scenario.jobspec, - create_quit_jobspec(scenario.workers, remote_host=args.remote_driver_host)], - newline_on_success=True, maxjobs=1) + jobs = [scenario.jobspec] + if len(scenario.workers) > 0: + jobs.append(create_quit_jobspec(scenario.workers, remote_host=args.remote_driver_host)) + scenario_failures, resultset = jobset.run(jobs, newline_on_success=True, maxjobs=1) total_scenario_failures += scenario_failures merged_resultset = dict(itertools.chain(merged_resultset.iteritems(), resultset.iteritems())) From ca5e92442fedd5bd72fe2438f7d03c7bf2f9b00d Mon Sep 17 00:00:00 2001 From: Alex Polcyn Date: Tue, 6 Dec 2016 04:21:37 +0000 Subject: [PATCH 43/51] clean up scenario.workers length check --- tools/run_tests/run_performance_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/run_tests/run_performance_tests.py b/tools/run_tests/run_performance_tests.py index 5e6ff44f4fa..0109099cccb 100755 --- a/tools/run_tests/run_performance_tests.py +++ b/tools/run_tests/run_performance_tests.py @@ -465,7 +465,7 @@ for scenario in scenarios: for worker in scenario.workers: worker.start() jobs = [scenario.jobspec] - if len(scenario.workers) > 0: + if scenario.workers: jobs.append(create_quit_jobspec(scenario.workers, remote_host=args.remote_driver_host)) scenario_failures, resultset = jobset.run(jobs, newline_on_success=True, maxjobs=1) total_scenario_failures += scenario_failures From 200ef28796aa2880bd95409d944fb5f959f93d4b Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 6 Dec 2016 14:18:21 -0800 Subject: [PATCH 44/51] Perform quit operations in a useful order in Node perf tests --- src/node/performance/worker_service_impl.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/node/performance/worker_service_impl.js b/src/node/performance/worker_service_impl.js index 3f317f64294..38888a7219a 100644 --- a/src/node/performance/worker_service_impl.js +++ b/src/node/performance/worker_service_impl.js @@ -55,9 +55,8 @@ module.exports = function WorkerServiceImpl(benchmark_impl, server) { } this.quitWorker = function quitWorker(call, callback) { - server.tryShutdown(function() { - callback(null, {}); - }); + callback(null, {}); + server.tryShutdown(function() {}); }; this.runClient = function runClient(call) { From ff3112188b1d00c7907465da3b08bc6f7d289631 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 6 Dec 2016 16:03:18 -0800 Subject: [PATCH 45/51] Renamed google_benchmark submodule to benchmark --- .gitmodules | 4 +- Makefile | 60 ++++----- build.yaml | 8 +- .../gen_build_yaml.py | 10 +- test/cpp/microbenchmarks/bm_fullstack.cc | 2 +- test/cpp/microbenchmarks/noop-benchmark.cc | 4 +- third_party/{google_benchmark => benchmark} | 0 tools/buildgen/generate_build_additions.sh | 2 +- tools/run_tests/sanity/check_submodules.sh | 2 +- tools/run_tests/sources_and_headers.json | 52 ++++---- .../benchmark.vcxproj} | 72 +++++----- .../benchmark/benchmark.vcxproj.filters | 125 ++++++++++++++++++ .../google_benchmark.vcxproj.filters | 125 ------------------ .../test/bm_fullstack/bm_fullstack.vcxproj | 4 +- .../noop-benchmark/noop-benchmark.vcxproj | 4 +- 15 files changed, 237 insertions(+), 237 deletions(-) rename src/{google_benchmark => benchmark}/gen_build_yaml.py (86%) rename third_party/{google_benchmark => benchmark} (100%) rename vsprojects/vcxproj/{google_benchmark/google_benchmark.vcxproj => benchmark/benchmark.vcxproj} (70%) create mode 100644 vsprojects/vcxproj/benchmark/benchmark.vcxproj.filters delete mode 100644 vsprojects/vcxproj/google_benchmark/google_benchmark.vcxproj.filters diff --git a/.gitmodules b/.gitmodules index c32881cb951..04d155cfb41 100644 --- a/.gitmodules +++ b/.gitmodules @@ -17,6 +17,6 @@ [submodule "third_party/thrift"] path = third_party/thrift url = https://github.com/apache/thrift.git -[submodule "third_party/google_benchmark"] - path = third_party/google_benchmark +[submodule "third_party/benchmark"] + path = third_party/benchmark url = https://github.com/google/benchmark diff --git a/Makefile b/Makefile index db30c215862..8f7328ae285 100644 --- a/Makefile +++ b/Makefile @@ -1260,9 +1260,9 @@ pc_cxx: $(LIBDIR)/$(CONFIG)/pkgconfig/grpc++.pc pc_cxx_unsecure: $(LIBDIR)/$(CONFIG)/pkgconfig/grpc++_unsecure.pc ifeq ($(EMBED_OPENSSL),true) -privatelibs_cxx: $(LIBDIR)/$(CONFIG)/libgrpc++_proto_reflection_desc_db.a $(LIBDIR)/$(CONFIG)/libgrpc++_test.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_config.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_cli_libs.a $(LIBDIR)/$(CONFIG)/libinterop_client_helper.a $(LIBDIR)/$(CONFIG)/libinterop_client_main.a $(LIBDIR)/$(CONFIG)/libinterop_server_helper.a $(LIBDIR)/$(CONFIG)/libinterop_server_lib.a $(LIBDIR)/$(CONFIG)/libinterop_server_main.a $(LIBDIR)/$(CONFIG)/libqps.a $(LIBDIR)/$(CONFIG)/libboringssl_test_util.a $(LIBDIR)/$(CONFIG)/libboringssl_aes_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_asn1_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_base64_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_bio_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_bn_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_bytestring_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_aead_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_cipher_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_cmac_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_ed25519_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_x25519_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_dh_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_digest_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_ec_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_ecdsa_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_err_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_evp_extra_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_evp_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_pbkdf_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_hmac_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_pkcs12_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_pkcs8_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_poly1305_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_rsa_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_x509_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_ssl_test_lib.a $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a +privatelibs_cxx: $(LIBDIR)/$(CONFIG)/libgrpc++_proto_reflection_desc_db.a $(LIBDIR)/$(CONFIG)/libgrpc++_test.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_config.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_cli_libs.a $(LIBDIR)/$(CONFIG)/libinterop_client_helper.a $(LIBDIR)/$(CONFIG)/libinterop_client_main.a $(LIBDIR)/$(CONFIG)/libinterop_server_helper.a $(LIBDIR)/$(CONFIG)/libinterop_server_lib.a $(LIBDIR)/$(CONFIG)/libinterop_server_main.a $(LIBDIR)/$(CONFIG)/libqps.a $(LIBDIR)/$(CONFIG)/libboringssl_test_util.a $(LIBDIR)/$(CONFIG)/libboringssl_aes_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_asn1_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_base64_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_bio_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_bn_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_bytestring_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_aead_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_cipher_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_cmac_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_ed25519_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_x25519_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_dh_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_digest_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_ec_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_ecdsa_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_err_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_evp_extra_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_evp_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_pbkdf_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_hmac_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_pkcs12_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_pkcs8_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_poly1305_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_rsa_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_x509_test_lib.a $(LIBDIR)/$(CONFIG)/libboringssl_ssl_test_lib.a $(LIBDIR)/$(CONFIG)/libbenchmark.a else -privatelibs_cxx: $(LIBDIR)/$(CONFIG)/libgrpc++_proto_reflection_desc_db.a $(LIBDIR)/$(CONFIG)/libgrpc++_test.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_config.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_cli_libs.a $(LIBDIR)/$(CONFIG)/libinterop_client_helper.a $(LIBDIR)/$(CONFIG)/libinterop_client_main.a $(LIBDIR)/$(CONFIG)/libinterop_server_helper.a $(LIBDIR)/$(CONFIG)/libinterop_server_lib.a $(LIBDIR)/$(CONFIG)/libinterop_server_main.a $(LIBDIR)/$(CONFIG)/libqps.a $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a +privatelibs_cxx: $(LIBDIR)/$(CONFIG)/libgrpc++_proto_reflection_desc_db.a $(LIBDIR)/$(CONFIG)/libgrpc++_test.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_config.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_cli_libs.a $(LIBDIR)/$(CONFIG)/libinterop_client_helper.a $(LIBDIR)/$(CONFIG)/libinterop_client_main.a $(LIBDIR)/$(CONFIG)/libinterop_server_helper.a $(LIBDIR)/$(CONFIG)/libinterop_server_lib.a $(LIBDIR)/$(CONFIG)/libinterop_server_main.a $(LIBDIR)/$(CONFIG)/libqps.a $(LIBDIR)/$(CONFIG)/libbenchmark.a endif @@ -6998,43 +6998,43 @@ ifneq ($(NO_DEPS),true) endif -LIBGOOGLE_BENCHMARK_SRC = \ - third_party/google_benchmark/src/benchmark.cc \ - third_party/google_benchmark/src/benchmark_register.cc \ - third_party/google_benchmark/src/colorprint.cc \ - third_party/google_benchmark/src/commandlineflags.cc \ - third_party/google_benchmark/src/complexity.cc \ - third_party/google_benchmark/src/console_reporter.cc \ - third_party/google_benchmark/src/csv_reporter.cc \ - third_party/google_benchmark/src/json_reporter.cc \ - third_party/google_benchmark/src/reporter.cc \ - third_party/google_benchmark/src/sleep.cc \ - third_party/google_benchmark/src/string_util.cc \ - third_party/google_benchmark/src/sysinfo.cc \ - third_party/google_benchmark/src/timers.cc \ +LIBBENCHMARK_SRC = \ + third_party/benchmark/src/benchmark.cc \ + third_party/benchmark/src/benchmark_register.cc \ + third_party/benchmark/src/colorprint.cc \ + third_party/benchmark/src/commandlineflags.cc \ + third_party/benchmark/src/complexity.cc \ + third_party/benchmark/src/console_reporter.cc \ + third_party/benchmark/src/csv_reporter.cc \ + third_party/benchmark/src/json_reporter.cc \ + third_party/benchmark/src/reporter.cc \ + third_party/benchmark/src/sleep.cc \ + third_party/benchmark/src/string_util.cc \ + third_party/benchmark/src/sysinfo.cc \ + third_party/benchmark/src/timers.cc \ PUBLIC_HEADERS_CXX += \ -LIBGOOGLE_BENCHMARK_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(LIBGOOGLE_BENCHMARK_SRC)))) +LIBBENCHMARK_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(LIBBENCHMARK_SRC)))) -$(LIBGOOGLE_BENCHMARK_OBJS): CPPFLAGS += -Ithird_party/google_benchmark/include -DHAVE_POSIX_REGEX +$(LIBBENCHMARK_OBJS): CPPFLAGS += -Ithird_party/benchmark/include -DHAVE_POSIX_REGEX ifeq ($(NO_PROTOBUF),true) # You can't build a C++ library if you don't have protobuf - a bit overreached, but still okay. -$(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a: protobuf_dep_error +$(LIBDIR)/$(CONFIG)/libbenchmark.a: protobuf_dep_error else -$(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a: $(ZLIB_DEP) $(PROTOBUF_DEP) $(LIBGOOGLE_BENCHMARK_OBJS) +$(LIBDIR)/$(CONFIG)/libbenchmark.a: $(ZLIB_DEP) $(PROTOBUF_DEP) $(LIBBENCHMARK_OBJS) $(E) "[AR] Creating $@" $(Q) mkdir -p `dirname $@` - $(Q) rm -f $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a - $(Q) $(AR) $(AROPTS) $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a $(LIBGOOGLE_BENCHMARK_OBJS) + $(Q) rm -f $(LIBDIR)/$(CONFIG)/libbenchmark.a + $(Q) $(AR) $(AROPTS) $(LIBDIR)/$(CONFIG)/libbenchmark.a $(LIBBENCHMARK_OBJS) ifeq ($(SYSTEM),Darwin) - $(Q) ranlib -no_warning_for_no_symbols $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a + $(Q) ranlib -no_warning_for_no_symbols $(LIBDIR)/$(CONFIG)/libbenchmark.a endif @@ -7043,7 +7043,7 @@ endif endif ifneq ($(NO_DEPS),true) --include $(LIBGOOGLE_BENCHMARK_OBJS:.o=.dep) +-include $(LIBBENCHMARK_OBJS:.o=.dep) endif @@ -11736,16 +11736,16 @@ $(BINDIR)/$(CONFIG)/bm_fullstack: protobuf_dep_error else -$(BINDIR)/$(CONFIG)/bm_fullstack: $(PROTOBUF_DEP) $(BM_FULLSTACK_OBJS) $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a +$(BINDIR)/$(CONFIG)/bm_fullstack: $(PROTOBUF_DEP) $(BM_FULLSTACK_OBJS) $(LIBDIR)/$(CONFIG)/libbenchmark.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(E) "[LD] Linking $@" $(Q) mkdir -p `dirname $@` - $(Q) $(LDXX) $(LDFLAGS) $(BM_FULLSTACK_OBJS) $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/bm_fullstack + $(Q) $(LDXX) $(LDFLAGS) $(BM_FULLSTACK_OBJS) $(LIBDIR)/$(CONFIG)/libbenchmark.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/bm_fullstack endif endif -$(OBJDIR)/$(CONFIG)/test/cpp/microbenchmarks/bm_fullstack.o: $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a +$(OBJDIR)/$(CONFIG)/test/cpp/microbenchmarks/bm_fullstack.o: $(LIBDIR)/$(CONFIG)/libbenchmark.a $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a deps_bm_fullstack: $(BM_FULLSTACK_OBJS:.o=.dep) @@ -13192,16 +13192,16 @@ $(BINDIR)/$(CONFIG)/noop-benchmark: protobuf_dep_error else -$(BINDIR)/$(CONFIG)/noop-benchmark: $(PROTOBUF_DEP) $(NOOP-BENCHMARK_OBJS) $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a +$(BINDIR)/$(CONFIG)/noop-benchmark: $(PROTOBUF_DEP) $(NOOP-BENCHMARK_OBJS) $(LIBDIR)/$(CONFIG)/libbenchmark.a $(E) "[LD] Linking $@" $(Q) mkdir -p `dirname $@` - $(Q) $(LDXX) $(LDFLAGS) $(NOOP-BENCHMARK_OBJS) $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/noop-benchmark + $(Q) $(LDXX) $(LDFLAGS) $(NOOP-BENCHMARK_OBJS) $(LIBDIR)/$(CONFIG)/libbenchmark.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/noop-benchmark endif endif -$(OBJDIR)/$(CONFIG)/test/cpp/microbenchmarks/noop-benchmark.o: $(LIBDIR)/$(CONFIG)/libgoogle_benchmark.a +$(OBJDIR)/$(CONFIG)/test/cpp/microbenchmarks/noop-benchmark.o: $(LIBDIR)/$(CONFIG)/libbenchmark.a deps_noop-benchmark: $(NOOP-BENCHMARK_OBJS:.o=.dep) diff --git a/build.yaml b/build.yaml index 68d19a6b44f..de9d253ef1c 100644 --- a/build.yaml +++ b/build.yaml @@ -2850,7 +2850,7 @@ targets: src: - test/cpp/microbenchmarks/bm_fullstack.cc deps: - - google_benchmark + - benchmark - grpc++_test_util - grpc_test_util - grpc++ @@ -3300,7 +3300,7 @@ targets: src: - test/cpp/microbenchmarks/noop-benchmark.cc deps: - - google_benchmark + - benchmark - name: proto_server_reflection_test gtest: true build: test @@ -3786,6 +3786,8 @@ configs: UBSAN_OPTIONS: halt_on_error=1:print_stacktrace=1 timeout_multiplier: 1.5 defaults: + benchmark: + CPPFLAGS: -Ithird_party/benchmark/include -DHAVE_POSIX_REGEX boringssl: CFLAGS: -Wno-sign-conversion -Wno-conversion -Wno-unused-value -Wno-unknown-pragmas -Wno-implicit-function-declaration -Wno-unused-variable -Wno-sign-compare $(NO_W_EXTRA_SEMI) @@ -3794,8 +3796,6 @@ defaults: global: CPPFLAGS: -g -Wall -Wextra -Werror -Wno-long-long -Wno-unused-parameter LDFLAGS: -g - google_benchmark: - CPPFLAGS: -Ithird_party/google_benchmark/include -DHAVE_POSIX_REGEX zlib: CFLAGS: -Wno-sign-conversion -Wno-conversion -Wno-unused-value -Wno-implicit-function-declaration $(W_NO_SHIFT_NEGATIVE_VALUE) -fvisibility=hidden diff --git a/src/google_benchmark/gen_build_yaml.py b/src/benchmark/gen_build_yaml.py similarity index 86% rename from src/google_benchmark/gen_build_yaml.py rename to src/benchmark/gen_build_yaml.py index 302e08737af..09b76115a86 100755 --- a/src/google_benchmark/gen_build_yaml.py +++ b/src/benchmark/gen_build_yaml.py @@ -39,15 +39,15 @@ os.chdir(os.path.dirname(sys.argv[0])+'/../..') out = {} out['libs'] = [{ - 'name': 'google_benchmark', + 'name': 'benchmark', 'build': 'private', 'language': 'c++', 'secure': 'no', - 'defaults': 'google_benchmark', - 'src': sorted(glob.glob('third_party/google_benchmark/src/*.cc')), + 'defaults': 'benchmark', + 'src': sorted(glob.glob('third_party/benchmark/src/*.cc')), 'headers': sorted( - glob.glob('third_party/google_benchmark/src/*.h') + - glob.glob('third_party/google_benchmark/include/benchmark/*.h')), + glob.glob('third_party/benchmark/src/*.h') + + glob.glob('third_party/benchmark/include/benchmark/*.h')), }] print yaml.dump(out) diff --git a/test/cpp/microbenchmarks/bm_fullstack.cc b/test/cpp/microbenchmarks/bm_fullstack.cc index 6cc780d44af..6c0bf804885 100644 --- a/test/cpp/microbenchmarks/bm_fullstack.cc +++ b/test/cpp/microbenchmarks/bm_fullstack.cc @@ -59,7 +59,7 @@ extern "C" { } #include "src/cpp/client/create_channel_internal.h" #include "src/proto/grpc/testing/echo.grpc.pb.h" -#include "third_party/google_benchmark/include/benchmark/benchmark.h" +#include "third_party/benchmark/include/benchmark/benchmark.h" namespace grpc { namespace testing { diff --git a/test/cpp/microbenchmarks/noop-benchmark.cc b/test/cpp/microbenchmarks/noop-benchmark.cc index 6b06c69c6e3..99fa6d5f6e5 100644 --- a/test/cpp/microbenchmarks/noop-benchmark.cc +++ b/test/cpp/microbenchmarks/noop-benchmark.cc @@ -31,10 +31,10 @@ * */ -/* This benchmark exists to ensure that the google_benchmark integration is +/* This benchmark exists to ensure that the benchmark integration is * working */ -#include "third_party/google_benchmark/include/benchmark/benchmark.h" +#include "third_party/benchmark/include/benchmark/benchmark.h" static void BM_NoOp(benchmark::State& state) { while (state.KeepRunning()) { diff --git a/third_party/google_benchmark b/third_party/benchmark similarity index 100% rename from third_party/google_benchmark rename to third_party/benchmark diff --git a/tools/buildgen/generate_build_additions.sh b/tools/buildgen/generate_build_additions.sh index 1ea47042f44..53c30c7609d 100644 --- a/tools/buildgen/generate_build_additions.sh +++ b/tools/buildgen/generate_build_additions.sh @@ -30,7 +30,7 @@ gen_build_yaml_dirs=" \ src/boringssl \ - src/google_benchmark \ + src/benchmark \ src/proto \ src/zlib \ test/core/bad_client \ diff --git a/tools/run_tests/sanity/check_submodules.sh b/tools/run_tests/sanity/check_submodules.sh index 6ec0786c966..be12f968d2b 100755 --- a/tools/run_tests/sanity/check_submodules.sh +++ b/tools/run_tests/sanity/check_submodules.sh @@ -43,7 +43,7 @@ git submodule | awk '{ print $1 }' | sort > $submodules cat << EOF | awk '{ print $1 }' | sort > $want_submodules c880e42ba1c8032d4cdde2aba0541d8a9d9fa2e9 third_party/boringssl (version_for_cocoapods_2.0-100-gc880e42) 05b155ff59114735ec8cd089f669c4c3d8f59029 third_party/gflags (v2.1.0-45-g05b155f) - 44c25c892a6229b20db7cd9dc05584ea865896de third_party/google_benchmark (v0.1.0-343-g44c25c8) + 44c25c892a6229b20db7cd9dc05584ea865896de third_party/benchmark (v0.1.0-343-g44c25c8) c99458533a9b4c743ed51537e25989ea55944908 third_party/googletest (release-1.7.0) a428e42072765993ff674fda72863c9f1aa2d268 third_party/protobuf (v3.1.0) 50893291621658f355bc5b4d450a8d06a563053d third_party/zlib (v1.2.8) diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index 2e6877ccac5..6ae269cc20d 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -2263,7 +2263,7 @@ }, { "deps": [ - "google_benchmark", + "benchmark", "gpr", "gpr_test_util", "grpc", @@ -2913,7 +2913,7 @@ }, { "deps": [ - "google_benchmark" + "benchmark" ], "headers": [], "is_filegroup": false, @@ -6207,30 +6207,30 @@ { "deps": [], "headers": [ - "third_party/google_benchmark/include/benchmark/benchmark.h", - "third_party/google_benchmark/include/benchmark/benchmark_api.h", - "third_party/google_benchmark/include/benchmark/macros.h", - "third_party/google_benchmark/include/benchmark/reporter.h", - "third_party/google_benchmark/src/arraysize.h", - "third_party/google_benchmark/src/benchmark_api_internal.h", - "third_party/google_benchmark/src/check.h", - "third_party/google_benchmark/src/colorprint.h", - "third_party/google_benchmark/src/commandlineflags.h", - "third_party/google_benchmark/src/complexity.h", - "third_party/google_benchmark/src/cycleclock.h", - "third_party/google_benchmark/src/internal_macros.h", - "third_party/google_benchmark/src/log.h", - "third_party/google_benchmark/src/mutex.h", - "third_party/google_benchmark/src/re.h", - "third_party/google_benchmark/src/sleep.h", - "third_party/google_benchmark/src/stat.h", - "third_party/google_benchmark/src/string_util.h", - "third_party/google_benchmark/src/sysinfo.h", - "third_party/google_benchmark/src/timers.h" - ], - "is_filegroup": false, - "language": "c++", - "name": "google_benchmark", + "third_party/benchmark/include/benchmark/benchmark.h", + "third_party/benchmark/include/benchmark/benchmark_api.h", + "third_party/benchmark/include/benchmark/macros.h", + "third_party/benchmark/include/benchmark/reporter.h", + "third_party/benchmark/src/arraysize.h", + "third_party/benchmark/src/benchmark_api_internal.h", + "third_party/benchmark/src/check.h", + "third_party/benchmark/src/colorprint.h", + "third_party/benchmark/src/commandlineflags.h", + "third_party/benchmark/src/complexity.h", + "third_party/benchmark/src/cycleclock.h", + "third_party/benchmark/src/internal_macros.h", + "third_party/benchmark/src/log.h", + "third_party/benchmark/src/mutex.h", + "third_party/benchmark/src/re.h", + "third_party/benchmark/src/sleep.h", + "third_party/benchmark/src/stat.h", + "third_party/benchmark/src/string_util.h", + "third_party/benchmark/src/sysinfo.h", + "third_party/benchmark/src/timers.h" + ], + "is_filegroup": false, + "language": "c++", + "name": "benchmark", "src": [], "third_party": false, "type": "lib" diff --git a/vsprojects/vcxproj/google_benchmark/google_benchmark.vcxproj b/vsprojects/vcxproj/benchmark/benchmark.vcxproj similarity index 70% rename from vsprojects/vcxproj/google_benchmark/google_benchmark.vcxproj rename to vsprojects/vcxproj/benchmark/benchmark.vcxproj index 52774e08025..9f262b3b00c 100644 --- a/vsprojects/vcxproj/google_benchmark/google_benchmark.vcxproj +++ b/vsprojects/vcxproj/benchmark/benchmark.vcxproj @@ -19,7 +19,7 @@ - {AAD4AEF3-DF1E-7A6D-EC35-233BD1031BF4} + {07978586-E47C-8709-A63E-895FBF3C3C7D} true $(SolutionDir)IntDir\$(MSBuildProjectName)\ @@ -57,10 +57,10 @@ - google_benchmark + benchmark - google_benchmark + benchmark @@ -147,53 +147,53 @@ - - - - - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + + + + + + + - + - + - + - + - + - + - + - + - + - + - + - + - + diff --git a/vsprojects/vcxproj/benchmark/benchmark.vcxproj.filters b/vsprojects/vcxproj/benchmark/benchmark.vcxproj.filters new file mode 100644 index 00000000000..ccc9ca2cae7 --- /dev/null +++ b/vsprojects/vcxproj/benchmark/benchmark.vcxproj.filters @@ -0,0 +1,125 @@ + + + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + + + third_party\benchmark\include\benchmark + + + third_party\benchmark\include\benchmark + + + third_party\benchmark\include\benchmark + + + third_party\benchmark\include\benchmark + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + third_party\benchmark\src + + + + + + {7b593518-9fee-107e-6b64-24bdce73f939} + + + {f0d35de1-6b41-778d-0ba0-faad514fb0f4} + + + {cbc02dfa-face-8cc6-0efb-efacc0c3369c} + + + {4f2f03fc-b82d-df33-63ee-bedebeb2c0ee} + + + {f42a8e0a-5a76-0e6f-d708-f0306858f673} + + + + diff --git a/vsprojects/vcxproj/google_benchmark/google_benchmark.vcxproj.filters b/vsprojects/vcxproj/google_benchmark/google_benchmark.vcxproj.filters deleted file mode 100644 index 9db6ed46574..00000000000 --- a/vsprojects/vcxproj/google_benchmark/google_benchmark.vcxproj.filters +++ /dev/null @@ -1,125 +0,0 @@ - - - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - - - third_party\google_benchmark\include\benchmark - - - third_party\google_benchmark\include\benchmark - - - third_party\google_benchmark\include\benchmark - - - third_party\google_benchmark\include\benchmark - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - third_party\google_benchmark\src - - - - - - {7458b63d-7ba4-103d-2bed-3e3ad30d8237} - - - {54a154e8-669b-a7c1-9b6e-bd1aab2f86e3} - - - {f54c3cb1-ec20-a651-6956-78379b51e1a5} - - - {0483a457-8050-4565-bc15-09695bf7b822} - - - {c39ff2d1-691e-4614-4d75-4bc20db05e09} - - - - diff --git a/vsprojects/vcxproj/test/bm_fullstack/bm_fullstack.vcxproj b/vsprojects/vcxproj/test/bm_fullstack/bm_fullstack.vcxproj index 1ce993e3230..3809beb508b 100644 --- a/vsprojects/vcxproj/test/bm_fullstack/bm_fullstack.vcxproj +++ b/vsprojects/vcxproj/test/bm_fullstack/bm_fullstack.vcxproj @@ -164,8 +164,8 @@ - - {AAD4AEF3-DF1E-7A6D-EC35-233BD1031BF4} + + {07978586-E47C-8709-A63E-895FBF3C3C7D} {0BE77741-552A-929B-A497-4EF7ECE17A64} diff --git a/vsprojects/vcxproj/test/noop-benchmark/noop-benchmark.vcxproj b/vsprojects/vcxproj/test/noop-benchmark/noop-benchmark.vcxproj index 99f33b21658..15a82c0ed6c 100644 --- a/vsprojects/vcxproj/test/noop-benchmark/noop-benchmark.vcxproj +++ b/vsprojects/vcxproj/test/noop-benchmark/noop-benchmark.vcxproj @@ -164,8 +164,8 @@ - - {AAD4AEF3-DF1E-7A6D-EC35-233BD1031BF4} + + {07978586-E47C-8709-A63E-895FBF3C3C7D} From 58d68f0d49fb2eaf03f19f6775f4f19a611899bc Mon Sep 17 00:00:00 2001 From: Muxi Yan Date: Wed, 7 Dec 2016 11:20:57 -0800 Subject: [PATCH 46/51] Turn off Cronet logging --- src/core/ext/transport/cronet/transport/cronet_transport.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index c3abb47735a..afc59f4b12a 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -59,7 +59,7 @@ } while (0) /* TODO (makdharma): Hook up into the wider tracing mechanism */ -int grpc_cronet_trace = 1; +int grpc_cronet_trace = 0; enum e_op_result { ACTION_TAKEN_WITH_CALLBACK, From dfd5199e0eaf61f1d729bece3828bc0dbeb4dbf3 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Mon, 5 Dec 2016 16:22:30 -0800 Subject: [PATCH 47/51] Unpin Python setuptools dependency Fixes #8285, allows us to work on later versions of virtualenv without getting problems with too-recent python packaging versions. --- tools/run_tests/build_python.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/run_tests/build_python.sh b/tools/run_tests/build_python.sh index fb884ad1668..7cac3949608 100755 --- a/tools/run_tests/build_python.sh +++ b/tools/run_tests/build_python.sh @@ -171,8 +171,7 @@ pip_install_dir() { } $VENV_PYTHON -m pip install --upgrade pip -# TODO(https://github.com/pypa/setuptools/issues/709) get the latest setuptools -$VENV_PYTHON -m pip install setuptools==25.1.1 +$VENV_PYTHON -m pip install setuptools $VENV_PYTHON -m pip install cython pip_install_dir $ROOT $VENV_PYTHON $ROOT/tools/distrib/python/make_grpcio_tools.py From 3da4188176eb76a1a786249ac840b0a83242e397 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 6 Dec 2016 15:40:19 -0800 Subject: [PATCH 48/51] Fix handling of streams waiting for concurrency --- src/core/ext/transport/chttp2/transport/chttp2_transport.c | 1 + src/core/ext/transport/chttp2/transport/internal.h | 2 ++ src/core/ext/transport/chttp2/transport/stream_lists.c | 5 +++++ 3 files changed, 8 insertions(+) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 3b84898fee0..72711d69dc0 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -1623,6 +1623,7 @@ void grpc_chttp2_mark_stream_closed(grpc_exec_ctx *exec_ctx, remove_stream(exec_ctx, t, s->id, removal_error(GRPC_ERROR_REF(error), s, "Stream removed")); } + grpc_chttp2_list_remove_waiting_for_concurrency(t, s); GRPC_CHTTP2_STREAM_UNREF(exec_ctx, s, "chttp2"); } GRPC_ERROR_UNREF(error); diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 31eb1e01ac0..b727965d435 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -496,6 +496,8 @@ void grpc_chttp2_list_add_waiting_for_concurrency(grpc_chttp2_transport *t, grpc_chttp2_stream *s); int grpc_chttp2_list_pop_waiting_for_concurrency(grpc_chttp2_transport *t, grpc_chttp2_stream **s); +void grpc_chttp2_list_remove_waiting_for_concurrency(grpc_chttp2_transport *t, + grpc_chttp2_stream *s); void grpc_chttp2_list_add_stalled_by_transport(grpc_chttp2_transport *t, grpc_chttp2_stream *s); diff --git a/src/core/ext/transport/chttp2/transport/stream_lists.c b/src/core/ext/transport/chttp2/transport/stream_lists.c index 6d25b3ae579..a60264cc51f 100644 --- a/src/core/ext/transport/chttp2/transport/stream_lists.c +++ b/src/core/ext/transport/chttp2/transport/stream_lists.c @@ -158,6 +158,11 @@ int grpc_chttp2_list_pop_waiting_for_concurrency(grpc_chttp2_transport *t, return stream_list_pop(t, s, GRPC_CHTTP2_LIST_WAITING_FOR_CONCURRENCY); } +void grpc_chttp2_list_remove_waiting_for_concurrency(grpc_chttp2_transport *t, + grpc_chttp2_stream *s) { + stream_list_maybe_remove(t, s, GRPC_CHTTP2_LIST_WAITING_FOR_CONCURRENCY); +} + void grpc_chttp2_list_add_stalled_by_transport(grpc_chttp2_transport *t, grpc_chttp2_stream *s) { stream_list_add(t, s, GRPC_CHTTP2_LIST_STALLED_BY_TRANSPORT); From ec7ba3931741f349b59f531d35677891a0a24baf Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Wed, 7 Dec 2016 10:50:30 -0800 Subject: [PATCH 49/51] PR comments --- src/core/ext/transport/chttp2/transport/chttp2_transport.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 72711d69dc0..6bc054866b1 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -1622,8 +1622,10 @@ void grpc_chttp2_mark_stream_closed(grpc_exec_ctx *exec_ctx, if (s->id != 0) { remove_stream(exec_ctx, t, s->id, removal_error(GRPC_ERROR_REF(error), s, "Stream removed")); + } else { + /* Purge streams waiting on concurrency still waiting for id assignment */ + grpc_chttp2_list_remove_waiting_for_concurrency(t, s); } - grpc_chttp2_list_remove_waiting_for_concurrency(t, s); GRPC_CHTTP2_STREAM_UNREF(exec_ctx, s, "chttp2"); } GRPC_ERROR_UNREF(error); From 9f08d110f84e46cb850a3bad8984689ac08fbbbe Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Mon, 24 Oct 2016 12:25:02 -0700 Subject: [PATCH 50/51] add a "perf" option to benchmarks script, generate profile text reports and flamegraphs --- tools/gce/linux_performance_worker_init.sh | 16 ++ .../process_local_perf_flamegraphs.sh | 40 +++++ .../process_remote_perf_flamegraphs.sh | 44 ++++++ tools/run_tests/report_utils.py | 7 + tools/run_tests/run_performance_tests.py | 141 ++++++++++++++++-- 5 files changed, 233 insertions(+), 15 deletions(-) create mode 100755 tools/run_tests/performance/process_local_perf_flamegraphs.sh create mode 100755 tools/run_tests/performance/process_remote_perf_flamegraphs.sh diff --git a/tools/gce/linux_performance_worker_init.sh b/tools/gce/linux_performance_worker_init.sh index 523749ee814..ab29e015e04 100755 --- a/tools/gce/linux_performance_worker_init.sh +++ b/tools/gce/linux_performance_worker_init.sh @@ -150,3 +150,19 @@ sudo tar -C /usr/local -xzf go$GO_VERSION.$OS-$ARCH.tar.gz # Put go on the PATH, keep the usual installation dir sudo ln -s /usr/local/go/bin/go /usr/bin/go rm go$GO_VERSION.$OS-$ARCH.tar.gz + +# Install perf, to profile benchmarks. (need to get the right linux-tools-<> for kernel version) +sudo apt-get install -y linux-tools-common linux-tools-generic linux-tools-`uname -r` +# see http://unix.stackexchange.com/questions/14227/do-i-need-root-admin-permissions-to-run-userspace-perf-tool-perf-events-ar +echo 0 | sudo tee /proc/sys/kernel/perf_event_paranoid +# see http://stackoverflow.com/questions/21284906/perf-couldnt-record-kernel-reference-relocation-symbol +echo 0 | sudo tee /proc/sys/kernel/kptr_restrict + +# qps workers under perf appear to need a lot of mmap pages under certain scenarios and perf args in +# order to not lose perf events or time out +echo 4096 | sudo tee /proc/sys/kernel/perf_event_mlock_kb + +# Fetch scripts to generate flame graphs from perf data collected +# on benchmarks +git clone -v https://github.com/brendangregg/FlameGraph ~/FlameGraph + diff --git a/tools/run_tests/performance/process_local_perf_flamegraphs.sh b/tools/run_tests/performance/process_local_perf_flamegraphs.sh new file mode 100755 index 00000000000..d15610f1370 --- /dev/null +++ b/tools/run_tests/performance/process_local_perf_flamegraphs.sh @@ -0,0 +1,40 @@ +#!/bin/bash +# Copyright 2015, Google Inc. +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +mkdir -p $OUTPUT_DIR + +PERF_DATA_FILE=${PERF_BASE_NAME}-perf.data +PERF_SCRIPT_OUTPUT=${PERF_BASE_NAME}-out.perf + +# Generate Flame graphs +echo "running perf script on $PERF_DATA_FILE" +perf script -i $PERF_DATA_FILE > $PERF_SCRIPT_OUTPUT + +~/FlameGraph/stackcollapse-perf.pl $PERF_SCRIPT_OUTPUT | ~/FlameGraph/flamegraph.pl > ${OUTPUT_DIR}/${OUTPUT_FILENAME}.svg diff --git a/tools/run_tests/performance/process_remote_perf_flamegraphs.sh b/tools/run_tests/performance/process_remote_perf_flamegraphs.sh new file mode 100755 index 00000000000..cc075354ccf --- /dev/null +++ b/tools/run_tests/performance/process_remote_perf_flamegraphs.sh @@ -0,0 +1,44 @@ +#!/bin/bash +# Copyright 2015, Google Inc. +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +mkdir -p $OUTPUT_DIR + +PERF_DATA_FILE=${PERF_BASE_NAME}-perf.data +PERF_SCRIPT_OUTPUT=${PERF_BASE_NAME}-out.perf + +# Generate Flame graphs +echo "running perf script on $USER_AT_HOST with perf.data" +ssh $USER_AT_HOST "cd ~/performance_workspace/grpc && perf script -i $PERF_DATA_FILE | gzip > ${PERF_SCRIPT_OUTPUT}.gz" + +scp $USER_AT_HOST:~/performance_workspace/grpc/$PERF_SCRIPT_OUTPUT.gz . + +gzip -d -f $PERF_SCRIPT_OUTPUT.gz + +~/FlameGraph/stackcollapse-perf.pl --kernel $PERF_SCRIPT_OUTPUT | ~/FlameGraph/flamegraph.pl --color=java --hash > ${OUTPUT_DIR}/${OUTPUT_FILENAME}.svg diff --git a/tools/run_tests/report_utils.py b/tools/run_tests/report_utils.py index 90055e3530c..5ce2a87cfaf 100644 --- a/tools/run_tests/report_utils.py +++ b/tools/run_tests/report_utils.py @@ -122,3 +122,10 @@ def render_interop_html_report( except: print(exceptions.text_error_template().render()) raise + +def render_perf_profiling_results(output_filepath, profile_names): + with open(output_filepath, 'w') as output_file: + output_file.write('
    \n') + for name in profile_names: + output_file.write('
  • %s
    • \n' % (name, name)) + output_file.write('
\n') diff --git a/tools/run_tests/run_performance_tests.py b/tools/run_tests/run_performance_tests.py index 1d0c98fb690..c2485cf5119 100755 --- a/tools/run_tests/run_performance_tests.py +++ b/tools/run_tests/run_performance_tests.py @@ -49,6 +49,7 @@ import tempfile import time import traceback import uuid +import report_utils _ROOT = os.path.abspath(os.path.join(os.path.dirname(sys.argv[0]), '../..')) @@ -57,15 +58,18 @@ os.chdir(_ROOT) _REMOTE_HOST_USERNAME = 'jenkins' +_PERF_REPORT_OUTPUT_DIR = 'perf_reports' + class QpsWorkerJob: """Encapsulates a qps worker server job.""" - def __init__(self, spec, language, host_and_port): + def __init__(self, spec, language, host_and_port, perf_file_base_name=None): self._spec = spec self.language = language self.host_and_port = host_and_port self._job = None + self.perf_file_base_name = perf_file_base_name def start(self): self._job = jobset.Job(self._spec, newline_on_success=True, travis=True, add_env={}) @@ -80,24 +84,32 @@ class QpsWorkerJob: self._job = None -def create_qpsworker_job(language, shortname=None, - port=10000, remote_host=None): - cmdline = language.worker_cmdline() + ['--driver_port=%s' % port] +def create_qpsworker_job(language, shortname=None, port=10000, remote_host=None, perf_cmd=None): + cmdline = (language.worker_cmdline() + ['--driver_port=%s' % port]) + if remote_host: - user_at_host = '%s@%s' % (_REMOTE_HOST_USERNAME, remote_host) - cmdline = ['ssh', - str(user_at_host), - 'cd ~/performance_workspace/grpc/ && %s' % ' '.join(cmdline)] host_and_port='%s:%s' % (remote_host, port) else: host_and_port='localhost:%s' % port + perf_file_base_name = None + if perf_cmd: + perf_file_base_name = '%s-%s' % (host_and_port, shortname) + # specify -o output file so perf.data gets collected when worker stopped + cmdline = perf_cmd + ['-o', '%s-perf.data' % perf_file_base_name] + cmdline + + if remote_host: + user_at_host = '%s@%s' % (_REMOTE_HOST_USERNAME, remote_host) + ssh_cmd = ['ssh'] + ssh_cmd.extend([str(user_at_host), 'cd ~/performance_workspace/grpc/ && %s' % ' '.join(cmdline)]) + cmdline = ssh_cmd + jobspec = jobset.JobSpec( cmdline=cmdline, shortname=shortname, timeout_seconds=5*60, # workers get restarted after each scenario verbose_success=True) - return QpsWorkerJob(jobspec, language, host_and_port) + return QpsWorkerJob(jobspec, language, host_and_port, perf_file_base_name) def create_scenario_jobspec(scenario_json, workers, remote_host=None, @@ -259,7 +271,7 @@ def build_on_remote_hosts(hosts, languages=scenario_config.LANGUAGES.keys(), bui sys.exit(1) -def create_qpsworkers(languages, worker_hosts): +def create_qpsworkers(languages, worker_hosts, perf_cmd=None): """Creates QPS workers (but does not start them).""" if not worker_hosts: # run two workers locally (for each language) @@ -275,11 +287,32 @@ def create_qpsworkers(languages, worker_hosts): shortname= 'qps_worker_%s_%s' % (language, worker_idx), port=worker[1] + language.worker_port_offset(), - remote_host=worker[0]) + remote_host=worker[0], + perf_cmd=perf_cmd) for language in languages for worker_idx, worker in enumerate(workers)] +def perf_report_processor_job(worker_host, perf_base_name, output_filename): + print('Creating perf report collection job for %s' % worker_host) + cmd = '' + if worker_host != 'localhost': + user_at_host = "%s@%s" % (_REMOTE_HOST_USERNAME, worker_host) + cmd = "USER_AT_HOST=%s OUTPUT_FILENAME=%s OUTPUT_DIR=%s PERF_BASE_NAME=%s\ + tools/run_tests/performance/process_remote_perf_flamegraphs.sh" \ + % (user_at_host, output_filename, _PERF_REPORT_OUTPUT_DIR, perf_base_name) + else: + cmd = "OUTPUT_FILENAME=%s OUTPUT_DIR=%s PERF_BASE_NAME=%s\ + tools/run_tests/performance/process_local_perf_flamegraphs.sh" \ + % (output_filename, _PERF_REPORT_OUTPUT_DIR, perf_base_name) + + return jobset.JobSpec(cmdline=cmd, + timeout_seconds=3*60, + shell=True, + verbose_success=True, + shortname='process perf report') + + Scenario = collections.namedtuple('Scenario', 'jobspec workers name') @@ -372,6 +405,31 @@ def finish_qps_workers(jobs): print('All QPS workers finished.') return num_killed +profile_output_files = [] + +# Collect perf text reports and flamegraphs if perf_cmd was used +# Note the base names of perf text reports are used when creating and processing +# perf data. The scenario name uniqifies the output name in the final +# perf reports directory. +# Alos, the perf profiles need to be fetched and processed after each scenario +# in order to avoid clobbering the output files. +def run_collect_perf_profile_jobs(hosts_and_base_names, scenario_name): + perf_report_jobs = [] + global profile_output_files + for host_and_port in hosts_and_base_names: + perf_base_name = hosts_and_base_names[host_and_port] + output_filename = '%s-%s' % (scenario_name, perf_base_name) + # from the base filename, create .svg output filename + host = host_and_port.split(':')[0] + profile_output_files.append('%s.svg' % output_filename) + perf_report_jobs.append(perf_report_processor_job(host, perf_base_name, output_filename)) + + jobset.message('START', 'Collecting perf reports from qps workers', do_newline=True) + failures, _ = jobset.run(perf_report_jobs, newline_on_success=True, maxjobs=1) + jobset.message('END', 'Collecting perf reports from qps workers', do_newline=True) + return failures + + argp = argparse.ArgumentParser(description='Run performance tests.') argp.add_argument('-l', '--language', choices=['all'] + sorted(scenario_config.LANGUAGES.keys()), @@ -405,6 +463,33 @@ argp.add_argument('--netperf', help='Run netperf benchmark as one of the scenarios.') argp.add_argument('-x', '--xml_report', default='report.xml', type=str, help='Name of XML report file to generate.') +argp.add_argument('--perf_args', + help=('Example usage: "--perf_args=record -F 99 -g". ' + 'Wrap QPS workers in a perf command ' + 'with the arguments to perf specified here. ' + '".svg" flame graph profiles will be ' + 'created for each Qps Worker on each scenario. ' + 'Files will output to "/perf_reports" ' + 'directory. Output files from running the worker ' + 'under perf are saved in the repo root where its ran. ' + 'Note that the perf "-g" flag is necessary for ' + 'flame graphs generation to work (assuming the binary ' + 'being profiled uses frame pointers, check out ' + '"--call-graph dwarf" option using libunwind otherwise.) ' + 'Also note that the entire "--perf_args=" must ' + 'be wrapped in quotes as in the example usage. ' + 'If the "--perg_args" is unspecified, "perf" will ' + 'not be used at all. ' + 'See http://www.brendangregg.com/perf.html ' + 'for more general perf examples.')) +argp.add_argument('--skip_generate_flamegraphs', + default=False, + action='store_const', + const=True, + help=('Turn flame graph generation off. ' + 'May be useful if "perf_args" arguments do not make sense for ' + 'generating flamegraphs (e.g., "--perf_args=stat ...")')) + args = argp.parse_args() @@ -435,7 +520,13 @@ if not args.remote_driver_host: if not args.dry_run: build_on_remote_hosts(remote_hosts, languages=[str(l) for l in languages], build_local=build_local) -qpsworker_jobs = create_qpsworkers(languages, args.remote_worker_host) +perf_cmd = None +if args.perf_args: + # Expect /usr/bin/perf to be installed here, as is usual + perf_cmd = ['/usr/bin/perf'] + perf_cmd.extend(re.split('\s+', args.perf_args)) + +qpsworker_jobs = create_qpsworkers(languages, args.remote_worker_host, perf_cmd=perf_cmd) # get list of worker addresses for each language. workers_by_lang = dict([(str(language), []) for language in languages]) @@ -457,10 +548,13 @@ if not scenarios: total_scenario_failures = 0 qps_workers_killed = 0 merged_resultset = {} +perf_report_failures = 0 + for scenario in scenarios: if args.dry_run: print(scenario.name) else: + scenario_failures = 0 try: for worker in scenario.workers: worker.start() @@ -474,10 +568,27 @@ for scenario in scenarios: # Consider qps workers that need to be killed as failures qps_workers_killed += finish_qps_workers(scenario.workers) + if perf_cmd and scenario_failures == 0 and not args.skip_generate_flamegraphs: + workers_and_base_names = {} + for worker in scenario.workers: + if not worker.perf_file_base_name: + raise Exception('using perf buf perf report filename is unspecified') + workers_and_base_names[worker.host_and_port] = worker.perf_file_base_name + perf_report_failures += run_collect_perf_profile_jobs(workers_and_base_names, scenario.name) + -report_utils.render_junit_xml_report(merged_resultset, args.xml_report, - suite_name='benchmarks') +# Still write the index.html even if some scenarios failed. +# 'profile_output_files' will only have names for scenarios that passed +if perf_cmd and not args.skip_generate_flamegraphs: + # write the index fil to the output dir, with all profiles from all scenarios/workers + report_utils.render_perf_profiling_results('%s/index.html' % _PERF_REPORT_OUTPUT_DIR, profile_output_files) if total_scenario_failures > 0 or qps_workers_killed > 0: - print ("%s scenarios failed and %s qps worker jobs killed" % (total_scenario_failures, qps_workers_killed)) + print('%s scenarios failed and %s qps worker jobs killed' % (total_scenario_failures, qps_workers_killed)) + sys.exit(1) + +report_utils.render_junit_xml_report(merged_resultset, args.xml_report, + suite_name='benchmarks') +if perf_report_failures > 0: + print('%s perf profile collection jobs failed' % perf_report_failures) sys.exit(1) From 9fe161c789216591edf2f73b75d06b6d720aa605 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Fri, 9 Dec 2016 12:04:34 +0100 Subject: [PATCH 51/51] readable output from run_tests.py on windows --- tools/run_tests/jobset.py | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tools/run_tests/jobset.py b/tools/run_tests/jobset.py index 2acc7971f62..1b5d6d66a09 100755 --- a/tools/run_tests/jobset.py +++ b/tools/run_tests/jobset.py @@ -139,16 +139,16 @@ def message(tag, msg, explanatory_text=None, do_newline=False): if explanatory_text: print(explanatory_text) print('%s: %s' % (tag, msg)) - return - sys.stdout.write('%s%s%s\x1b[%d;%dm%s\x1b[0m: %s%s' % ( - _BEGINNING_OF_LINE, - _CLEAR_LINE, - '\n%s' % explanatory_text if explanatory_text is not None else '', - _COLORS[_TAG_COLOR[tag]][1], - _COLORS[_TAG_COLOR[tag]][0], - tag, - msg, - '\n' if do_newline or explanatory_text is not None else '')) + else: + sys.stdout.write('%s%s%s\x1b[%d;%dm%s\x1b[0m: %s%s' % ( + _BEGINNING_OF_LINE, + _CLEAR_LINE, + '\n%s' % explanatory_text if explanatory_text is not None else '', + _COLORS[_TAG_COLOR[tag]][1], + _COLORS[_TAG_COLOR[tag]][0], + tag, + msg, + '\n' if do_newline or explanatory_text is not None else '')) sys.stdout.flush() except: pass @@ -406,7 +406,7 @@ class Jobset(object): self.resultset[job.GetSpec().shortname].append(job.result) self._running.remove(job) if dead: return - if (not self._travis): + if not self._travis and platform_string() != 'windows': rstr = '' if self._remaining is None else '%d queued, ' % self._remaining if self._remaining is not None and self._completed > 0: now = time.time()