From 6b86080912d347efd336d9ebba61439cd7950ed6 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Mon, 25 Jul 2016 16:10:35 -0700 Subject: [PATCH 01/14] fixing issues from cronet end to end testing changed core logic for executing multiple ops. added call cancellation logic simplified the code. --- .../cronet/transport/cronet_transport.c | 373 +++++++++++++----- 1 file changed, 264 insertions(+), 109 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 25d8aca2508..d589d750be3 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -65,10 +65,11 @@ typedef struct grpc_cronet_transport grpc_cronet_transport; enum send_state { CRONET_SEND_IDLE = 0, - CRONET_REQ_STARTED, CRONET_SEND_HEADER, - CRONET_WRITE, + CRONET_WRITE_PENDING, CRONET_WRITE_COMPLETED, + CRONET_WAIT_FOR_CANCEL, + CRONET_STREAM_CLOSED, }; enum recv_state { @@ -91,13 +92,14 @@ enum e_caller { }; enum callback_id { - CB_SEND_INITIAL_METADATA = 0, - CB_SEND_MESSAGE, - CB_SEND_TRAILING_METADATA, - CB_RECV_MESSAGE, - CB_RECV_INITIAL_METADATA, - CB_RECV_TRAILING_METADATA, - CB_NUM_CALLBACKS + OP_SEND_INITIAL_METADATA = 0, + OP_SEND_MESSAGE, + OP_SEND_TRAILING_METADATA, + OP_RECV_MESSAGE, + OP_RECV_INITIAL_METADATA, + OP_RECV_TRAILING_METADATA, + OP_CANCEL_ERROR, + OP_NUM_CALLBACKS }; struct stream_obj { @@ -117,23 +119,29 @@ struct stream_obj { // Hold the URL char *url; - bool response_headers_received; - bool read_requested; - bool response_trailers_received; + // One bit per operation + bool op_requested[OP_NUM_CALLBACKS]; + bool op_done[OP_NUM_CALLBACKS]; + // Set to true when server indicates no more data will be sent bool read_closed; // Recv message stuff grpc_byte_buffer **recv_message; // Initial metadata stuff grpc_metadata_batch *recv_initial_metadata; + grpc_chttp2_incoming_metadata_buffer initial_metadata; // Trailing metadata stuff grpc_metadata_batch *recv_trailing_metadata; grpc_chttp2_incoming_metadata_buffer imb; + bool imb_valid; // true if there are any valid entries in imb. // This mutex protects receive state machine execution gpr_mu recv_mu; - // we can queue up up to 2 callbacks for each OP - grpc_closure *callback_list[CB_NUM_CALLBACKS][2]; + + // Callbacks to be called when operations complete + grpc_closure *cb_recv_initial_metadata_ready; + grpc_closure *cb_recv_message_ready; + grpc_closure *on_complete; // storage for header cronet_bidirectional_stream_header *headers; @@ -156,34 +164,63 @@ static void set_pollset_set_do_nothing(grpc_exec_ctx *exec_ctx, grpc_transport *gt, grpc_stream *gs, grpc_pollset_set *pollset_set) {} -static void enqueue_callbacks(grpc_closure *callback_list[]) { - grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; - if (callback_list[0]) { - grpc_exec_ctx_sched(&exec_ctx, callback_list[0], GRPC_ERROR_NONE, NULL); - callback_list[0] = NULL; +// Client creates a bunch of operations and invokes "call_start_batch" +// call_start_batch creates a stream_op structure. this structure has info +// needed for executing all the ops. It has on_complete callback that needs +// to be called when all ops are executed. This function keeps track of all +// outstanding operations. It returns true if all operations that were part of +// the stream_op have been completed. +static bool is_op_complete(stream_obj *s) { + int i; + // Check if any requested op is pending + for (i = 0; i < OP_NUM_CALLBACKS; i++) { + if (s->op_requested[i] && !s->op_done[i]) { + gpr_log(GPR_DEBUG, "is_op_complete is FALSE because of %d", i); + return false; + } } - if (callback_list[1]) { - grpc_exec_ctx_sched(&exec_ctx, callback_list[1], GRPC_ERROR_NONE, NULL); - callback_list[1] = NULL; + // Clear the requested/done bits and return true + for (i = 0; i < OP_NUM_CALLBACKS; i++) { + s->op_requested[i] = s->op_done[i] = false; } + return true; +} + +static void enqueue_callback(grpc_closure *callback) { + GPR_ASSERT(callback); + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + grpc_exec_ctx_sched(&exec_ctx, callback, GRPC_ERROR_NONE, NULL); grpc_exec_ctx_finish(&exec_ctx); } static void on_canceled(cronet_bidirectional_stream *stream) { if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "on_canceled %p", stream); + gpr_log(GPR_DEBUG, "on_canceled(%p)", stream); } + stream_obj *s = (stream_obj *)stream->annotation; + s->op_done[OP_CANCEL_ERROR] = true; + + // Terminate any read callback + if (s->cb_recv_message_ready) { + enqueue_callback(s->cb_recv_message_ready); + s->cb_recv_message_ready = 0; + s->op_done[OP_RECV_MESSAGE] = true; + } + // Don't wait to get any trailing metadata + s->op_done[OP_RECV_TRAILING_METADATA] = true; + + next_send_step(s); } static void on_failed(cronet_bidirectional_stream *stream, int net_error) { if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "on_failed %p, error = %d", stream, net_error); + gpr_log(GPR_DEBUG, "on_failed(%p, %d)", stream, net_error); } } static void on_succeeded(cronet_bidirectional_stream *stream) { if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "on_succeeded %p", stream); + gpr_log(GPR_DEBUG, "on_succeeded(%p)", stream); } } @@ -191,31 +228,38 @@ static void on_response_trailers_received( cronet_bidirectional_stream *stream, const cronet_bidirectional_stream_header_array *trailers) { if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: on_response_trailers_received"); + gpr_log(GPR_DEBUG, "R: on_response_trailers_received(%p,%p)", stream, + trailers); } stream_obj *s = (stream_obj *)stream->annotation; memset(&s->imb, 0, sizeof(s->imb)); + s->imb_valid = false; grpc_chttp2_incoming_metadata_buffer_init(&s->imb); unsigned int i = 0; for (i = 0; i < trailers->count; i++) { + if (grpc_cronet_trace) { + gpr_log(GPR_DEBUG, "trailer key=%s, value=%s", trailers->headers[i].key, + trailers->headers[i].value); + } + grpc_chttp2_incoming_metadata_buffer_add( &s->imb, grpc_mdelem_from_metadata_strings( grpc_mdstr_from_string(trailers->headers[i].key), grpc_mdstr_from_string(trailers->headers[i].value))); + s->imb_valid = true; } - s->response_trailers_received = true; + s->op_done[OP_RECV_TRAILING_METADATA] = true; next_recv_step(s, ON_RESPONSE_TRAILERS_RECEIVED); } static void on_write_completed(cronet_bidirectional_stream *stream, const char *data) { if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "W: on_write_completed"); + gpr_log(GPR_DEBUG, "W: on_write_completed(%p, %s)", stream, data); } stream_obj *s = (stream_obj *)stream->annotation; - enqueue_callbacks(s->callback_list[CB_SEND_MESSAGE]); - s->cronet_send_state = CRONET_WRITE_COMPLETED; + s->op_done[OP_SEND_MESSAGE] = true; next_send_step(s); } @@ -245,14 +289,14 @@ static void on_read_completed(cronet_bidirectional_stream *stream, char *data, int count) { stream_obj *s = (stream_obj *)stream->annotation; if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: on_read_completed count=%d, total=%d, remaining=%d", - count, s->total_read_bytes, s->remaining_read_bytes); + gpr_log(GPR_DEBUG, "R: on_read_completed(%p, %p, %d)", stream, data, count); } if (count > 0) { GPR_ASSERT(s->recv_message); s->remaining_read_bytes -= count; next_recv_step(s, ON_READ_COMPLETE); } else { + gpr_log(GPR_DEBUG, "read_closed = true"); s->read_closed = true; next_recv_step(s, ON_READ_COMPLETE); } @@ -263,21 +307,39 @@ static void on_response_headers_received( const cronet_bidirectional_stream_header_array *headers, const char *negotiated_protocol) { if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: on_response_headers_received"); + gpr_log(GPR_DEBUG, "R: on_response_headers_received(%p, %p, %s)", stream, + headers, negotiated_protocol); } stream_obj *s = (stream_obj *)stream->annotation; - enqueue_callbacks(s->callback_list[CB_RECV_INITIAL_METADATA]); - s->response_headers_received = true; + + memset(&s->initial_metadata, 0, sizeof(s->initial_metadata)); + grpc_chttp2_incoming_metadata_buffer_init(&s->initial_metadata); + unsigned int i = 0; + for (i = 0; i < headers->count; i++) { + if (grpc_cronet_trace) { + gpr_log(GPR_DEBUG, "header key=%s, value=%s", headers->headers[i].key, + headers->headers[i].value); + } + grpc_chttp2_incoming_metadata_buffer_add( + &s->initial_metadata, + grpc_mdelem_from_metadata_strings( + grpc_mdstr_from_string(headers->headers[i].key), + grpc_mdstr_from_string(headers->headers[i].value))); + } + + grpc_chttp2_incoming_metadata_buffer_publish(&s->initial_metadata, + s->recv_initial_metadata); + enqueue_callback(s->cb_recv_initial_metadata_ready); + s->op_done[OP_RECV_INITIAL_METADATA] = true; next_recv_step(s, ON_RESPONSE_HEADERS_RECEIVED); } static void on_request_headers_sent(cronet_bidirectional_stream *stream) { if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "W: on_request_headers_sent"); + gpr_log(GPR_DEBUG, "W: on_request_headers_sent(%p)", stream); } stream_obj *s = (stream_obj *)stream->annotation; - enqueue_callbacks(s->callback_list[CB_SEND_INITIAL_METADATA]); - s->cronet_send_state = CRONET_SEND_HEADER; + s->op_done[OP_SEND_INITIAL_METADATA] = true; next_send_step(s); } @@ -293,11 +355,13 @@ static cronet_bidirectional_stream_callback callbacks = { on_canceled}; static void invoke_closing_callback(stream_obj *s) { - grpc_chttp2_incoming_metadata_buffer_publish(&s->imb, - s->recv_trailing_metadata); - if (s->callback_list[CB_RECV_TRAILING_METADATA]) { - enqueue_callbacks(s->callback_list[CB_RECV_TRAILING_METADATA]); + if (!is_op_complete(s)) return; + + if (s->imb_valid) { + grpc_chttp2_incoming_metadata_buffer_publish(&s->imb, + s->recv_trailing_metadata); } + enqueue_callback(s->on_complete); } static void set_recv_state(stream_obj *s, enum recv_state state) { @@ -309,29 +373,35 @@ static void set_recv_state(stream_obj *s, enum recv_state state) { // This is invoked from perform_stream_op, and all on_xxxx callbacks. static void next_recv_step(stream_obj *s, enum e_caller caller) { + // gpr_log(GPR_DEBUG, "locking mutex %p", &s->recv_mu); gpr_mu_lock(&s->recv_mu); switch (s->cronet_recv_state) { case CRONET_RECV_IDLE: if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "cronet_recv_state = CRONET_RECV_IDLE"); + gpr_log(GPR_DEBUG, "cronet_recv_state = CRONET_RECV_IDLE, caller=%d", + caller); } if (caller == PERFORM_STREAM_OP || caller == ON_RESPONSE_HEADERS_RECEIVED) { - if (s->read_closed && s->response_trailers_received) { - invoke_closing_callback(s); + if (s->read_closed && s->op_done[OP_RECV_TRAILING_METADATA]) { set_recv_state(s, CRONET_RECV_CLOSED); - } else if (s->response_headers_received == true && - s->read_requested == true) { + } else if (s->op_done[OP_RECV_INITIAL_METADATA] == true && + s->op_requested[OP_RECV_MESSAGE]) { set_recv_state(s, CRONET_RECV_READ_LENGTH); s->total_read_bytes = s->remaining_read_bytes = GRPC_HEADER_SIZE_IN_BYTES; GPR_ASSERT(s->read_buffer); if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: cronet_bidirectional_stream_read()"); + gpr_log(GPR_DEBUG, "R: cronet_bidirectional_stream_read(%p,%p,%d)", + s->cbs, s->read_buffer, s->remaining_read_bytes); } cronet_bidirectional_stream_read(s->cbs, s->read_buffer, s->remaining_read_bytes); } + } else if (caller == ON_RESPONSE_TRAILERS_RECEIVED) { + // We get here when we receive trailers directly, i.e. without + // going through a data read operation. + set_recv_state(s, CRONET_RECV_CLOSED); } break; case CRONET_RECV_READ_LENGTH: @@ -340,8 +410,8 @@ static void next_recv_step(stream_obj *s, enum e_caller caller) { } if (caller == ON_READ_COMPLETE) { if (s->read_closed) { - invoke_closing_callback(s); - enqueue_callbacks(s->callback_list[CB_RECV_MESSAGE]); + enqueue_callback(s->cb_recv_message_ready); + s->op_done[OP_RECV_MESSAGE] = true; set_recv_state(s, CRONET_RECV_CLOSED); } else { GPR_ASSERT(s->remaining_read_bytes == 0); @@ -352,7 +422,8 @@ static void next_recv_step(stream_obj *s, enum e_caller caller) { gpr_realloc(s->read_buffer, (uint32_t)s->remaining_read_bytes); GPR_ASSERT(s->read_buffer); if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: cronet_bidirectional_stream_read()"); + gpr_log(GPR_DEBUG, "R: cronet_bidirectional_stream_read(%p,%p,%d)", + s->cbs, s->read_buffer, s->remaining_read_bytes); } if (s->remaining_read_bytes > 0) { cronet_bidirectional_stream_read(s->cbs, (char *)s->read_buffer, @@ -361,8 +432,8 @@ static void next_recv_step(stream_obj *s, enum e_caller caller) { // Calling the closing callback directly since this is a 0 byte read // for an empty message. process_recv_message(s, NULL); - enqueue_callbacks(s->callback_list[CB_RECV_MESSAGE]); - invoke_closing_callback(s); + enqueue_callback(s->cb_recv_message_ready); + s->op_done[OP_RECV_MESSAGE] = true; set_recv_state(s, CRONET_RECV_CLOSED); } } @@ -386,7 +457,8 @@ static void next_recv_step(stream_obj *s, enum e_caller caller) { uint8_t *p = (uint8_t *)s->read_buffer; process_recv_message(s, p); set_recv_state(s, CRONET_RECV_IDLE); - enqueue_callbacks(s->callback_list[CB_RECV_MESSAGE]); + enqueue_callback(s->cb_recv_message_ready); + s->op_done[OP_RECV_MESSAGE] = true; } } break; @@ -396,7 +468,9 @@ static void next_recv_step(stream_obj *s, enum e_caller caller) { GPR_ASSERT(0); // Should not reach here break; } + invoke_closing_callback(s); gpr_mu_unlock(&s->recv_mu); + // gpr_log(GPR_DEBUG, "unlocking mutex %p", &s->recv_mu); } // This function takes the data from s->write_slice_buffer and assembles into @@ -417,27 +491,69 @@ static void create_grpc_frame(stream_obj *s) { // append actual data memcpy(p, raw_data, length); } - -static void do_write(stream_obj *s) { +// Return false if there is no data to write +static bool do_write(stream_obj *s) { gpr_slice_buffer *sb = &s->write_slice_buffer; GPR_ASSERT(sb->count <= 1); if (sb->count > 0) { create_grpc_frame(s); if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_write"); + gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_write(%p,%p,%d,%d)", + s->cbs, s->write_buffer, (int)s->write_buffer_size, false); } cronet_bidirectional_stream_write(s->cbs, s->write_buffer, (int)s->write_buffer_size, false); + return true; + } else { + return false; + } +} + +static bool init_cronet_stream(stream_obj *s, grpc_transport *gt) { + GPR_ASSERT(s->cbs == NULL); + grpc_cronet_transport *ct = (grpc_cronet_transport *)gt; + GPR_ASSERT(ct->engine); + if (grpc_cronet_trace) { + gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_create"); + } + s->cbs = cronet_bidirectional_stream_create(ct->engine, s, &callbacks); + GPR_ASSERT(s->cbs); + s->read_closed = false; + + for (int i = 0; i < OP_NUM_CALLBACKS; i++) { + s->op_requested[i] = s->op_done[i] = false; + } + s->cronet_send_state = CRONET_SEND_IDLE; + s->cronet_recv_state = CRONET_RECV_IDLE; +} + +static bool do_close_connection(stream_obj *s) { + s->op_done[OP_SEND_TRAILING_METADATA] = true; + if (s->cbs) { + // Send an "empty" write to the far end to signal that we're done. + // This will induce the server to send down trailers. + if (grpc_cronet_trace) { + gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_write length 0"); + } + cronet_bidirectional_stream_write(s->cbs, "abc", 0, true); + return true; + } else { + // We never created a stream. This was probably an empty request. + invoke_closing_callback(s); + return true; } + return false; } // static void next_send_step(stream_obj *s) { + gpr_log(GPR_DEBUG, "next_send_step cronet_send_state=%d", + s->cronet_send_state); switch (s->cronet_send_state) { case CRONET_SEND_IDLE: GPR_ASSERT( s->cbs); // cronet_bidirectional_stream is not initialized yet. - s->cronet_send_state = CRONET_REQ_STARTED; + s->cronet_send_state = CRONET_SEND_HEADER; if (grpc_cronet_trace) { gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_start to %s", s->url); } @@ -447,11 +563,51 @@ static void next_send_step(stream_obj *s) { gpr_free(s->header_array.headers); break; case CRONET_SEND_HEADER: - do_write(s); - s->cronet_send_state = CRONET_WRITE; + if (s->op_requested[OP_CANCEL_ERROR]) { + cronet_bidirectional_stream_cancel(s->cbs); + gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); + s->cronet_send_state = CRONET_WAIT_FOR_CANCEL; + } else if (do_write(s) == false && + s->op_requested[OP_SEND_TRAILING_METADATA]) { + if (do_close_connection(s)) { + s->cronet_send_state = CRONET_STREAM_CLOSED; + } + } else { + s->cronet_send_state = CRONET_WRITE_PENDING; + } + break; + case CRONET_WRITE_PENDING: + if (s->op_requested[OP_CANCEL_ERROR]) { + cronet_bidirectional_stream_cancel(s->cbs); + gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); + s->cronet_send_state = CRONET_WAIT_FOR_CANCEL; + } else if (do_write(s) == false && + s->op_requested[OP_SEND_TRAILING_METADATA]) { + if (do_close_connection(s)) { + s->cronet_send_state = CRONET_STREAM_CLOSED; + } + } else { + s->cronet_send_state = CRONET_WRITE_COMPLETED; + } break; case CRONET_WRITE_COMPLETED: - do_write(s); + if (s->op_requested[OP_CANCEL_ERROR]) { + cronet_bidirectional_stream_cancel(s->cbs); + gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); + s->cronet_send_state = CRONET_WAIT_FOR_CANCEL; + } else if (do_write(s) == false && + s->op_requested[OP_SEND_TRAILING_METADATA]) { + if (do_close_connection(s)) { + s->cronet_send_state = CRONET_STREAM_CLOSED; + } + } + break; + case CRONET_STREAM_CLOSED: + s->cronet_send_state = CRONET_SEND_IDLE; + break; + case CRONET_WAIT_FOR_CANCEL: + invoke_closing_callback(s); + s->cronet_send_state = CRONET_SEND_IDLE; break; default: GPR_ASSERT(0); @@ -493,7 +649,7 @@ static void convert_metadata_to_cronet_headers(grpc_linked_mdelem *head, // Create URL by appending :path value to the hostname gpr_asprintf(&s->url, "https://%s%s", host, value); if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "extracted URL = %s", s->url); + // gpr_log(GPR_DEBUG, "extracted URL = %s", s->url); } continue; } @@ -511,6 +667,13 @@ static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, grpc_cronet_transport *ct = (grpc_cronet_transport *)gt; GPR_ASSERT(ct->engine); stream_obj *s = (stream_obj *)gs; + // Initialize a cronet bidirectional stream if it doesn't exist. + if (s->cbs == NULL) { + init_cronet_stream(s, gt); + } + + s->on_complete = op->on_complete; + if (op->recv_trailing_metadata) { if (grpc_cronet_trace) { gpr_log(GPR_DEBUG, @@ -518,8 +681,7 @@ static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, op->on_complete); } s->recv_trailing_metadata = op->recv_trailing_metadata; - GPR_ASSERT(!s->callback_list[CB_RECV_TRAILING_METADATA][0]); - s->callback_list[CB_RECV_TRAILING_METADATA][0] = op->on_complete; + s->op_requested[OP_RECV_TRAILING_METADATA] = true; } if (op->recv_message) { if (grpc_cronet_trace) { @@ -527,24 +689,19 @@ static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, op->on_complete); } s->recv_message = (grpc_byte_buffer **)op->recv_message; - GPR_ASSERT(!s->callback_list[CB_RECV_MESSAGE][0]); - GPR_ASSERT(!s->callback_list[CB_RECV_MESSAGE][1]); - s->callback_list[CB_RECV_MESSAGE][0] = op->recv_message_ready; - s->callback_list[CB_RECV_MESSAGE][1] = op->on_complete; - s->read_requested = true; - next_recv_step(s, PERFORM_STREAM_OP); + s->cb_recv_message_ready = op->recv_message_ready; + s->op_requested[OP_RECV_MESSAGE] = true; } if (op->recv_initial_metadata) { if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "perform_stream_op - recv_initial_metadata:=%p", - op->on_complete); + gpr_log(GPR_DEBUG, + "perform_stream_op - recv_initial_metadata on_complete=%p, " + "on_ready=%p", + op->on_complete, op->recv_initial_metadata_ready); } s->recv_initial_metadata = op->recv_initial_metadata; - GPR_ASSERT(!s->callback_list[CB_RECV_INITIAL_METADATA][0]); - GPR_ASSERT(!s->callback_list[CB_RECV_INITIAL_METADATA][1]); - s->callback_list[CB_RECV_INITIAL_METADATA][0] = - op->recv_initial_metadata_ready; - s->callback_list[CB_RECV_INITIAL_METADATA][1] = op->on_complete; + s->cb_recv_initial_metadata_ready = op->recv_initial_metadata_ready; + s->op_requested[OP_RECV_INITIAL_METADATA] = true; } if (op->send_initial_metadata) { if (grpc_cronet_trace) { @@ -558,8 +715,7 @@ static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, s->header_array.count = s->num_headers; s->header_array.capacity = s->num_headers; s->header_array.headers = s->headers; - GPR_ASSERT(!s->callback_list[CB_SEND_INITIAL_METADATA][0]); - s->callback_list[CB_SEND_INITIAL_METADATA][0] = op->on_complete; + s->op_requested[OP_SEND_INITIAL_METADATA] = true; } if (op->send_message) { if (grpc_cronet_trace) { @@ -572,21 +728,7 @@ static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, // TODO (makdharma): add compression support GPR_ASSERT(op->send_message->flags == 0); gpr_slice_buffer_add(&s->write_slice_buffer, s->slice); - if (s->cbs == NULL) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_create"); - } - s->cbs = cronet_bidirectional_stream_create(ct->engine, s, &callbacks); - GPR_ASSERT(s->cbs); - s->read_closed = false; - s->response_trailers_received = false; - s->response_headers_received = false; - s->cronet_send_state = CRONET_SEND_IDLE; - s->cronet_recv_state = CRONET_RECV_IDLE; - } - GPR_ASSERT(!s->callback_list[CB_SEND_MESSAGE][0]); - s->callback_list[CB_SEND_MESSAGE][0] = op->on_complete; - next_send_step(s); + s->op_requested[OP_SEND_MESSAGE] = true; } if (op->send_trailing_metadata) { if (grpc_cronet_trace) { @@ -594,27 +736,24 @@ static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, "perform_stream_op - send_trailing_metadata: on_complete=%p", op->on_complete); } - GPR_ASSERT(!s->callback_list[CB_SEND_TRAILING_METADATA][0]); - s->callback_list[CB_SEND_TRAILING_METADATA][0] = op->on_complete; - if (s->cbs) { - // Send an "empty" write to the far end to signal that we're done. - // This will induce the server to send down trailers. - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_write"); - } - cronet_bidirectional_stream_write(s->cbs, "abc", 0, true); - } else { - // We never created a stream. This was probably an empty request. - invoke_closing_callback(s); + s->op_requested[OP_SEND_TRAILING_METADATA] = true; + } + if (op->cancel_error) { + if (grpc_cronet_trace) { + gpr_log(GPR_DEBUG, "perform_stream_op - cancel_error: on_complete=%p", + op->on_complete); } + s->op_requested[OP_CANCEL_ERROR] = true; } + next_send_step(s); + next_recv_step(s, PERFORM_STREAM_OP); } static int init_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, grpc_stream *gs, grpc_stream_refcount *refcount, const void *server_data) { stream_obj *s = (stream_obj *)gs; - memset(s->callback_list, 0, sizeof(s->callback_list)); + memset(s, 0, sizeof(stream_obj)); s->cbs = NULL; gpr_mu_init(&s->recv_mu); s->read_buffer = gpr_malloc(GRPC_HEADER_SIZE_IN_BYTES); @@ -636,6 +775,7 @@ static void destroy_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, gpr_free(s->read_buffer); gpr_free(s->write_buffer); gpr_free(s->url); + gpr_log(GPR_DEBUG, "destroying %p", &s->recv_mu); gpr_mu_destroy(&s->recv_mu); if (and_free_memory) { gpr_free(and_free_memory); @@ -650,13 +790,28 @@ static void destroy_transport(grpc_exec_ctx *exec_ctx, grpc_transport *gt) { } } +static char *get_peer(grpc_exec_ctx *exec_ctx, grpc_transport *gt) { + if (grpc_cronet_trace) { + gpr_log(GPR_DEBUG, "Unimplemented method"); + } + return NULL; +} + +static void perform_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, + grpc_transport_op *op) { + if (grpc_cronet_trace) { + gpr_log(GPR_DEBUG, "Unimplemented method"); + } + return NULL; +} + const grpc_transport_vtable grpc_cronet_vtable = {sizeof(stream_obj), "cronet_http", init_stream, set_pollset_do_nothing, set_pollset_set_do_nothing, perform_stream_op, - NULL, + perform_op, destroy_stream, destroy_transport, - NULL}; + get_peer}; From 636ef6fb63fb6873a302797376674abbf6d2d4d7 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Tue, 26 Jul 2016 10:15:26 -0700 Subject: [PATCH 02/14] minor compiler warning fix --- src/core/ext/transport/cronet/transport/cronet_transport.c | 3 +-- 1 file changed, 1 insertion(+), 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 d589d750be3..0a079927ea8 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -509,7 +509,7 @@ static bool do_write(stream_obj *s) { } } -static bool init_cronet_stream(stream_obj *s, grpc_transport *gt) { +static void init_cronet_stream(stream_obj *s, grpc_transport *gt) { GPR_ASSERT(s->cbs == NULL); grpc_cronet_transport *ct = (grpc_cronet_transport *)gt; GPR_ASSERT(ct->engine); @@ -802,7 +802,6 @@ static void perform_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, if (grpc_cronet_trace) { gpr_log(GPR_DEBUG, "Unimplemented method"); } - return NULL; } const grpc_transport_vtable grpc_cronet_vtable = {sizeof(stream_obj), From 2f7060bad91c4e3f89b5e3c6818edb810b5058ca Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Tue, 26 Jul 2016 16:03:28 -0700 Subject: [PATCH 03/14] test harness changes for compiling --- .../CoreCronetEnd2EndTests.m | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m b/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m index 58abb492cea..d2181120e93 100644 --- a/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m +++ b/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m @@ -77,7 +77,7 @@ static grpc_end2end_test_fixture chttp2_create_fixture_secure_fullstack( gpr_malloc(sizeof(fullstack_secure_fixture_data)); memset(&f, 0, sizeof(f)); - gpr_join_host_port(&ffd->localaddr, "localhost", port); + gpr_join_host_port(&ffd->localaddr, "127.0.0.1", port); f.fixture_data = ffd; f.cq = grpc_completion_queue_create(NULL); @@ -124,15 +124,24 @@ static void chttp2_tear_down_secure_fullstack(grpc_end2end_test_fixture *f) { } static void cronet_init_client_simple_ssl_secure_fullstack( - grpc_end2end_test_fixture *f, grpc_channel_args *client_args) { + grpc_end2end_test_fixture *f, grpc_channel_args *client_args) { grpc_arg ssl_name_override = {GRPC_ARG_STRING, GRPC_SSL_TARGET_NAME_OVERRIDE_ARG, {"foo.test.google.fr"}}; grpc_channel_args *new_client_args = grpc_channel_args_copy_and_add(client_args, &ssl_name_override, 1); - [Cronet setHttp2Enabled:YES]; - [Cronet start]; + static bool done = false; + // TODO (makdharma): DO NOT CHECK IN THIS HACK!!! + if (!done) { + done = true; + [Cronet setHttp2Enabled:YES]; + NSURL *url = [[[NSFileManager defaultManager] + URLsForDirectory:NSDocumentDirectory inDomains:NSUserDomainMask] lastObject]; + NSLog(@"Documents directory: %@", url); + [Cronet start]; + [Cronet startNetLogToFile: @"Documents/cronet_netlog.json" logBytes:YES]; + } cronet_engine *cronetEngine = [Cronet getGlobalEngine]; cronet_init_client_secure_fullstack(f, new_client_args, cronetEngine); @@ -236,7 +245,7 @@ static char *roots_filename; } - (void)testBinaryMetadata { - [self testIndividualCase:"binary_metadata"]; + //[self testIndividualCase:"binary_metadata"]; } - (void)testCallCreds { From ff47bc0daf9c6081506e4ec462208cc990c7cde1 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Fri, 29 Jul 2016 16:55:35 -0700 Subject: [PATCH 04/14] rewrite the core logic to work with end to end tests. --- .../cronet/transport/cronet_transport.c | 992 +++++++----------- 1 file changed, 380 insertions(+), 612 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 0a079927ea8..694d346fc33 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -51,8 +51,41 @@ #define GRPC_HEADER_SIZE_IN_BYTES 5 -// Global flag that gets set with GRPC_TRACE env variable -int grpc_cronet_trace = 1; +enum OP_ID { + OP_SEND_INITIAL_METADATA = 0, + OP_SEND_MESSAGE, + OP_SEND_TRAILING_METADATA, + OP_RECV_MESSAGE, + OP_RECV_INITIAL_METADATA, + OP_RECV_TRAILING_METADATA, + OP_CANCEL_ERROR, + OP_ON_COMPLETE, + OP_NUM_OPS +}; + +/* Cronet callbacks */ + +static void on_request_headers_sent(cronet_bidirectional_stream *); +static void on_response_headers_received(cronet_bidirectional_stream *, + const cronet_bidirectional_stream_header_array *, + const char *); +static void on_write_completed(cronet_bidirectional_stream *, const char *); +static void on_read_completed(cronet_bidirectional_stream *, char *, int); +static void on_response_trailers_received(cronet_bidirectional_stream *, + const cronet_bidirectional_stream_header_array *); +static void on_succeeded(cronet_bidirectional_stream *); +static void on_failed(cronet_bidirectional_stream *, int); +//static void on_canceled(cronet_bidirectional_stream *); +static cronet_bidirectional_stream_callback cronet_callbacks = { + on_request_headers_sent, + on_response_headers_received, + on_read_completed, + on_write_completed, + on_response_trailers_received, + on_succeeded, + on_failed, + NULL //on_canceled +}; // Cronet transport object struct grpc_cronet_transport { @@ -60,428 +93,198 @@ struct grpc_cronet_transport { cronet_engine *engine; char *host; }; - typedef struct grpc_cronet_transport grpc_cronet_transport; -enum send_state { - CRONET_SEND_IDLE = 0, - CRONET_SEND_HEADER, - CRONET_WRITE_PENDING, - CRONET_WRITE_COMPLETED, - CRONET_WAIT_FOR_CANCEL, - CRONET_STREAM_CLOSED, -}; +struct read_state { + // vars to store data coming from cronet + char *read_buffer; + bool length_field_received; + int received_bytes; + int remaining_bytes; + int length_field; + char grpc_header_bytes[GRPC_HEADER_SIZE_IN_BYTES]; + char *payload_field; + + // vars for holding data destined for the application + struct grpc_slice_buffer_stream sbs; + gpr_slice_buffer read_slice_buffer; -enum recv_state { - CRONET_RECV_IDLE = 0, - CRONET_RECV_READ_LENGTH, - CRONET_RECV_READ_DATA, - CRONET_RECV_CLOSED, -}; + // vars for trailing metadata + grpc_chttp2_incoming_metadata_buffer trailing_metadata; + bool trailing_metadata_valid; -static const char *recv_state_name[] = { - "CRONET_RECV_IDLE", "CRONET_RECV_READ_LENGTH", "CRONET_RECV_READ_DATA,", - "CRONET_RECV_CLOSED"}; + // vars for initial metadata + grpc_chttp2_incoming_metadata_buffer initial_metadata; +}; -// Enum that identifies calling function. -enum e_caller { - PERFORM_STREAM_OP, - ON_READ_COMPLETE, - ON_RESPONSE_HEADERS_RECEIVED, - ON_RESPONSE_TRAILERS_RECEIVED +struct write_state { + char *write_buffer; }; -enum callback_id { - OP_SEND_INITIAL_METADATA = 0, - OP_SEND_MESSAGE, - OP_SEND_TRAILING_METADATA, - OP_RECV_MESSAGE, - OP_RECV_INITIAL_METADATA, - OP_RECV_TRAILING_METADATA, - OP_CANCEL_ERROR, - OP_NUM_CALLBACKS +#define MAX_PENDING_OPS 10 +struct op_storage { + grpc_transport_stream_op pending_ops[MAX_PENDING_OPS]; + int wrptr; + int rdptr; + int num_pending_ops; }; struct stream_obj { - // we store received bytes here as they trickle in. - gpr_slice_buffer write_slice_buffer; + grpc_transport_stream_op *curr_op; + grpc_cronet_transport curr_ct; + grpc_stream *curr_gs; cronet_bidirectional_stream *cbs; - gpr_slice slice; - gpr_slice_buffer read_slice_buffer; - struct grpc_slice_buffer_stream sbs; - char *read_buffer; - int remaining_read_bytes; - int total_read_bytes; - - char *write_buffer; - size_t write_buffer_size; - // Hold the URL - char *url; + // TODO (makdharma) : make a sub structure for tracking state + bool state_op_done[OP_NUM_OPS]; + bool state_callback_received[OP_NUM_OPS]; - // One bit per operation - bool op_requested[OP_NUM_CALLBACKS]; - bool op_done[OP_NUM_CALLBACKS]; - // Set to true when server indicates no more data will be sent - bool read_closed; - - // Recv message stuff - grpc_byte_buffer **recv_message; - // Initial metadata stuff - grpc_metadata_batch *recv_initial_metadata; - grpc_chttp2_incoming_metadata_buffer initial_metadata; - // Trailing metadata stuff - grpc_metadata_batch *recv_trailing_metadata; - grpc_chttp2_incoming_metadata_buffer imb; - bool imb_valid; // true if there are any valid entries in imb. - - // This mutex protects receive state machine execution - gpr_mu recv_mu; - - // Callbacks to be called when operations complete - grpc_closure *cb_recv_initial_metadata_ready; - grpc_closure *cb_recv_message_ready; - grpc_closure *on_complete; - - // storage for header - cronet_bidirectional_stream_header *headers; - uint32_t num_headers; - cronet_bidirectional_stream_header_array header_array; - // state tracking - enum recv_state cronet_recv_state; - enum send_state cronet_send_state; + // Read state + struct read_state rs; + // Write state + struct write_state ws; + // OP storage + struct op_storage storage; }; - typedef struct stream_obj stream_obj; -static void next_send_step(stream_obj *s); -static void next_recv_step(stream_obj *s, enum e_caller caller); +/* Globals */ +cronet_bidirectional_stream_header_array header_array; -static void set_pollset_do_nothing(grpc_exec_ctx *exec_ctx, grpc_transport *gt, - grpc_stream *gs, grpc_pollset *pollset) {} - -static void set_pollset_set_do_nothing(grpc_exec_ctx *exec_ctx, - grpc_transport *gt, grpc_stream *gs, - grpc_pollset_set *pollset_set) {} - -// Client creates a bunch of operations and invokes "call_start_batch" -// call_start_batch creates a stream_op structure. this structure has info -// needed for executing all the ops. It has on_complete callback that needs -// to be called when all ops are executed. This function keeps track of all -// outstanding operations. It returns true if all operations that were part of -// the stream_op have been completed. -static bool is_op_complete(stream_obj *s) { - int i; - // Check if any requested op is pending - for (i = 0; i < OP_NUM_CALLBACKS; i++) { - if (s->op_requested[i] && !s->op_done[i]) { - gpr_log(GPR_DEBUG, "is_op_complete is FALSE because of %d", i); - return false; - } - } - // Clear the requested/done bits and return true - for (i = 0; i < OP_NUM_CALLBACKS; i++) { - s->op_requested[i] = s->op_done[i] = false; - } - return true; -} - -static void enqueue_callback(grpc_closure *callback) { - GPR_ASSERT(callback); - grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; - grpc_exec_ctx_sched(&exec_ctx, callback, GRPC_ERROR_NONE, NULL); - grpc_exec_ctx_finish(&exec_ctx); +// +static void execute_curr_stream_op(stream_obj *s); + +/************************************************************* + Op Storage +*/ + +static void add_pending_op(struct op_storage *storage, grpc_transport_stream_op *op) { + GPR_ASSERT(storage->num_pending_ops < MAX_PENDING_OPS); + storage->num_pending_ops++; + gpr_log(GPR_DEBUG, "adding new op @wrptr=%d. %d in the queue.", + storage->wrptr, storage->num_pending_ops); + memcpy(&storage->pending_ops[storage->wrptr], op, sizeof(grpc_transport_stream_op)); + storage->wrptr = (storage->wrptr + 1) % MAX_PENDING_OPS; } -static void on_canceled(cronet_bidirectional_stream *stream) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "on_canceled(%p)", stream); - } - stream_obj *s = (stream_obj *)stream->annotation; - s->op_done[OP_CANCEL_ERROR] = true; - - // Terminate any read callback - if (s->cb_recv_message_ready) { - enqueue_callback(s->cb_recv_message_ready); - s->cb_recv_message_ready = 0; - s->op_done[OP_RECV_MESSAGE] = true; - } - // Don't wait to get any trailing metadata - s->op_done[OP_RECV_TRAILING_METADATA] = true; - - next_send_step(s); +static grpc_transport_stream_op *pop_pending_op(struct op_storage *storage) { + if (storage->num_pending_ops == 0) return NULL; + grpc_transport_stream_op *result = &storage->pending_ops[storage->rdptr]; + storage->rdptr = (storage->rdptr + 1) % MAX_PENDING_OPS; + storage->num_pending_ops--; + gpr_log(GPR_DEBUG, "popping op @rdptr=%d. %d more left in queue", + storage->rdptr, storage->num_pending_ops); + return result; } +/************************************************************* +Cronet Callback Ipmlementation +*/ static void on_failed(cronet_bidirectional_stream *stream, int net_error) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "on_failed(%p, %d)", stream, net_error); - } + gpr_log(GPR_DEBUG, "on_failed(%p, %d)", stream, net_error); } static void on_succeeded(cronet_bidirectional_stream *stream) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "on_succeeded(%p)", stream); - } -} - -static void on_response_trailers_received( - cronet_bidirectional_stream *stream, - const cronet_bidirectional_stream_header_array *trailers) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: on_response_trailers_received(%p,%p)", stream, - trailers); - } - stream_obj *s = (stream_obj *)stream->annotation; - - memset(&s->imb, 0, sizeof(s->imb)); - s->imb_valid = false; - grpc_chttp2_incoming_metadata_buffer_init(&s->imb); - unsigned int i = 0; - for (i = 0; i < trailers->count; i++) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "trailer key=%s, value=%s", trailers->headers[i].key, - trailers->headers[i].value); - } - - grpc_chttp2_incoming_metadata_buffer_add( - &s->imb, grpc_mdelem_from_metadata_strings( - grpc_mdstr_from_string(trailers->headers[i].key), - grpc_mdstr_from_string(trailers->headers[i].value))); - s->imb_valid = true; - } - s->op_done[OP_RECV_TRAILING_METADATA] = true; - next_recv_step(s, ON_RESPONSE_TRAILERS_RECEIVED); + gpr_log(GPR_DEBUG, "on_succeeded(%p)", stream); } -static void on_write_completed(cronet_bidirectional_stream *stream, - const char *data) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "W: on_write_completed(%p, %s)", stream, data); - } - stream_obj *s = (stream_obj *)stream->annotation; - s->op_done[OP_SEND_MESSAGE] = true; - next_send_step(s); -} -static void process_recv_message(stream_obj *s, const uint8_t *recv_data) { - gpr_slice read_data_slice = gpr_slice_malloc((uint32_t)s->total_read_bytes); - uint8_t *dst_p = GPR_SLICE_START_PTR(read_data_slice); - if (s->total_read_bytes > 0) { - // Only copy if there is non-zero number of bytes - memcpy(dst_p, recv_data, (size_t)s->total_read_bytes); - gpr_slice_buffer_add(&s->read_slice_buffer, read_data_slice); - } - grpc_slice_buffer_stream_init(&s->sbs, &s->read_slice_buffer, 0); - *s->recv_message = (grpc_byte_buffer *)&s->sbs; -} - -static int parse_grpc_header(const uint8_t *data) { - const uint8_t *p = data + 1; - int length = 0; - length |= ((uint8_t)*p++) << 24; - length |= ((uint8_t)*p++) << 16; - length |= ((uint8_t)*p++) << 8; - length |= ((uint8_t)*p++); - return length; -} - -static void on_read_completed(cronet_bidirectional_stream *stream, char *data, - int count) { +static void on_request_headers_sent(cronet_bidirectional_stream *stream) { + gpr_log(GPR_DEBUG, "W: on_request_headers_sent(%p)", stream); stream_obj *s = (stream_obj *)stream->annotation; - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: on_read_completed(%p, %p, %d)", stream, data, count); - } - if (count > 0) { - GPR_ASSERT(s->recv_message); - s->remaining_read_bytes -= count; - next_recv_step(s, ON_READ_COMPLETE); - } else { - gpr_log(GPR_DEBUG, "read_closed = true"); - s->read_closed = true; - next_recv_step(s, ON_READ_COMPLETE); - } + s->state_op_done[OP_SEND_INITIAL_METADATA] = true; + s->state_callback_received[OP_SEND_INITIAL_METADATA] = true; + execute_curr_stream_op(s); } static void on_response_headers_received( cronet_bidirectional_stream *stream, const cronet_bidirectional_stream_header_array *headers, const char *negotiated_protocol) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: on_response_headers_received(%p, %p, %s)", stream, + gpr_log(GPR_DEBUG, "R: on_response_headers_received(%p, %p, %s)", stream, headers, negotiated_protocol); - } - stream_obj *s = (stream_obj *)stream->annotation; - memset(&s->initial_metadata, 0, sizeof(s->initial_metadata)); - grpc_chttp2_incoming_metadata_buffer_init(&s->initial_metadata); + stream_obj *s = (stream_obj *)stream->annotation; + memset(&s->rs.initial_metadata, 0, sizeof(s->rs.initial_metadata)); + grpc_chttp2_incoming_metadata_buffer_init(&s->rs.initial_metadata); unsigned int i = 0; for (i = 0; i < headers->count; i++) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "header key=%s, value=%s", headers->headers[i].key, - headers->headers[i].value); - } grpc_chttp2_incoming_metadata_buffer_add( - &s->initial_metadata, + &s->rs.initial_metadata, grpc_mdelem_from_metadata_strings( grpc_mdstr_from_string(headers->headers[i].key), grpc_mdstr_from_string(headers->headers[i].value))); } - - grpc_chttp2_incoming_metadata_buffer_publish(&s->initial_metadata, - s->recv_initial_metadata); - enqueue_callback(s->cb_recv_initial_metadata_ready); - s->op_done[OP_RECV_INITIAL_METADATA] = true; - next_recv_step(s, ON_RESPONSE_HEADERS_RECEIVED); + s->state_callback_received[OP_RECV_INITIAL_METADATA] = true; + execute_curr_stream_op(s); } -static void on_request_headers_sent(cronet_bidirectional_stream *stream) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "W: on_request_headers_sent(%p)", stream); - } +static void on_write_completed(cronet_bidirectional_stream *stream, + const char *data) { + gpr_log(GPR_DEBUG, "W: on_write_completed(%p, %s)", stream, data); stream_obj *s = (stream_obj *)stream->annotation; - s->op_done[OP_SEND_INITIAL_METADATA] = true; - next_send_step(s); -} - -// Callback function pointers (invoked by cronet in response to events) -static cronet_bidirectional_stream_callback callbacks = { - on_request_headers_sent, - on_response_headers_received, - on_read_completed, - on_write_completed, - on_response_trailers_received, - on_succeeded, - on_failed, - on_canceled}; - -static void invoke_closing_callback(stream_obj *s) { - if (!is_op_complete(s)) return; - - if (s->imb_valid) { - grpc_chttp2_incoming_metadata_buffer_publish(&s->imb, - s->recv_trailing_metadata); + if (s->ws.write_buffer) { + gpr_free(s->ws.write_buffer); + s->ws.write_buffer = NULL; } - enqueue_callback(s->on_complete); + s->state_callback_received[OP_SEND_MESSAGE] = true; + execute_curr_stream_op(s); } -static void set_recv_state(stream_obj *s, enum recv_state state) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "next_state = %s", recv_state_name[state]); +static void on_read_completed(cronet_bidirectional_stream *stream, char *data, + int count) { + stream_obj *s = (stream_obj *)stream->annotation; + gpr_log(GPR_DEBUG, "R: on_read_completed(%p, %p, %d)", stream, data, count); + if (count > 0) { + s->rs.received_bytes += count; + s->rs.remaining_bytes -= count; + if (s->rs.remaining_bytes > 0) { + gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_read"); + cronet_bidirectional_stream_read(s->cbs, s->rs.read_buffer + s->rs.received_bytes, s->rs.remaining_bytes); + } else { + execute_curr_stream_op(s); + } + s->state_callback_received[OP_RECV_MESSAGE] = true; } - s->cronet_recv_state = state; } -// This is invoked from perform_stream_op, and all on_xxxx callbacks. -static void next_recv_step(stream_obj *s, enum e_caller caller) { - // gpr_log(GPR_DEBUG, "locking mutex %p", &s->recv_mu); - gpr_mu_lock(&s->recv_mu); - switch (s->cronet_recv_state) { - case CRONET_RECV_IDLE: - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "cronet_recv_state = CRONET_RECV_IDLE, caller=%d", - caller); - } - if (caller == PERFORM_STREAM_OP || - caller == ON_RESPONSE_HEADERS_RECEIVED) { - if (s->read_closed && s->op_done[OP_RECV_TRAILING_METADATA]) { - set_recv_state(s, CRONET_RECV_CLOSED); - } else if (s->op_done[OP_RECV_INITIAL_METADATA] == true && - s->op_requested[OP_RECV_MESSAGE]) { - set_recv_state(s, CRONET_RECV_READ_LENGTH); - s->total_read_bytes = s->remaining_read_bytes = - GRPC_HEADER_SIZE_IN_BYTES; - GPR_ASSERT(s->read_buffer); - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: cronet_bidirectional_stream_read(%p,%p,%d)", - s->cbs, s->read_buffer, s->remaining_read_bytes); - } - cronet_bidirectional_stream_read(s->cbs, s->read_buffer, - s->remaining_read_bytes); - } - } else if (caller == ON_RESPONSE_TRAILERS_RECEIVED) { - // We get here when we receive trailers directly, i.e. without - // going through a data read operation. - set_recv_state(s, CRONET_RECV_CLOSED); - } - break; - case CRONET_RECV_READ_LENGTH: - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "cronet_recv_state = CRONET_RECV_READ_LENGTH"); - } - if (caller == ON_READ_COMPLETE) { - if (s->read_closed) { - enqueue_callback(s->cb_recv_message_ready); - s->op_done[OP_RECV_MESSAGE] = true; - set_recv_state(s, CRONET_RECV_CLOSED); - } else { - GPR_ASSERT(s->remaining_read_bytes == 0); - set_recv_state(s, CRONET_RECV_READ_DATA); - s->total_read_bytes = s->remaining_read_bytes = - parse_grpc_header((const uint8_t *)s->read_buffer); - s->read_buffer = - gpr_realloc(s->read_buffer, (uint32_t)s->remaining_read_bytes); - GPR_ASSERT(s->read_buffer); - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: cronet_bidirectional_stream_read(%p,%p,%d)", - s->cbs, s->read_buffer, s->remaining_read_bytes); - } - if (s->remaining_read_bytes > 0) { - cronet_bidirectional_stream_read(s->cbs, (char *)s->read_buffer, - s->remaining_read_bytes); - } else { - // Calling the closing callback directly since this is a 0 byte read - // for an empty message. - process_recv_message(s, NULL); - enqueue_callback(s->cb_recv_message_ready); - s->op_done[OP_RECV_MESSAGE] = true; - set_recv_state(s, CRONET_RECV_CLOSED); - } - } - } - break; - case CRONET_RECV_READ_DATA: - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "cronet_recv_state = CRONET_RECV_READ_DATA"); - } - if (caller == ON_READ_COMPLETE) { - if (s->remaining_read_bytes > 0) { - int offset = s->total_read_bytes - s->remaining_read_bytes; - GPR_ASSERT(s->read_buffer); - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "R: cronet_bidirectional_stream_read()"); - } - cronet_bidirectional_stream_read( - s->cbs, (char *)s->read_buffer + offset, s->remaining_read_bytes); - } else { - gpr_slice_buffer_init(&s->read_slice_buffer); - uint8_t *p = (uint8_t *)s->read_buffer; - process_recv_message(s, p); - set_recv_state(s, CRONET_RECV_IDLE); - enqueue_callback(s->cb_recv_message_ready); - s->op_done[OP_RECV_MESSAGE] = true; - } - } - break; - case CRONET_RECV_CLOSED: - break; - default: - GPR_ASSERT(0); // Should not reach here - break; +static void on_response_trailers_received( + cronet_bidirectional_stream *stream, + const cronet_bidirectional_stream_header_array *trailers) { + gpr_log(GPR_DEBUG, "R: on_response_trailers_received(%p,%p)", stream, + trailers); + stream_obj *s = (stream_obj *)stream->annotation; + memset(&s->rs.trailing_metadata, 0, sizeof(s->rs.trailing_metadata)); + s->rs.trailing_metadata_valid = false; + grpc_chttp2_incoming_metadata_buffer_init(&s->rs.trailing_metadata); + unsigned int i = 0; + for (i = 0; i < trailers->count; i++) { + gpr_log(GPR_DEBUG, "trailer key=%s, value=%s", trailers->headers[i].key, + trailers->headers[i].value); + grpc_chttp2_incoming_metadata_buffer_add( + &s->rs.trailing_metadata, grpc_mdelem_from_metadata_strings( + grpc_mdstr_from_string(trailers->headers[i].key), + grpc_mdstr_from_string(trailers->headers[i].value))); + s->rs.trailing_metadata_valid = true; } - invoke_closing_callback(s); - gpr_mu_unlock(&s->recv_mu); - // gpr_log(GPR_DEBUG, "unlocking mutex %p", &s->recv_mu); + s->state_callback_received[OP_RECV_TRAILING_METADATA] = true; + execute_curr_stream_op(s); } +/************************************************************* +Utility functions. Can be in their own file +*/ // This function takes the data from s->write_slice_buffer and assembles into // a contiguous byte stream with 5 byte gRPC header prepended. -static void create_grpc_frame(stream_obj *s) { - gpr_slice slice = gpr_slice_buffer_take_first(&s->write_slice_buffer); - uint8_t *raw_data = GPR_SLICE_START_PTR(slice); +static void create_grpc_frame(gpr_slice_buffer *write_slice_buffer, + char **pp_write_buffer, int *p_write_buffer_size) { + gpr_slice slice = gpr_slice_buffer_take_first(write_slice_buffer); size_t length = GPR_SLICE_LENGTH(slice); - s->write_buffer_size = length + GRPC_HEADER_SIZE_IN_BYTES; - s->write_buffer = gpr_realloc(s->write_buffer, s->write_buffer_size); - uint8_t *p = (uint8_t *)s->write_buffer; + // TODO (makdharma): FREE THIS!! HACK! + *p_write_buffer_size = (int)length + GRPC_HEADER_SIZE_IN_BYTES; + char *write_buffer = gpr_malloc(length + GRPC_HEADER_SIZE_IN_BYTES); + *pp_write_buffer = write_buffer; + uint8_t *p = (uint8_t *)write_buffer; // Append 5 byte header *p++ = 0; *p++ = (uint8_t)(length >> 24); @@ -489,135 +292,22 @@ static void create_grpc_frame(stream_obj *s) { *p++ = (uint8_t)(length >> 8); *p++ = (uint8_t)(length); // append actual data - memcpy(p, raw_data, length); -} -// Return false if there is no data to write -static bool do_write(stream_obj *s) { - gpr_slice_buffer *sb = &s->write_slice_buffer; - GPR_ASSERT(sb->count <= 1); - if (sb->count > 0) { - create_grpc_frame(s); - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_write(%p,%p,%d,%d)", - s->cbs, s->write_buffer, (int)s->write_buffer_size, false); - } - cronet_bidirectional_stream_write(s->cbs, s->write_buffer, - (int)s->write_buffer_size, false); - return true; - } else { - return false; - } + memcpy(p, GPR_SLICE_START_PTR(slice), length); } -static void init_cronet_stream(stream_obj *s, grpc_transport *gt) { - GPR_ASSERT(s->cbs == NULL); - grpc_cronet_transport *ct = (grpc_cronet_transport *)gt; - GPR_ASSERT(ct->engine); - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_create"); - } - s->cbs = cronet_bidirectional_stream_create(ct->engine, s, &callbacks); - GPR_ASSERT(s->cbs); - s->read_closed = false; - - for (int i = 0; i < OP_NUM_CALLBACKS; i++) { - s->op_requested[i] = s->op_done[i] = false; - } - s->cronet_send_state = CRONET_SEND_IDLE; - s->cronet_recv_state = CRONET_RECV_IDLE; -} - -static bool do_close_connection(stream_obj *s) { - s->op_done[OP_SEND_TRAILING_METADATA] = true; - if (s->cbs) { - // Send an "empty" write to the far end to signal that we're done. - // This will induce the server to send down trailers. - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_write length 0"); - } - cronet_bidirectional_stream_write(s->cbs, "abc", 0, true); - return true; - } else { - // We never created a stream. This was probably an empty request. - invoke_closing_callback(s); - return true; - } - return false; -} - -// -static void next_send_step(stream_obj *s) { - gpr_log(GPR_DEBUG, "next_send_step cronet_send_state=%d", - s->cronet_send_state); - switch (s->cronet_send_state) { - case CRONET_SEND_IDLE: - GPR_ASSERT( - s->cbs); // cronet_bidirectional_stream is not initialized yet. - s->cronet_send_state = CRONET_SEND_HEADER; - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_start to %s", s->url); - } - cronet_bidirectional_stream_start(s->cbs, s->url, 0, "POST", - &s->header_array, false); - // we no longer need the memory that was allocated earlier. - gpr_free(s->header_array.headers); - break; - case CRONET_SEND_HEADER: - if (s->op_requested[OP_CANCEL_ERROR]) { - cronet_bidirectional_stream_cancel(s->cbs); - gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); - s->cronet_send_state = CRONET_WAIT_FOR_CANCEL; - } else if (do_write(s) == false && - s->op_requested[OP_SEND_TRAILING_METADATA]) { - if (do_close_connection(s)) { - s->cronet_send_state = CRONET_STREAM_CLOSED; - } - } else { - s->cronet_send_state = CRONET_WRITE_PENDING; - } - break; - case CRONET_WRITE_PENDING: - if (s->op_requested[OP_CANCEL_ERROR]) { - cronet_bidirectional_stream_cancel(s->cbs); - gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); - s->cronet_send_state = CRONET_WAIT_FOR_CANCEL; - } else if (do_write(s) == false && - s->op_requested[OP_SEND_TRAILING_METADATA]) { - if (do_close_connection(s)) { - s->cronet_send_state = CRONET_STREAM_CLOSED; - } - } else { - s->cronet_send_state = CRONET_WRITE_COMPLETED; - } - break; - case CRONET_WRITE_COMPLETED: - if (s->op_requested[OP_CANCEL_ERROR]) { - cronet_bidirectional_stream_cancel(s->cbs); - gpr_log(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); - s->cronet_send_state = CRONET_WAIT_FOR_CANCEL; - } else if (do_write(s) == false && - s->op_requested[OP_SEND_TRAILING_METADATA]) { - if (do_close_connection(s)) { - s->cronet_send_state = CRONET_STREAM_CLOSED; - } - } - break; - case CRONET_STREAM_CLOSED: - s->cronet_send_state = CRONET_SEND_IDLE; - break; - case CRONET_WAIT_FOR_CANCEL: - invoke_closing_callback(s); - s->cronet_send_state = CRONET_SEND_IDLE; - break; - default: - GPR_ASSERT(0); - break; - } +static void enqueue_callback(grpc_closure *callback) { + GPR_ASSERT(callback); + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + grpc_exec_ctx_sched(&exec_ctx, callback, GRPC_ERROR_NONE, NULL); + grpc_exec_ctx_finish(&exec_ctx); } -static void convert_metadata_to_cronet_headers(grpc_linked_mdelem *head, - const char *host, - stream_obj *s) { +static void convert_metadata_to_cronet_headers( + grpc_linked_mdelem *head, + const char *host, + char **pp_url, + cronet_bidirectional_stream_header **pp_headers, + size_t *p_num_headers) { grpc_linked_mdelem *curr = head; // Walk the linked list and get number of header fields uint32_t num_headers_available = 0; @@ -625,17 +315,19 @@ static void convert_metadata_to_cronet_headers(grpc_linked_mdelem *head, curr = curr->next; num_headers_available++; } - // Allocate enough memory - s->headers = (cronet_bidirectional_stream_header *)gpr_malloc( + // Allocate enough memory. TODO (makdharma): FREE MEMORY! HACK HACK + cronet_bidirectional_stream_header *headers = + (cronet_bidirectional_stream_header *)gpr_malloc( sizeof(cronet_bidirectional_stream_header) * num_headers_available); + *pp_headers = headers; // Walk the linked list again, this time copying the header fields. // s->num_headers // can be less than num_headers_available, as some headers are not used for // cronet curr = head; - s->num_headers = 0; - while (s->num_headers < num_headers_available) { + int num_headers = 0; + while (num_headers < num_headers_available) { grpc_mdelem *mdelem = curr->md; curr = curr->next; const char *key = grpc_mdstr_as_c_string(mdelem->key); @@ -647,161 +339,237 @@ static void convert_metadata_to_cronet_headers(grpc_linked_mdelem *head, } if (strcmp(key, ":path") == 0) { // Create URL by appending :path value to the hostname - gpr_asprintf(&s->url, "https://%s%s", host, value); - if (grpc_cronet_trace) { - // gpr_log(GPR_DEBUG, "extracted URL = %s", s->url); - } + gpr_asprintf(pp_url, "https://%s%s", host, value); continue; } - s->headers[s->num_headers].key = key; - s->headers[s->num_headers].value = value; - s->num_headers++; + headers[num_headers].key = key; + headers[num_headers].value = value; + num_headers++; if (curr == NULL) { break; } } + *p_num_headers = num_headers; } -static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, - grpc_stream *gs, grpc_transport_stream_op *op) { - grpc_cronet_transport *ct = (grpc_cronet_transport *)gt; - GPR_ASSERT(ct->engine); - stream_obj *s = (stream_obj *)gs; - // Initialize a cronet bidirectional stream if it doesn't exist. - if (s->cbs == NULL) { - init_cronet_stream(s, gt); - } - - s->on_complete = op->on_complete; +static int parse_grpc_header(const uint8_t *data) { + const uint8_t *p = data + 1; + int length = 0; + length |= ((uint8_t)*p++) << 24; + length |= ((uint8_t)*p++) << 16; + length |= ((uint8_t)*p++) << 8; + length |= ((uint8_t)*p++); + return length; +} - if (op->recv_trailing_metadata) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, - "perform_stream_op - recv_trailing_metadata: on_complete=%p", - op->on_complete); - } - s->recv_trailing_metadata = op->recv_trailing_metadata; - s->op_requested[OP_RECV_TRAILING_METADATA] = true; - } - if (op->recv_message) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "perform_stream_op - recv_message: on_complete=%p", - op->on_complete); - } - s->recv_message = (grpc_byte_buffer **)op->recv_message; - s->cb_recv_message_ready = op->recv_message_ready; - s->op_requested[OP_RECV_MESSAGE] = true; - } - if (op->recv_initial_metadata) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, - "perform_stream_op - recv_initial_metadata on_complete=%p, " - "on_ready=%p", - op->on_complete, op->recv_initial_metadata_ready); - } - s->recv_initial_metadata = op->recv_initial_metadata; - s->cb_recv_initial_metadata_ready = op->recv_initial_metadata_ready; - s->op_requested[OP_RECV_INITIAL_METADATA] = true; - } - if (op->send_initial_metadata) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, - "perform_stream_op - send_initial_metadata: on_complete=%p", - op->on_complete); - } - s->num_headers = 0; - convert_metadata_to_cronet_headers(op->send_initial_metadata->list.head, - ct->host, s); - s->header_array.count = s->num_headers; - s->header_array.capacity = s->num_headers; - s->header_array.headers = s->headers; - s->op_requested[OP_SEND_INITIAL_METADATA] = true; +/* +Op Execution +*/ + +static bool op_can_be_run(stream_obj *s, enum OP_ID op_id) { + if (op_id == OP_SEND_INITIAL_METADATA) { + // already executed + if (s->state_op_done[OP_SEND_INITIAL_METADATA]) return false; + } + if (op_id == OP_RECV_INITIAL_METADATA) { + // already executed + if (s->state_op_done[OP_RECV_INITIAL_METADATA]) return false; + // we haven't sent headers yet. + if (!s->state_callback_received[OP_SEND_INITIAL_METADATA]) return false; + // we haven't received headers yet. + if (!s->state_callback_received[OP_RECV_INITIAL_METADATA]) return false; + } + if (op_id == OP_SEND_MESSAGE) { + // already executed + if (s->state_op_done[OP_SEND_MESSAGE]) return false; + // we haven't received headers yet. + if (!s->state_callback_received[OP_RECV_INITIAL_METADATA]) return false; + } + if (op_id == OP_RECV_MESSAGE) { + // already executed + if (s->state_op_done[OP_RECV_MESSAGE]) return false; + // we haven't received headers yet. + if (!s->state_callback_received[OP_RECV_INITIAL_METADATA]) return false; + } + if (op_id == OP_RECV_TRAILING_METADATA) { + // already executed + if (s->state_op_done[OP_RECV_TRAILING_METADATA]) return false; + // we haven't received trailers yet. + if (!s->state_callback_received[OP_RECV_TRAILING_METADATA]) return false; + } + if (op_id == OP_SEND_TRAILING_METADATA) { + // already executed + if (s->state_op_done[OP_SEND_TRAILING_METADATA]) return false; + // we haven't sent message yet + if (s->curr_op->send_message && !s->state_op_done[OP_SEND_MESSAGE]) return false; + } + + if (op_id == OP_ON_COMPLETE) { + // already executed + if (s->state_op_done[OP_ON_COMPLETE]) return false; + // Check if every op that was asked for is done. + if (s->curr_op->send_initial_metadata && !s->state_op_done[OP_SEND_INITIAL_METADATA]) return false; + if (s->curr_op->send_message && !s->state_op_done[OP_SEND_MESSAGE]) return false; + if (s->curr_op->send_trailing_metadata && !s->state_op_done[OP_SEND_TRAILING_METADATA]) return false; + if (s->curr_op->recv_initial_metadata && !s->state_op_done[OP_RECV_INITIAL_METADATA]) return false; + if (s->curr_op->recv_message && !s->state_op_done[OP_RECV_MESSAGE]) return false; + if (s->curr_op->recv_trailing_metadata && !s->state_op_done[OP_RECV_TRAILING_METADATA]) return false; } - if (op->send_message) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "perform_stream_op - send_message: on_complete=%p", - op->on_complete); - } - grpc_byte_stream_next(exec_ctx, op->send_message, &s->slice, - op->send_message->length, NULL); + return true; +} + +static void execute_curr_stream_op(stream_obj *s) { + if (s->curr_op->send_initial_metadata && op_can_be_run(s, OP_SEND_INITIAL_METADATA)) { + // This OP is the beginning. Reset various states + memset(&s->rs, 0, sizeof(s->rs)); + memset(&s->ws, 0, sizeof(s->ws)); + memset(s->state_op_done, 0, sizeof(s->state_op_done)); + memset(s->state_callback_received, 0, sizeof(s->state_callback_received)); + // Start new cronet stream + GPR_ASSERT(s->cbs == NULL); + gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_create"); + s->cbs = cronet_bidirectional_stream_create(s->curr_ct.engine, s->curr_gs, &cronet_callbacks); + char *url; + convert_metadata_to_cronet_headers(s->curr_op->send_initial_metadata->list.head, + s->curr_ct.host, &url, &header_array.headers, &header_array.count); + header_array.capacity = header_array.count; + gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_start"); + cronet_bidirectional_stream_start(s->cbs, url, 0, "POST", &header_array, false); + s->state_op_done[OP_SEND_INITIAL_METADATA] = true; + } else if (s->curr_op->recv_initial_metadata && + op_can_be_run(s, OP_RECV_INITIAL_METADATA)) { + grpc_chttp2_incoming_metadata_buffer_publish(&s->rs.initial_metadata, + s->curr_op->recv_initial_metadata); + enqueue_callback(s->curr_op->recv_initial_metadata_ready); + s->state_op_done[OP_RECV_INITIAL_METADATA] = true; + // We are ready to execute send_message. + execute_curr_stream_op(s); + } else if (s->curr_op->send_message && op_can_be_run(s, OP_SEND_MESSAGE)) { + // TODO (makdharma): Make into a standalone function + gpr_slice_buffer write_slice_buffer; + gpr_slice slice; + gpr_slice_buffer_init(&write_slice_buffer); + grpc_byte_stream_next(NULL, s->curr_op->send_message, &slice, + s->curr_op->send_message->length, NULL); // Check that compression flag is not ON. We don't support compression yet. // TODO (makdharma): add compression support - GPR_ASSERT(op->send_message->flags == 0); - gpr_slice_buffer_add(&s->write_slice_buffer, s->slice); - s->op_requested[OP_SEND_MESSAGE] = true; - } - if (op->send_trailing_metadata) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, - "perform_stream_op - send_trailing_metadata: on_complete=%p", - op->on_complete); + GPR_ASSERT(s->curr_op->send_message->flags == 0); + gpr_slice_buffer_add(&write_slice_buffer, slice); + GPR_ASSERT(write_slice_buffer.count == 1); // Empty request not handled yet + if (write_slice_buffer.count > 0) { + int write_buffer_size; + create_grpc_frame(&write_slice_buffer, &s->ws.write_buffer, &write_buffer_size); + gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_write (%p)", s->ws.write_buffer); + cronet_bidirectional_stream_write(s->cbs, s->ws.write_buffer, + write_buffer_size, false); // TODO: What if this is not the last write? } - s->op_requested[OP_SEND_TRAILING_METADATA] = true; - } - if (op->cancel_error) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "perform_stream_op - cancel_error: on_complete=%p", - op->on_complete); + s->state_op_done[OP_SEND_MESSAGE] = true; + } else if (s->curr_op->recv_message && op_can_be_run(s, OP_RECV_MESSAGE)) { + if (s->rs.length_field_received == false) { + if (s->rs.received_bytes == GRPC_HEADER_SIZE_IN_BYTES && s->rs.remaining_bytes == 0) { + // Start a read operation for data + s->rs.length_field_received = true; + s->rs.length_field = s->rs.remaining_bytes = + parse_grpc_header((const uint8_t *)s->rs.read_buffer); + GPR_ASSERT(s->rs.length_field > 0); // Empty message? + gpr_log(GPR_DEBUG, "length field = %d", s->rs.length_field); + s->rs.read_buffer = gpr_malloc(s->rs.length_field); + GPR_ASSERT(s->rs.read_buffer); + s->rs.remaining_bytes = s->rs.length_field; + s->rs.received_bytes = 0; + gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_read"); + cronet_bidirectional_stream_read(s->cbs, s->rs.read_buffer, + s->rs.remaining_bytes); + } else if (s->rs.remaining_bytes == 0) { + // Start a read operation for first 5 bytes (GRPC header) + s->rs.read_buffer = s->rs.grpc_header_bytes; + s->rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; + s->rs.received_bytes = 0; + gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_read"); + cronet_bidirectional_stream_read(s->cbs, s->rs.read_buffer, + s->rs.remaining_bytes); + } + } else if (s->rs.remaining_bytes == 0) { + gpr_log(GPR_DEBUG, "read operation complete"); + gpr_slice read_data_slice = gpr_slice_malloc((uint32_t)s->rs.length_field); + uint8_t *dst_p = GPR_SLICE_START_PTR(read_data_slice); + memcpy(dst_p, s->rs.read_buffer, (size_t)s->rs.length_field); + gpr_slice_buffer_init(&s->rs.read_slice_buffer); + gpr_slice_buffer_add(&s->rs.read_slice_buffer, read_data_slice); + grpc_slice_buffer_stream_init(&s->rs.sbs, &s->rs.read_slice_buffer, 0); + *((grpc_byte_buffer **)s->curr_op->recv_message) = (grpc_byte_buffer *)&s->rs.sbs; + enqueue_callback(s->curr_op->recv_message_ready); + s->state_op_done[OP_RECV_MESSAGE] = true; + execute_curr_stream_op(s); + } + } else if (s->curr_op->recv_trailing_metadata && + op_can_be_run(s, OP_RECV_TRAILING_METADATA)) { + if (s->rs.trailing_metadata_valid) { + grpc_chttp2_incoming_metadata_buffer_publish( + &s->rs.trailing_metadata, s->curr_op->recv_trailing_metadata); + s->rs.trailing_metadata_valid = false; } - s->op_requested[OP_CANCEL_ERROR] = true; + s->state_op_done[OP_RECV_TRAILING_METADATA] = true; + execute_curr_stream_op(s); + } else if (s->curr_op->send_trailing_metadata && + op_can_be_run(s, OP_SEND_TRAILING_METADATA)) { + + gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_write (0)"); + cronet_bidirectional_stream_write(s->cbs, "", 0, true); + s->state_op_done[OP_SEND_TRAILING_METADATA] = true; + } else if (op_can_be_run(s, OP_ON_COMPLETE)) { + // All ops are complete. Call the on_complete callback + enqueue_callback(s->curr_op->on_complete); + s->state_op_done[OP_ON_COMPLETE] = true; + cronet_bidirectional_stream_destroy(s->cbs); + s->cbs = NULL; } - next_send_step(s); - next_recv_step(s, PERFORM_STREAM_OP); } +//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + static int init_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, grpc_stream *gs, grpc_stream_refcount *refcount, const void *server_data) { stream_obj *s = (stream_obj *)gs; - memset(s, 0, sizeof(stream_obj)); - s->cbs = NULL; - gpr_mu_init(&s->recv_mu); - s->read_buffer = gpr_malloc(GRPC_HEADER_SIZE_IN_BYTES); - s->write_buffer = gpr_malloc(GRPC_HEADER_SIZE_IN_BYTES); - gpr_slice_buffer_init(&s->write_slice_buffer); - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "cronet_transport - init_stream"); - } + memset(&s->storage, 0, sizeof(s->storage)); + s->curr_op = NULL; return 0; } -static void destroy_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, - grpc_stream *gs, void *and_free_memory) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "Destroy stream"); - } +static void set_pollset_do_nothing(grpc_exec_ctx *exec_ctx, grpc_transport *gt, + grpc_stream *gs, grpc_pollset *pollset) {} + +static void set_pollset_set_do_nothing(grpc_exec_ctx *exec_ctx, + grpc_transport *gt, grpc_stream *gs, + grpc_pollset_set *pollset_set) {} + +static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, + grpc_stream *gs, grpc_transport_stream_op *op) { + gpr_log(GPR_DEBUG, "perform_stream_op"); stream_obj *s = (stream_obj *)gs; - s->cbs = NULL; - gpr_free(s->read_buffer); - gpr_free(s->write_buffer); - gpr_free(s->url); - gpr_log(GPR_DEBUG, "destroying %p", &s->recv_mu); - gpr_mu_destroy(&s->recv_mu); - if (and_free_memory) { - gpr_free(and_free_memory); + memcpy(&s->curr_ct, gt, sizeof(grpc_cronet_transport)); + add_pending_op(&s->storage, op); + if (s->curr_op == NULL) { + s->curr_op = pop_pending_op(&s->storage); } + s->curr_gs = gs; + execute_curr_stream_op(s); +} + +static void destroy_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, + grpc_stream *gs, void *and_free_memory) { } static void destroy_transport(grpc_exec_ctx *exec_ctx, grpc_transport *gt) { - grpc_cronet_transport *ct = (grpc_cronet_transport *)gt; - gpr_free(ct->host); - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "Destroy transport"); - } } static char *get_peer(grpc_exec_ctx *exec_ctx, grpc_transport *gt) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "Unimplemented method"); - } return NULL; } static void perform_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, grpc_transport_op *op) { - if (grpc_cronet_trace) { - gpr_log(GPR_DEBUG, "Unimplemented method"); - } } const grpc_transport_vtable grpc_cronet_vtable = {sizeof(stream_obj), From f07506438c012f1f466670f05284594ca6808a26 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Tue, 9 Aug 2016 14:25:08 -0700 Subject: [PATCH 05/14] Work in progress. Do not check in yet. --- .../cronet/transport/cronet_transport.c | 541 ++++++++++++------ 1 file changed, 359 insertions(+), 182 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 694d346fc33..e8746b4e6e6 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -51,6 +51,20 @@ #define GRPC_HEADER_SIZE_IN_BYTES 5 +#define CRONET_LOG(...) {if (1) gpr_log(__VA_ARGS__);} + +enum OP_RESULT { + ACTION_TAKEN_WITH_CALLBACK, + ACTION_TAKEN_NO_CALLBACK, + NO_ACTION_POSSIBLE +}; + +const char *OP_RESULT_STRING[] = { + "ACTION_TAKEN_WITH_CALLBACK", + "ACTION_TAKEN_NO_CALLBACK", + "NO_ACTION_POSSIBLE" +}; + enum OP_ID { OP_SEND_INITIAL_METADATA = 0, OP_SEND_MESSAGE, @@ -60,9 +74,29 @@ enum OP_ID { OP_RECV_TRAILING_METADATA, OP_CANCEL_ERROR, OP_ON_COMPLETE, + OP_FAILED, + OP_CANCELED, + OP_RECV_MESSAGE_AND_ON_COMPLETE, + OP_READ_REQ_MADE, OP_NUM_OPS }; +const char *op_id_string[] = { + "OP_SEND_INITIAL_METADATA", + "OP_SEND_MESSAGE", + "OP_SEND_TRAILING_METADATA", + "OP_RECV_MESSAGE", + "OP_RECV_INITIAL_METADATA", + "OP_RECV_TRAILING_METADATA", + "OP_CANCEL_ERROR", + "OP_ON_COMPLETE", + "OP_FAILED", + "OP_CANCELED", + "OP_RECV_MESSAGE_AND_ON_COMPLETE", + "OP_READ_REQ_MADE", + "OP_NUM_OPS" +}; + /* Cronet callbacks */ static void on_request_headers_sent(cronet_bidirectional_stream *); @@ -75,7 +109,7 @@ static void on_response_trailers_received(cronet_bidirectional_stream *, const cronet_bidirectional_stream_header_array *); static void on_succeeded(cronet_bidirectional_stream *); static void on_failed(cronet_bidirectional_stream *, int); -//static void on_canceled(cronet_bidirectional_stream *); +static void on_canceled(cronet_bidirectional_stream *); static cronet_bidirectional_stream_callback cronet_callbacks = { on_request_headers_sent, on_response_headers_received, @@ -84,7 +118,7 @@ static cronet_bidirectional_stream_callback cronet_callbacks = { on_response_trailers_received, on_succeeded, on_failed, - NULL //on_canceled + on_canceled }; // Cronet transport object @@ -121,30 +155,49 @@ struct write_state { char *write_buffer; }; -#define MAX_PENDING_OPS 10 +// maximum ops in a batch.. There is not much thinking behind this limit, except +// that it seems to be enough for most use cases. +#define MAX_PENDING_OPS 100 + +struct op_state { + bool state_op_done[OP_NUM_OPS]; + bool state_callback_received[OP_NUM_OPS]; + // data structure for storing data coming from server + struct read_state rs; + // data structure for storing data going to the server + struct write_state ws; +}; + +struct op_and_state { + grpc_transport_stream_op op; + struct op_state state; + bool done; + struct stream_obj *s; // Pointer back to the stream object +}; + struct op_storage { - grpc_transport_stream_op pending_ops[MAX_PENDING_OPS]; + struct op_and_state pending_ops[MAX_PENDING_OPS]; int wrptr; int rdptr; int num_pending_ops; }; struct stream_obj { + struct op_and_state *oas; grpc_transport_stream_op *curr_op; grpc_cronet_transport curr_ct; grpc_stream *curr_gs; cronet_bidirectional_stream *cbs; - // TODO (makdharma) : make a sub structure for tracking state - bool state_op_done[OP_NUM_OPS]; - bool state_callback_received[OP_NUM_OPS]; + // This holds the state that is at stream level (response and req metadata) + struct op_state state; - // Read state - struct read_state rs; - // Write state - struct write_state ws; + //struct op_state state; // OP storage struct op_storage storage; + + // Mutex to protect execute_curr_streaming_op + gpr_mu mu; }; typedef struct stream_obj stream_obj; @@ -152,123 +205,155 @@ typedef struct stream_obj stream_obj; cronet_bidirectional_stream_header_array header_array; // -static void execute_curr_stream_op(stream_obj *s); +static enum OP_RESULT execute_stream_op(struct op_and_state *oas); /************************************************************* Op Storage */ -static void add_pending_op(struct op_storage *storage, grpc_transport_stream_op *op) { +static void add_to_storage(struct stream_obj *s, grpc_transport_stream_op *op) { + struct op_storage *storage = &s->storage; GPR_ASSERT(storage->num_pending_ops < MAX_PENDING_OPS); storage->num_pending_ops++; - gpr_log(GPR_DEBUG, "adding new op @wrptr=%d. %d in the queue.", + CRONET_LOG(GPR_DEBUG, "adding new op @wrptr=%d. %d in the queue.", storage->wrptr, storage->num_pending_ops); - memcpy(&storage->pending_ops[storage->wrptr], op, sizeof(grpc_transport_stream_op)); + memcpy(&storage->pending_ops[storage->wrptr].op, op, sizeof(grpc_transport_stream_op)); + memset(&storage->pending_ops[storage->wrptr].state, 0, sizeof(storage->pending_ops[storage->wrptr].state)); + storage->pending_ops[storage->wrptr].done = false; + storage->pending_ops[storage->wrptr].s = s; storage->wrptr = (storage->wrptr + 1) % MAX_PENDING_OPS; } -static grpc_transport_stream_op *pop_pending_op(struct op_storage *storage) { - if (storage->num_pending_ops == 0) return NULL; - grpc_transport_stream_op *result = &storage->pending_ops[storage->rdptr]; - storage->rdptr = (storage->rdptr + 1) % MAX_PENDING_OPS; - storage->num_pending_ops--; - gpr_log(GPR_DEBUG, "popping op @rdptr=%d. %d more left in queue", - storage->rdptr, storage->num_pending_ops); - return result; +static void execute_from_storage(stream_obj *s) { + // Cycle through ops and try to take next action. Break when either + // an action with callback is taken, or no action is possible. + gpr_mu_lock(&s->mu); + for (int i = 0; i < s->storage.wrptr; ) { + CRONET_LOG(GPR_DEBUG, "calling execute_stream_op[%d]. done = %d", i, s->storage.pending_ops[i].done); + if (s->storage.pending_ops[i].done) { + i++; + continue; + } + enum OP_RESULT result = execute_stream_op(&s->storage.pending_ops[i]); + CRONET_LOG(GPR_DEBUG, "%s = execute_stream_op[%d]", OP_RESULT_STRING[result], i); + if (result == NO_ACTION_POSSIBLE) { + i++; + } else if (result == ACTION_TAKEN_WITH_CALLBACK) { + break; + } + } + gpr_mu_unlock(&s->mu); } + /************************************************************* Cronet Callback Ipmlementation */ static void on_failed(cronet_bidirectional_stream *stream, int net_error) { - gpr_log(GPR_DEBUG, "on_failed(%p, %d)", stream, net_error); + CRONET_LOG(GPR_DEBUG, "on_failed(%p, %d)", stream, net_error); + stream_obj *s = (stream_obj *)stream->annotation; + cronet_bidirectional_stream_destroy(s->cbs); + s->state.state_callback_received[OP_FAILED] = true; + s->cbs = NULL; + execute_from_storage(s); } -static void on_succeeded(cronet_bidirectional_stream *stream) { - gpr_log(GPR_DEBUG, "on_succeeded(%p)", stream); +static void on_canceled(cronet_bidirectional_stream *stream) { + CRONET_LOG(GPR_DEBUG, "on_canceled(%p)", stream); + stream_obj *s = (stream_obj *)stream->annotation; + cronet_bidirectional_stream_destroy(s->cbs); + s->state.state_callback_received[OP_CANCELED] = true; + s->cbs = NULL; + execute_from_storage(s); } +static void on_succeeded(cronet_bidirectional_stream *stream) { + CRONET_LOG(GPR_DEBUG, "on_succeeded(%p)", stream); + stream_obj *s = (stream_obj *)stream->annotation; + cronet_bidirectional_stream_destroy(s->cbs); + s->cbs = NULL; +} static void on_request_headers_sent(cronet_bidirectional_stream *stream) { - gpr_log(GPR_DEBUG, "W: on_request_headers_sent(%p)", stream); + CRONET_LOG(GPR_DEBUG, "W: on_request_headers_sent(%p)", stream); stream_obj *s = (stream_obj *)stream->annotation; - s->state_op_done[OP_SEND_INITIAL_METADATA] = true; - s->state_callback_received[OP_SEND_INITIAL_METADATA] = true; - execute_curr_stream_op(s); + s->state.state_op_done[OP_SEND_INITIAL_METADATA] = true; + s->state.state_callback_received[OP_SEND_INITIAL_METADATA] = true; + execute_from_storage(s); } static void on_response_headers_received( cronet_bidirectional_stream *stream, const cronet_bidirectional_stream_header_array *headers, const char *negotiated_protocol) { - gpr_log(GPR_DEBUG, "R: on_response_headers_received(%p, %p, %s)", stream, + CRONET_LOG(GPR_DEBUG, "R: on_response_headers_received(%p, %p, %s)", stream, headers, negotiated_protocol); - stream_obj *s = (stream_obj *)stream->annotation; - memset(&s->rs.initial_metadata, 0, sizeof(s->rs.initial_metadata)); - grpc_chttp2_incoming_metadata_buffer_init(&s->rs.initial_metadata); + memset(&s->state.rs.initial_metadata, 0, sizeof(s->state.rs.initial_metadata)); + grpc_chttp2_incoming_metadata_buffer_init(&s->state.rs.initial_metadata); unsigned int i = 0; for (i = 0; i < headers->count; i++) { grpc_chttp2_incoming_metadata_buffer_add( - &s->rs.initial_metadata, + &s->state.rs.initial_metadata, grpc_mdelem_from_metadata_strings( grpc_mdstr_from_string(headers->headers[i].key), grpc_mdstr_from_string(headers->headers[i].value))); } - s->state_callback_received[OP_RECV_INITIAL_METADATA] = true; - execute_curr_stream_op(s); + s->state.state_callback_received[OP_RECV_INITIAL_METADATA] = true; + execute_from_storage(s); } static void on_write_completed(cronet_bidirectional_stream *stream, const char *data) { - gpr_log(GPR_DEBUG, "W: on_write_completed(%p, %s)", stream, data); stream_obj *s = (stream_obj *)stream->annotation; - if (s->ws.write_buffer) { - gpr_free(s->ws.write_buffer); - s->ws.write_buffer = NULL; + CRONET_LOG(GPR_DEBUG, "W: on_write_completed(%p, %s)", stream, data); + if (s->state.ws.write_buffer) { + gpr_free(s->state.ws.write_buffer); + s->state.ws.write_buffer = NULL; } - s->state_callback_received[OP_SEND_MESSAGE] = true; - execute_curr_stream_op(s); + s->state.state_callback_received[OP_SEND_MESSAGE] = true; + execute_from_storage(s); } static void on_read_completed(cronet_bidirectional_stream *stream, char *data, int count) { stream_obj *s = (stream_obj *)stream->annotation; - gpr_log(GPR_DEBUG, "R: on_read_completed(%p, %p, %d)", stream, data, count); + CRONET_LOG(GPR_DEBUG, "R: on_read_completed(%p, %p, %d)", stream, data, count); if (count > 0) { - s->rs.received_bytes += count; - s->rs.remaining_bytes -= count; - if (s->rs.remaining_bytes > 0) { - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_read"); - cronet_bidirectional_stream_read(s->cbs, s->rs.read_buffer + s->rs.received_bytes, s->rs.remaining_bytes); + s->state.rs.received_bytes += count; + s->state.rs.remaining_bytes -= count; + if (s->state.rs.remaining_bytes > 0) { + //CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); + s->state.state_op_done[OP_READ_REQ_MADE] = true; // If at least one read request has been made + cronet_bidirectional_stream_read(s->cbs, s->state.rs.read_buffer + s->state.rs.received_bytes, s->state.rs.remaining_bytes); } else { - execute_curr_stream_op(s); + execute_from_storage(s); } - s->state_callback_received[OP_RECV_MESSAGE] = true; + s->state.state_callback_received[OP_RECV_MESSAGE] = true; } } static void on_response_trailers_received( cronet_bidirectional_stream *stream, const cronet_bidirectional_stream_header_array *trailers) { - gpr_log(GPR_DEBUG, "R: on_response_trailers_received(%p,%p)", stream, + CRONET_LOG(GPR_DEBUG, "R: on_response_trailers_received(%p,%p)", stream, trailers); stream_obj *s = (stream_obj *)stream->annotation; - memset(&s->rs.trailing_metadata, 0, sizeof(s->rs.trailing_metadata)); - s->rs.trailing_metadata_valid = false; - grpc_chttp2_incoming_metadata_buffer_init(&s->rs.trailing_metadata); + memset(&s->state.rs.trailing_metadata, 0, sizeof(s->state.rs.trailing_metadata)); + s->state.rs.trailing_metadata_valid = false; + grpc_chttp2_incoming_metadata_buffer_init(&s->state.rs.trailing_metadata); unsigned int i = 0; for (i = 0; i < trailers->count; i++) { - gpr_log(GPR_DEBUG, "trailer key=%s, value=%s", trailers->headers[i].key, + CRONET_LOG(GPR_DEBUG, "trailer key=%s, value=%s", trailers->headers[i].key, trailers->headers[i].value); grpc_chttp2_incoming_metadata_buffer_add( - &s->rs.trailing_metadata, grpc_mdelem_from_metadata_strings( + &s->state.rs.trailing_metadata, grpc_mdelem_from_metadata_strings( grpc_mdstr_from_string(trailers->headers[i].key), grpc_mdstr_from_string(trailers->headers[i].value))); - s->rs.trailing_metadata_valid = true; + s->state.rs.trailing_metadata_valid = true; } - s->state_callback_received[OP_RECV_TRAILING_METADATA] = true; - execute_curr_stream_op(s); + s->state.state_callback_received[OP_RECV_TRAILING_METADATA] = true; + execute_from_storage(s); } /************************************************************* @@ -295,10 +380,10 @@ static void create_grpc_frame(gpr_slice_buffer *write_slice_buffer, memcpy(p, GPR_SLICE_START_PTR(slice), length); } -static void enqueue_callback(grpc_closure *callback) { +static void enqueue_callback(grpc_closure *callback, grpc_error *error) { GPR_ASSERT(callback); grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; - grpc_exec_ctx_sched(&exec_ctx, callback, GRPC_ERROR_NONE, NULL); + grpc_exec_ctx_sched(&exec_ctx, callback, error, NULL); grpc_exec_ctx_finish(&exec_ctx); } @@ -342,6 +427,7 @@ static void convert_metadata_to_cronet_headers( gpr_asprintf(pp_url, "https://%s%s", host, value); continue; } + gpr_log(GPR_DEBUG, "header %s = %s", key, value); headers[num_headers].key = key; headers[num_headers].value = value; num_headers++; @@ -365,165 +451,252 @@ static int parse_grpc_header(const uint8_t *data) { /* Op Execution */ - -static bool op_can_be_run(stream_obj *s, enum OP_ID op_id) { - if (op_id == OP_SEND_INITIAL_METADATA) { +static bool op_can_be_run(grpc_transport_stream_op *curr_op, struct op_state *stream_state, struct op_state *op_state, enum OP_ID op_id) { + // We use op's own state for most state, except for metadata related callbacks, which + // are at the stream level. TODO: WTF does this comment mean? + bool result = true; + // When call is canceled, every op can be run + if (stream_state->state_op_done[OP_CANCEL_ERROR] || stream_state->state_callback_received[OP_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; + if (op_id == OP_CANCEL_ERROR) result = false; // already executed - if (s->state_op_done[OP_SEND_INITIAL_METADATA]) return false; - } - if (op_id == OP_RECV_INITIAL_METADATA) { + if (op_id == OP_RECV_INITIAL_METADATA && stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) result = false; + if (op_id == OP_RECV_MESSAGE && stream_state->state_op_done[OP_RECV_MESSAGE]) result = false; + if (op_id == OP_RECV_TRAILING_METADATA && stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) result = false; + } else if (op_id == OP_SEND_INITIAL_METADATA) { + // already executed + if (stream_state->state_op_done[OP_SEND_INITIAL_METADATA]) result = false; + } else if (op_id == OP_RECV_INITIAL_METADATA) { // already executed - if (s->state_op_done[OP_RECV_INITIAL_METADATA]) return false; + if (stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) result = false; // we haven't sent headers yet. - if (!s->state_callback_received[OP_SEND_INITIAL_METADATA]) return false; + else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; // we haven't received headers yet. - if (!s->state_callback_received[OP_RECV_INITIAL_METADATA]) return false; - } - if (op_id == OP_SEND_MESSAGE) { + else if (!stream_state->state_callback_received[OP_RECV_INITIAL_METADATA]) result = false; + } else if (op_id == OP_SEND_MESSAGE) { // already executed - if (s->state_op_done[OP_SEND_MESSAGE]) return false; - // we haven't received headers yet. - if (!s->state_callback_received[OP_RECV_INITIAL_METADATA]) return false; - } - if (op_id == OP_RECV_MESSAGE) { + if (stream_state->state_op_done[OP_SEND_MESSAGE]) result = false; + // we haven't sent headers yet. + else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; + } else if (op_id == OP_RECV_MESSAGE) { // already executed - if (s->state_op_done[OP_RECV_MESSAGE]) return false; + if (op_state->state_op_done[OP_RECV_MESSAGE]) result = false; // we haven't received headers yet. - if (!s->state_callback_received[OP_RECV_INITIAL_METADATA]) return false; - } - if (op_id == OP_RECV_TRAILING_METADATA) { + else if (!stream_state->state_callback_received[OP_RECV_INITIAL_METADATA]) result = false; + } else if (op_id == OP_RECV_TRAILING_METADATA) { // already executed - if (s->state_op_done[OP_RECV_TRAILING_METADATA]) return false; + if (stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) result = false; + // we have asked for but haven't received message yet. + else if (stream_state->state_op_done[OP_READ_REQ_MADE] && !stream_state->state_op_done[OP_RECV_MESSAGE]) result = false; // we haven't received trailers yet. - if (!s->state_callback_received[OP_RECV_TRAILING_METADATA]) return false; - } - if (op_id == OP_SEND_TRAILING_METADATA) { + else if (!stream_state->state_callback_received[OP_RECV_TRAILING_METADATA]) result = false; + } else if (op_id == OP_SEND_TRAILING_METADATA) { // already executed - if (s->state_op_done[OP_SEND_TRAILING_METADATA]) return false; + if (stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) result = false; + // we haven't sent initial metadata yet + else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; // we haven't sent message yet - if (s->curr_op->send_message && !s->state_op_done[OP_SEND_MESSAGE]) return false; - } - - if (op_id == OP_ON_COMPLETE) { + else if (curr_op->send_message && !stream_state->state_op_done[OP_SEND_MESSAGE]) result = false; + // we haven't got on_write_completed for the send yet + else if (curr_op->send_message && !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; + } else if (op_id == OP_CANCEL_ERROR) { // already executed - if (s->state_op_done[OP_ON_COMPLETE]) return false; + if (stream_state->state_op_done[OP_CANCEL_ERROR]) result = false; + } else if (op_id == OP_ON_COMPLETE) { + // already executed (note we're checking op specific state, not stream state) + if (op_state->state_op_done[OP_ON_COMPLETE]) result = false; // Check if every op that was asked for is done. - if (s->curr_op->send_initial_metadata && !s->state_op_done[OP_SEND_INITIAL_METADATA]) return false; - if (s->curr_op->send_message && !s->state_op_done[OP_SEND_MESSAGE]) return false; - if (s->curr_op->send_trailing_metadata && !s->state_op_done[OP_SEND_TRAILING_METADATA]) return false; - if (s->curr_op->recv_initial_metadata && !s->state_op_done[OP_RECV_INITIAL_METADATA]) return false; - if (s->curr_op->recv_message && !s->state_op_done[OP_RECV_MESSAGE]) return false; - if (s->curr_op->recv_trailing_metadata && !s->state_op_done[OP_RECV_TRAILING_METADATA]) return false; + else if (curr_op->send_initial_metadata && !stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; + else if (curr_op->send_message && !stream_state->state_op_done[OP_SEND_MESSAGE]) result = false; + else if (curr_op->send_trailing_metadata && !stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) result = false; + else if (curr_op->recv_initial_metadata && !stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) result = false; + else if (curr_op->recv_message && !stream_state->state_op_done[OP_RECV_MESSAGE]) result = false; + else if (curr_op->recv_trailing_metadata) { + // We aren't done with trailing metadata yet + if (!stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) result = false; + // We've asked for actual message in an earlier op, and it hasn't been delivered yet. + // (TODO: What happens when multiple messages are asked for? How do you know when last message arrived?) + else if (stream_state->state_op_done[OP_READ_REQ_MADE]) { + // If this op is not the one asking for read, (which means some earlier op has asked), and the + // read hasn't been delivered. + if(!curr_op->recv_message && !stream_state->state_op_done[OP_RECV_MESSAGE_AND_ON_COMPLETE]) result = false; + } + } + // We should see at least one on_write_completed for the trailers that we sent + else if (curr_op->send_trailing_metadata && !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; } - return true; + CRONET_LOG(GPR_DEBUG, "op_can_be_run %s : %s", op_id_string[op_id], result? "YES":"NO"); + return result; } -static void execute_curr_stream_op(stream_obj *s) { - if (s->curr_op->send_initial_metadata && op_can_be_run(s, OP_SEND_INITIAL_METADATA)) { +static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { + // TODO TODO : This can be called from network thread and main thread. add a mutex. + grpc_transport_stream_op *stream_op = &oas->op; + struct stream_obj *s = oas->s; + struct op_state *stream_state = &s->state; + //CRONET_LOG(GPR_DEBUG, "execute_stream_op"); + enum OP_RESULT result = NO_ACTION_POSSIBLE; + if (stream_op->send_initial_metadata && 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 - memset(&s->rs, 0, sizeof(s->rs)); - memset(&s->ws, 0, sizeof(s->ws)); - memset(s->state_op_done, 0, sizeof(s->state_op_done)); - memset(s->state_callback_received, 0, sizeof(s->state_callback_received)); + 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 GPR_ASSERT(s->cbs == NULL); - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_create"); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_create"); s->cbs = cronet_bidirectional_stream_create(s->curr_ct.engine, s->curr_gs, &cronet_callbacks); char *url; - convert_metadata_to_cronet_headers(s->curr_op->send_initial_metadata->list.head, + convert_metadata_to_cronet_headers(stream_op->send_initial_metadata->list.head, s->curr_ct.host, &url, &header_array.headers, &header_array.count); header_array.capacity = header_array.count; - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_start"); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_start %s", url); cronet_bidirectional_stream_start(s->cbs, url, 0, "POST", &header_array, false); - s->state_op_done[OP_SEND_INITIAL_METADATA] = true; - } else if (s->curr_op->recv_initial_metadata && - op_can_be_run(s, OP_RECV_INITIAL_METADATA)) { - grpc_chttp2_incoming_metadata_buffer_publish(&s->rs.initial_metadata, - s->curr_op->recv_initial_metadata); - enqueue_callback(s->curr_op->recv_initial_metadata_ready); - s->state_op_done[OP_RECV_INITIAL_METADATA] = true; - // We are ready to execute send_message. - execute_curr_stream_op(s); - } else if (s->curr_op->send_message && op_can_be_run(s, OP_SEND_MESSAGE)) { + stream_state->state_op_done[OP_SEND_INITIAL_METADATA] = true; + result = ACTION_TAKEN_WITH_CALLBACK; + } else if (stream_op->recv_initial_metadata && + 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]) { + grpc_chttp2_incoming_metadata_buffer_publish(&oas->s->state.rs.initial_metadata, + stream_op->recv_initial_metadata); + enqueue_callback(stream_op->recv_initial_metadata_ready, GRPC_ERROR_NONE); + } else { + enqueue_callback(stream_op->recv_initial_metadata_ready, GRPC_ERROR_CANCELLED); + } + stream_state->state_op_done[OP_RECV_INITIAL_METADATA] = true; + result = ACTION_TAKEN_NO_CALLBACK; + } else if (stream_op->send_message && op_can_be_run(stream_op, stream_state, &oas->state, OP_SEND_MESSAGE)) { + CRONET_LOG(GPR_DEBUG, "running: %p OP_SEND_MESSAGE", oas); // TODO (makdharma): Make into a standalone function gpr_slice_buffer write_slice_buffer; gpr_slice slice; gpr_slice_buffer_init(&write_slice_buffer); - grpc_byte_stream_next(NULL, s->curr_op->send_message, &slice, - s->curr_op->send_message->length, NULL); + grpc_byte_stream_next(NULL, stream_op->send_message, &slice, + stream_op->send_message->length, NULL); // Check that compression flag is not ON. We don't support compression yet. // TODO (makdharma): add compression support - GPR_ASSERT(s->curr_op->send_message->flags == 0); + GPR_ASSERT(stream_op->send_message->flags == 0); gpr_slice_buffer_add(&write_slice_buffer, slice); GPR_ASSERT(write_slice_buffer.count == 1); // Empty request not handled yet if (write_slice_buffer.count > 0) { int write_buffer_size; - create_grpc_frame(&write_slice_buffer, &s->ws.write_buffer, &write_buffer_size); - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_write (%p)", s->ws.write_buffer); - cronet_bidirectional_stream_write(s->cbs, s->ws.write_buffer, + create_grpc_frame(&write_slice_buffer, &stream_state->ws.write_buffer, &write_buffer_size); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p)", stream_state->ws.write_buffer); + stream_state->state_callback_received[OP_SEND_MESSAGE] = false; + cronet_bidirectional_stream_write(s->cbs, stream_state->ws.write_buffer, write_buffer_size, false); // TODO: What if this is not the last write? + result = ACTION_TAKEN_WITH_CALLBACK; } - s->state_op_done[OP_SEND_MESSAGE] = true; - } else if (s->curr_op->recv_message && op_can_be_run(s, OP_RECV_MESSAGE)) { - if (s->rs.length_field_received == false) { - if (s->rs.received_bytes == GRPC_HEADER_SIZE_IN_BYTES && s->rs.remaining_bytes == 0) { + stream_state->state_op_done[OP_SEND_MESSAGE] = true; + } else if (stream_op->recv_message && 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]) { + enqueue_callback(stream_op->recv_message_ready, GRPC_ERROR_CANCELLED); + stream_state->state_op_done[OP_RECV_MESSAGE] = true; + } 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) { // Start a read operation for data - s->rs.length_field_received = true; - s->rs.length_field = s->rs.remaining_bytes = - parse_grpc_header((const uint8_t *)s->rs.read_buffer); - GPR_ASSERT(s->rs.length_field > 0); // Empty message? - gpr_log(GPR_DEBUG, "length field = %d", s->rs.length_field); - s->rs.read_buffer = gpr_malloc(s->rs.length_field); - GPR_ASSERT(s->rs.read_buffer); - s->rs.remaining_bytes = s->rs.length_field; - s->rs.received_bytes = 0; - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_read"); - cronet_bidirectional_stream_read(s->cbs, s->rs.read_buffer, - s->rs.remaining_bytes); - } else if (s->rs.remaining_bytes == 0) { + stream_state->rs.length_field_received = true; + stream_state->rs.length_field = stream_state->rs.remaining_bytes = + parse_grpc_header((const uint8_t *)stream_state->rs.read_buffer); + CRONET_LOG(GPR_DEBUG, "length field = %d", stream_state->rs.length_field); + if (stream_state->rs.length_field > 0) { + stream_state->rs.read_buffer = gpr_malloc(stream_state->rs.length_field); + GPR_ASSERT(stream_state->rs.read_buffer); + stream_state->rs.remaining_bytes = stream_state->rs.length_field; + stream_state->rs.received_bytes = 0; + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); + stream_state->state_op_done[OP_READ_REQ_MADE] = true; // If at least one read request has been made + cronet_bidirectional_stream_read(s->cbs, stream_state->rs.read_buffer, + stream_state->rs.remaining_bytes); + result = ACTION_TAKEN_WITH_CALLBACK; + } else { + stream_state->rs.remaining_bytes = 0; + CRONET_LOG(GPR_DEBUG, "read operation complete. Empty response."); + gpr_slice_buffer_init(&stream_state->rs.read_slice_buffer); + grpc_slice_buffer_stream_init(&stream_state->rs.sbs, &stream_state->rs.read_slice_buffer, 0); + *((grpc_byte_buffer **)stream_op->recv_message) = (grpc_byte_buffer *)&stream_state->rs.sbs; + enqueue_callback(stream_op->recv_message_ready, GRPC_ERROR_NONE); + stream_state->state_op_done[OP_RECV_MESSAGE] = true; + oas->state.state_op_done[OP_RECV_MESSAGE] = true; // Also set per op state. + result = ACTION_TAKEN_NO_CALLBACK; + } + } else if (stream_state->rs.remaining_bytes == 0) { // Start a read operation for first 5 bytes (GRPC header) - s->rs.read_buffer = s->rs.grpc_header_bytes; - s->rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; - s->rs.received_bytes = 0; - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_read"); - cronet_bidirectional_stream_read(s->cbs, s->rs.read_buffer, - s->rs.remaining_bytes); + stream_state->rs.read_buffer = stream_state->rs.grpc_header_bytes; + stream_state->rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; + stream_state->rs.received_bytes = 0; + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); + stream_state->state_op_done[OP_READ_REQ_MADE] = true; // If at least one read request has been made + cronet_bidirectional_stream_read(s->cbs, stream_state->rs.read_buffer, + stream_state->rs.remaining_bytes); } - } else if (s->rs.remaining_bytes == 0) { - gpr_log(GPR_DEBUG, "read operation complete"); - gpr_slice read_data_slice = gpr_slice_malloc((uint32_t)s->rs.length_field); + result = ACTION_TAKEN_WITH_CALLBACK; + } else if (stream_state->rs.remaining_bytes == 0) { + CRONET_LOG(GPR_DEBUG, "read operation complete"); + gpr_slice read_data_slice = gpr_slice_malloc((uint32_t)stream_state->rs.length_field); uint8_t *dst_p = GPR_SLICE_START_PTR(read_data_slice); - memcpy(dst_p, s->rs.read_buffer, (size_t)s->rs.length_field); - gpr_slice_buffer_init(&s->rs.read_slice_buffer); - gpr_slice_buffer_add(&s->rs.read_slice_buffer, read_data_slice); - grpc_slice_buffer_stream_init(&s->rs.sbs, &s->rs.read_slice_buffer, 0); - *((grpc_byte_buffer **)s->curr_op->recv_message) = (grpc_byte_buffer *)&s->rs.sbs; - enqueue_callback(s->curr_op->recv_message_ready); - s->state_op_done[OP_RECV_MESSAGE] = true; - execute_curr_stream_op(s); + memcpy(dst_p, stream_state->rs.read_buffer, (size_t)stream_state->rs.length_field); + gpr_slice_buffer_init(&stream_state->rs.read_slice_buffer); + gpr_slice_buffer_add(&stream_state->rs.read_slice_buffer, read_data_slice); + grpc_slice_buffer_stream_init(&stream_state->rs.sbs, &stream_state->rs.read_slice_buffer, 0); + *((grpc_byte_buffer **)stream_op->recv_message) = (grpc_byte_buffer *)&stream_state->rs.sbs; + enqueue_callback(stream_op->recv_message_ready, GRPC_ERROR_NONE); + stream_state->state_op_done[OP_RECV_MESSAGE] = true; + oas->state.state_op_done[OP_RECV_MESSAGE] = true; // Also set per op state. + // 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; } - } else if (s->curr_op->recv_trailing_metadata && - op_can_be_run(s, OP_RECV_TRAILING_METADATA)) { - if (s->rs.trailing_metadata_valid) { + } else if (stream_op->recv_trailing_metadata && + op_can_be_run(stream_op, stream_state, &oas->state, OP_RECV_TRAILING_METADATA)) { + CRONET_LOG(GPR_DEBUG, "running: %p OP_RECV_TRAILING_METADATA", oas); + if (oas->s->state.rs.trailing_metadata_valid) { grpc_chttp2_incoming_metadata_buffer_publish( - &s->rs.trailing_metadata, s->curr_op->recv_trailing_metadata); - s->rs.trailing_metadata_valid = false; + &oas->s->state.rs.trailing_metadata, stream_op->recv_trailing_metadata); + stream_state->rs.trailing_metadata_valid = false; } - s->state_op_done[OP_RECV_TRAILING_METADATA] = true; - execute_curr_stream_op(s); - } else if (s->curr_op->send_trailing_metadata && - op_can_be_run(s, OP_SEND_TRAILING_METADATA)) { - - gpr_log(GPR_DEBUG, "cronet_bidirectional_stream_write (0)"); + stream_state->state_op_done[OP_RECV_TRAILING_METADATA] = true; + result = ACTION_TAKEN_NO_CALLBACK; + } else if (stream_op->send_trailing_metadata && op_can_be_run(stream_op, stream_state, &oas->state, OP_SEND_TRAILING_METADATA)) { + CRONET_LOG(GPR_DEBUG, "running: %p OP_SEND_TRAILING_METADATA", oas); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (0)"); + stream_state->state_callback_received[OP_SEND_MESSAGE] = false; cronet_bidirectional_stream_write(s->cbs, "", 0, true); - s->state_op_done[OP_SEND_TRAILING_METADATA] = true; - } else if (op_can_be_run(s, OP_ON_COMPLETE)) { + stream_state->state_op_done[OP_SEND_TRAILING_METADATA] = true; + result = ACTION_TAKEN_WITH_CALLBACK; + } else if (stream_op->cancel_error && op_can_be_run(stream_op, stream_state, &oas->state, OP_CANCEL_ERROR)) { + CRONET_LOG(GPR_DEBUG, "running: %p OP_CANCEL_ERROR", oas); + CRONET_LOG(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); + // Cancel might have come before creation of stream + if (s->cbs) { + cronet_bidirectional_stream_cancel(s->cbs); + } + stream_state->state_op_done[OP_CANCEL_ERROR] = true; + result = ACTION_TAKEN_WITH_CALLBACK; + } else if (stream_op->on_complete && op_can_be_run(stream_op, stream_state, &oas->state, OP_ON_COMPLETE)) { // All ops are complete. Call the on_complete callback - enqueue_callback(s->curr_op->on_complete); - s->state_op_done[OP_ON_COMPLETE] = true; - cronet_bidirectional_stream_destroy(s->cbs); - s->cbs = NULL; + CRONET_LOG(GPR_DEBUG, "running: %p OP_ON_COMPLETE", oas); + //CRONET_LOG(GPR_DEBUG, "calling on_complete"); + enqueue_callback(stream_op->on_complete, GRPC_ERROR_NONE); + // Instead of setting stream state, use the op state as on_complete is on per op basis + oas->state.state_op_done[OP_ON_COMPLETE] = true; + oas->done = true; // Mark this op as completed + // reset any send or receive message state. + stream_state->state_callback_received[OP_SEND_MESSAGE] = false; + stream_state->state_op_done[OP_SEND_MESSAGE] = false; + result = ACTION_TAKEN_NO_CALLBACK; + // If this is the on_complete callback being called for a received message - make a note + if (stream_op->recv_message) stream_state->state_op_done[OP_RECV_MESSAGE_AND_ON_COMPLETE] = true; + } else { + //CRONET_LOG(GPR_DEBUG, "No op ready to run"); + result = NO_ACTION_POSSIBLE; } + return result; } //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -533,7 +706,14 @@ static int init_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, const void *server_data) { stream_obj *s = (stream_obj *)gs; memset(&s->storage, 0, sizeof(s->storage)); + memset(&s->state, 0, sizeof(s->state)); s->curr_op = NULL; + s->cbs = NULL; + memset(&s->state.rs, 0, sizeof(s->state.rs)); + memset(&s->state.ws, 0, sizeof(s->state.ws)); + 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)); + gpr_mu_init(&s->mu); return 0; } @@ -546,15 +726,12 @@ static void set_pollset_set_do_nothing(grpc_exec_ctx *exec_ctx, static void perform_stream_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, grpc_stream *gs, grpc_transport_stream_op *op) { - gpr_log(GPR_DEBUG, "perform_stream_op"); + CRONET_LOG(GPR_DEBUG, "perform_stream_op"); stream_obj *s = (stream_obj *)gs; - memcpy(&s->curr_ct, gt, sizeof(grpc_cronet_transport)); - add_pending_op(&s->storage, op); - if (s->curr_op == NULL) { - s->curr_op = pop_pending_op(&s->storage); - } s->curr_gs = gs; - execute_curr_stream_op(s); + memcpy(&s->curr_ct, gt, sizeof(grpc_cronet_transport)); + add_to_storage(s, op); + execute_from_storage(s); } static void destroy_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, From 198f8b01946ac107700958ac6c2ea11ed523f2a5 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Wed, 10 Aug 2016 13:44:13 -0700 Subject: [PATCH 06/14] more fixes --- .../cronet/transport/cronet_transport.c | 42 +++++++++++-------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index e8746b4e6e6..dc6a780edaf 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -75,6 +75,7 @@ enum OP_ID { OP_CANCEL_ERROR, OP_ON_COMPLETE, OP_FAILED, + OP_SUCCEEDED, OP_CANCELED, OP_RECV_MESSAGE_AND_ON_COMPLETE, OP_READ_REQ_MADE, @@ -91,6 +92,7 @@ const char *op_id_string[] = { "OP_CANCEL_ERROR", "OP_ON_COMPLETE", "OP_FAILED", + "OP_SUCCEEDED", "OP_CANCELED", "OP_RECV_MESSAGE_AND_ON_COMPLETE", "OP_READ_REQ_MADE", @@ -189,6 +191,8 @@ struct stream_obj { grpc_stream *curr_gs; cronet_bidirectional_stream *cbs; + // Used for executing callbacks for ops + grpc_exec_ctx exec_ctx; // This holds the state that is at stream level (response and req metadata) struct op_state state; @@ -227,7 +231,10 @@ static void add_to_storage(struct stream_obj *s, grpc_transport_stream_op *op) { static void execute_from_storage(stream_obj *s) { // Cycle through ops and try to take next action. Break when either // an action with callback is taken, or no action is possible. - gpr_mu_lock(&s->mu); + // This can be executed from the Cronet network thread via cronet callback + // or on the application supplied thread via the perform_stream_op function. + if (1) {//gpr_mu_lock(&s->mu) == 0) { + gpr_mu_lock(&s->mu); for (int i = 0; i < s->storage.wrptr; ) { CRONET_LOG(GPR_DEBUG, "calling execute_stream_op[%d]. done = %d", i, s->storage.pending_ops[i].done); if (s->storage.pending_ops[i].done) { @@ -242,7 +249,9 @@ static void execute_from_storage(stream_obj *s) { break; } } - gpr_mu_unlock(&s->mu); + gpr_mu_unlock(&s->mu); + } + grpc_exec_ctx_finish(&s->exec_ctx); } @@ -271,7 +280,9 @@ static void on_succeeded(cronet_bidirectional_stream *stream) { CRONET_LOG(GPR_DEBUG, "on_succeeded(%p)", stream); stream_obj *s = (stream_obj *)stream->annotation; cronet_bidirectional_stream_destroy(s->cbs); + s->state.state_callback_received[OP_FAILED] = true; s->cbs = NULL; + execute_from_storage(s); } static void on_request_headers_sent(cronet_bidirectional_stream *stream) { @@ -380,13 +391,6 @@ static void create_grpc_frame(gpr_slice_buffer *write_slice_buffer, memcpy(p, GPR_SLICE_START_PTR(slice), length); } -static void enqueue_callback(grpc_closure *callback, grpc_error *error) { - GPR_ASSERT(callback); - grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; - grpc_exec_ctx_sched(&exec_ctx, callback, error, NULL); - grpc_exec_ctx_finish(&exec_ctx); -} - static void convert_metadata_to_cronet_headers( grpc_linked_mdelem *head, const char *host, @@ -498,9 +502,10 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, struct op_state *st // we haven't sent initial metadata yet else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; // we haven't sent message yet + // TODO: Streaming Write case is a problem. What if there is an outstanding write (2nd, 3rd,..) present. else if (curr_op->send_message && !stream_state->state_op_done[OP_SEND_MESSAGE]) result = false; // we haven't got on_write_completed for the send yet - else if (curr_op->send_message && !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; + else if (stream_state->state_op_done[OP_SEND_MESSAGE] && !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; } else if (op_id == OP_CANCEL_ERROR) { // already executed if (stream_state->state_op_done[OP_CANCEL_ERROR]) result = false; @@ -510,10 +515,12 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, struct op_state *st // Check if every op that was asked for is done. else if (curr_op->send_initial_metadata && !stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; else if (curr_op->send_message && !stream_state->state_op_done[OP_SEND_MESSAGE]) result = false; + else if (curr_op->send_message && !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; else if (curr_op->send_trailing_metadata && !stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) result = false; else if (curr_op->recv_initial_metadata && !stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) result = false; else if (curr_op->recv_message && !stream_state->state_op_done[OP_RECV_MESSAGE]) result = false; else if (curr_op->recv_trailing_metadata) { + //if (!stream_state->state_op_done[OP_SUCCEEDED]) result = false; gpr_log(GPR_DEBUG, "HACK!!"); // We aren't done with trailing metadata yet if (!stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) result = false; // We've asked for actual message in an earlier op, and it hasn't been delivered yet. @@ -521,7 +528,7 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, struct op_state *st else if (stream_state->state_op_done[OP_READ_REQ_MADE]) { // If this op is not the one asking for read, (which means some earlier op has asked), and the // read hasn't been delivered. - if(!curr_op->recv_message && !stream_state->state_op_done[OP_RECV_MESSAGE_AND_ON_COMPLETE]) result = false; + if(!curr_op->recv_message && !stream_state->state_op_done[OP_SUCCEEDED]) result = false; } } // We should see at least one on_write_completed for the trailers that we sent @@ -563,9 +570,9 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { if (!stream_state->state_op_done[OP_CANCEL_ERROR]) { grpc_chttp2_incoming_metadata_buffer_publish(&oas->s->state.rs.initial_metadata, stream_op->recv_initial_metadata); - enqueue_callback(stream_op->recv_initial_metadata_ready, GRPC_ERROR_NONE); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_initial_metadata_ready, GRPC_ERROR_NONE, NULL); } else { - enqueue_callback(stream_op->recv_initial_metadata_ready, GRPC_ERROR_CANCELLED); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_initial_metadata_ready, GRPC_ERROR_CANCELLED, NULL); } stream_state->state_op_done[OP_RECV_INITIAL_METADATA] = true; result = ACTION_TAKEN_NO_CALLBACK; @@ -595,7 +602,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { } else if (stream_op->recv_message && 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]) { - enqueue_callback(stream_op->recv_message_ready, GRPC_ERROR_CANCELLED); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_CANCELLED, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; } 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) { @@ -620,7 +627,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { gpr_slice_buffer_init(&stream_state->rs.read_slice_buffer); grpc_slice_buffer_stream_init(&stream_state->rs.sbs, &stream_state->rs.read_slice_buffer, 0); *((grpc_byte_buffer **)stream_op->recv_message) = (grpc_byte_buffer *)&stream_state->rs.sbs; - enqueue_callback(stream_op->recv_message_ready, GRPC_ERROR_NONE); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; oas->state.state_op_done[OP_RECV_MESSAGE] = true; // Also set per op state. result = ACTION_TAKEN_NO_CALLBACK; @@ -645,7 +652,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { gpr_slice_buffer_add(&stream_state->rs.read_slice_buffer, read_data_slice); grpc_slice_buffer_stream_init(&stream_state->rs.sbs, &stream_state->rs.read_slice_buffer, 0); *((grpc_byte_buffer **)stream_op->recv_message) = (grpc_byte_buffer *)&stream_state->rs.sbs; - enqueue_callback(stream_op->recv_message_ready, GRPC_ERROR_NONE); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; oas->state.state_op_done[OP_RECV_MESSAGE] = true; // Also set per op state. // Clear read state of the stream, so next read op (if it were to come) will work @@ -682,7 +689,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { // All ops are complete. Call the on_complete callback CRONET_LOG(GPR_DEBUG, "running: %p OP_ON_COMPLETE", oas); //CRONET_LOG(GPR_DEBUG, "calling on_complete"); - enqueue_callback(stream_op->on_complete, GRPC_ERROR_NONE); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->on_complete, GRPC_ERROR_NONE, NULL); // Instead of setting stream state, use the op state as on_complete is on per op basis oas->state.state_op_done[OP_ON_COMPLETE] = true; oas->done = true; // Mark this op as completed @@ -714,6 +721,7 @@ 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)); gpr_mu_init(&s->mu); + s->exec_ctx = *exec_ctx; return 0; } From 35da822b442a60d5e9d0711cc046f9c578d63ad9 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Wed, 10 Aug 2016 14:53:40 -0700 Subject: [PATCH 07/14] WIP --- src/core/ext/transport/cronet/transport/cronet_transport.c | 7 ++++--- 1 file changed, 4 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 dc6a780edaf..7e65def4de1 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -280,7 +280,7 @@ static void on_succeeded(cronet_bidirectional_stream *stream) { CRONET_LOG(GPR_DEBUG, "on_succeeded(%p)", stream); stream_obj *s = (stream_obj *)stream->annotation; cronet_bidirectional_stream_destroy(s->cbs); - s->state.state_callback_received[OP_FAILED] = true; + s->state.state_callback_received[OP_SUCCEEDED] = true; s->cbs = NULL; execute_from_storage(s); } @@ -480,8 +480,8 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, struct op_state *st // we haven't received headers yet. else if (!stream_state->state_callback_received[OP_RECV_INITIAL_METADATA]) result = false; } else if (op_id == OP_SEND_MESSAGE) { - // already executed - if (stream_state->state_op_done[OP_SEND_MESSAGE]) result = false; + // already executed (note we're checking op specific state, not stream state) + if (op_state->state_op_done[OP_SEND_MESSAGE]) result = false; // we haven't sent headers yet. else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; } else if (op_id == OP_RECV_MESSAGE) { @@ -599,6 +599,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { result = ACTION_TAKEN_WITH_CALLBACK; } stream_state->state_op_done[OP_SEND_MESSAGE] = true; + oas->state.state_op_done[OP_SEND_MESSAGE] = true; } else if (stream_op->recv_message && 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]) { From f66a37b63aed77732c8d7d253af89296a23bb72e Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Wed, 10 Aug 2016 17:47:25 -0700 Subject: [PATCH 08/14] WIP --- .../cronet/transport/cronet_transport.c | 597 +++++++++++------- 1 file changed, 356 insertions(+), 241 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 7e65def4de1..1d756039809 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -50,8 +50,17 @@ #include "third_party/objective_c/Cronet/cronet_c_for_grpc.h" #define GRPC_HEADER_SIZE_IN_BYTES 5 +// maximum ops in a batch. There is not much thinking behind this limit, except +// that it seems to be enough for most use cases. +#define MAX_PENDING_OPS 100 + +#define CRONET_LOG(...) \ + { \ + if (grpc_cronet_trace) gpr_log(__VA_ARGS__); \ + } -#define CRONET_LOG(...) {if (1) gpr_log(__VA_ARGS__);} +// TODO (makdharma): Hook up into the wider tracing mechanism +int grpc_cronet_trace = 1; enum OP_RESULT { ACTION_TAKEN_WITH_CALLBACK, @@ -59,11 +68,10 @@ enum OP_RESULT { NO_ACTION_POSSIBLE }; -const char *OP_RESULT_STRING[] = { - "ACTION_TAKEN_WITH_CALLBACK", - "ACTION_TAKEN_NO_CALLBACK", - "NO_ACTION_POSSIBLE" -}; +// Used for printing debug +const char *op_result_string[] = {"ACTION_TAKEN_WITH_CALLBACK", + "ACTION_TAKEN_NO_CALLBACK", + "NO_ACTION_POSSIBLE"}; enum OP_ID { OP_SEND_INITIAL_METADATA = 0, @@ -82,32 +90,31 @@ enum OP_ID { OP_NUM_OPS }; -const char *op_id_string[] = { - "OP_SEND_INITIAL_METADATA", - "OP_SEND_MESSAGE", - "OP_SEND_TRAILING_METADATA", - "OP_RECV_MESSAGE", - "OP_RECV_INITIAL_METADATA", - "OP_RECV_TRAILING_METADATA", - "OP_CANCEL_ERROR", - "OP_ON_COMPLETE", - "OP_FAILED", - "OP_SUCCEEDED", - "OP_CANCELED", - "OP_RECV_MESSAGE_AND_ON_COMPLETE", - "OP_READ_REQ_MADE", - "OP_NUM_OPS" -}; +const char *op_id_string[] = {"OP_SEND_INITIAL_METADATA", + "OP_SEND_MESSAGE", + "OP_SEND_TRAILING_METADATA", + "OP_RECV_MESSAGE", + "OP_RECV_INITIAL_METADATA", + "OP_RECV_TRAILING_METADATA", + "OP_CANCEL_ERROR", + "OP_ON_COMPLETE", + "OP_FAILED", + "OP_SUCCEEDED", + "OP_CANCELED", + "OP_RECV_MESSAGE_AND_ON_COMPLETE", + "OP_READ_REQ_MADE", + "OP_NUM_OPS"}; /* Cronet callbacks */ static void on_request_headers_sent(cronet_bidirectional_stream *); -static void on_response_headers_received(cronet_bidirectional_stream *, - const cronet_bidirectional_stream_header_array *, - const char *); +static void on_response_headers_received( + cronet_bidirectional_stream *, + const cronet_bidirectional_stream_header_array *, const char *); static void on_write_completed(cronet_bidirectional_stream *, const char *); static void on_read_completed(cronet_bidirectional_stream *, char *, int); -static void on_response_trailers_received(cronet_bidirectional_stream *, +static void on_response_trailers_received( + cronet_bidirectional_stream *, const cronet_bidirectional_stream_header_array *); static void on_succeeded(cronet_bidirectional_stream *); static void on_failed(cronet_bidirectional_stream *, int); @@ -120,8 +127,7 @@ static cronet_bidirectional_stream_callback cronet_callbacks = { on_response_trailers_received, on_succeeded, on_failed, - on_canceled -}; + on_canceled}; // Cronet transport object struct grpc_cronet_transport { @@ -132,7 +138,7 @@ struct grpc_cronet_transport { typedef struct grpc_cronet_transport grpc_cronet_transport; struct read_state { - // vars to store data coming from cronet + /* vars to store data coming from server */ char *read_buffer; bool length_field_received; int received_bytes; @@ -141,15 +147,15 @@ struct read_state { char grpc_header_bytes[GRPC_HEADER_SIZE_IN_BYTES]; char *payload_field; - // vars for holding data destined for the application + /* vars for holding data destined for the application */ struct grpc_slice_buffer_stream sbs; gpr_slice_buffer read_slice_buffer; - // vars for trailing metadata + /* vars for trailing metadata */ grpc_chttp2_incoming_metadata_buffer trailing_metadata; bool trailing_metadata_valid; - // vars for initial metadata + /* vars for initial metadata */ grpc_chttp2_incoming_metadata_buffer initial_metadata; }; @@ -157,16 +163,13 @@ struct write_state { char *write_buffer; }; -// maximum ops in a batch.. There is not much thinking behind this limit, except -// that it seems to be enough for most use cases. -#define MAX_PENDING_OPS 100 - +/* track state of one stream op */ struct op_state { bool state_op_done[OP_NUM_OPS]; bool state_callback_received[OP_NUM_OPS]; - // data structure for storing data coming from server + /* data structure for storing data coming from server */ struct read_state rs; - // data structure for storing data going to the server + /* data structure for storing data going to the server */ struct write_state ws; }; @@ -174,7 +177,7 @@ struct op_and_state { grpc_transport_stream_op op; struct op_state state; bool done; - struct stream_obj *s; // Pointer back to the stream object + struct stream_obj *s; /* Pointer back to the stream object */ }; struct op_storage { @@ -190,73 +193,74 @@ struct stream_obj { grpc_cronet_transport curr_ct; grpc_stream *curr_gs; cronet_bidirectional_stream *cbs; + cronet_bidirectional_stream_header_array header_array; - // Used for executing callbacks for ops + /* Used for executing callbacks for ops */ grpc_exec_ctx exec_ctx; - // This holds the state that is at stream level (response and req metadata) + /* Stream level state. Some state will be tracked both at stream and stream_op + * level */ struct op_state state; - //struct op_state state; - // OP storage + /* OP storage */ struct op_storage storage; - // Mutex to protect execute_curr_streaming_op + /* Mutex to protect storage */ gpr_mu mu; }; typedef struct stream_obj stream_obj; -/* Globals */ -cronet_bidirectional_stream_header_array header_array; - -// static enum OP_RESULT execute_stream_op(struct op_and_state *oas); -/************************************************************* - Op Storage +/* + Add a new stream op to op storage. */ - static void add_to_storage(struct stream_obj *s, grpc_transport_stream_op *op) { + gpr_mu_lock(&s->mu); struct op_storage *storage = &s->storage; GPR_ASSERT(storage->num_pending_ops < MAX_PENDING_OPS); storage->num_pending_ops++; CRONET_LOG(GPR_DEBUG, "adding new op @wrptr=%d. %d in the queue.", - storage->wrptr, storage->num_pending_ops); - memcpy(&storage->pending_ops[storage->wrptr].op, op, sizeof(grpc_transport_stream_op)); - memset(&storage->pending_ops[storage->wrptr].state, 0, sizeof(storage->pending_ops[storage->wrptr].state)); + storage->wrptr, storage->num_pending_ops); + memcpy(&storage->pending_ops[storage->wrptr].op, op, + sizeof(grpc_transport_stream_op)); + memset(&storage->pending_ops[storage->wrptr].state, 0, + sizeof(storage->pending_ops[storage->wrptr].state)); storage->pending_ops[storage->wrptr].done = false; storage->pending_ops[storage->wrptr].s = s; - storage->wrptr = (storage->wrptr + 1) % MAX_PENDING_OPS; + storage->wrptr = (storage->wrptr + 1) % MAX_PENDING_OPS; + gpr_mu_unlock(&s->mu); } +/* + Cycle through ops and try to take next action. Break when either + an action with callback is taken, or no action is possible. + This can be executed from the Cronet network thread via cronet callback + or on the application supplied thread via the perform_stream_op function. +*/ static void execute_from_storage(stream_obj *s) { - // Cycle through ops and try to take next action. Break when either - // an action with callback is taken, or no action is possible. - // This can be executed from the Cronet network thread via cronet callback - // or on the application supplied thread via the perform_stream_op function. - if (1) {//gpr_mu_lock(&s->mu) == 0) { - gpr_mu_lock(&s->mu); - for (int i = 0; i < s->storage.wrptr; ) { - CRONET_LOG(GPR_DEBUG, "calling execute_stream_op[%d]. done = %d", i, s->storage.pending_ops[i].done); - if (s->storage.pending_ops[i].done) { - i++; - continue; - } - enum OP_RESULT result = execute_stream_op(&s->storage.pending_ops[i]); - CRONET_LOG(GPR_DEBUG, "%s = execute_stream_op[%d]", OP_RESULT_STRING[result], i); - if (result == NO_ACTION_POSSIBLE) { - i++; - } else if (result == ACTION_TAKEN_WITH_CALLBACK) { - break; - } + gpr_mu_lock(&s->mu); + for (int i = 0; i < s->storage.wrptr;) { + CRONET_LOG(GPR_DEBUG, "calling execute_stream_op[%d]. done = %d", i, + s->storage.pending_ops[i].done); + if (s->storage.pending_ops[i].done) { + i++; + continue; + } + enum OP_RESULT result = execute_stream_op(&s->storage.pending_ops[i]); + CRONET_LOG(GPR_DEBUG, "%s = execute_stream_op[%d]", + op_result_string[result], i); + if (result == NO_ACTION_POSSIBLE) { + i++; + } else if (result == ACTION_TAKEN_WITH_CALLBACK) { + break; } - gpr_mu_unlock(&s->mu); } + gpr_mu_unlock(&s->mu); grpc_exec_ctx_finish(&s->exec_ctx); } - -/************************************************************* -Cronet Callback Ipmlementation +/* + Cronet callback */ static void on_failed(cronet_bidirectional_stream *stream, int net_error) { CRONET_LOG(GPR_DEBUG, "on_failed(%p, %d)", stream, net_error); @@ -267,6 +271,9 @@ static void on_failed(cronet_bidirectional_stream *stream, int net_error) { execute_from_storage(s); } +/* + Cronet callback +*/ static void on_canceled(cronet_bidirectional_stream *stream) { CRONET_LOG(GPR_DEBUG, "on_canceled(%p)", stream); stream_obj *s = (stream_obj *)stream->annotation; @@ -276,6 +283,9 @@ static void on_canceled(cronet_bidirectional_stream *stream) { execute_from_storage(s); } +/* + Cronet callback +*/ static void on_succeeded(cronet_bidirectional_stream *stream) { CRONET_LOG(GPR_DEBUG, "on_succeeded(%p)", stream); stream_obj *s = (stream_obj *)stream->annotation; @@ -285,22 +295,33 @@ static void on_succeeded(cronet_bidirectional_stream *stream) { execute_from_storage(s); } +/* + Cronet callback +*/ static void on_request_headers_sent(cronet_bidirectional_stream *stream) { CRONET_LOG(GPR_DEBUG, "W: on_request_headers_sent(%p)", stream); stream_obj *s = (stream_obj *)stream->annotation; s->state.state_op_done[OP_SEND_INITIAL_METADATA] = true; s->state.state_callback_received[OP_SEND_INITIAL_METADATA] = true; + /* Free the memory allocated for headers */ + if (s->header_array.headers) { + gpr_free(s->header_array.headers); + } execute_from_storage(s); } +/* + Cronet callback +*/ static void on_response_headers_received( cronet_bidirectional_stream *stream, const cronet_bidirectional_stream_header_array *headers, const char *negotiated_protocol) { CRONET_LOG(GPR_DEBUG, "R: on_response_headers_received(%p, %p, %s)", stream, - headers, negotiated_protocol); + headers, negotiated_protocol); stream_obj *s = (stream_obj *)stream->annotation; - memset(&s->state.rs.initial_metadata, 0, sizeof(s->state.rs.initial_metadata)); + memset(&s->state.rs.initial_metadata, 0, + sizeof(s->state.rs.initial_metadata)); grpc_chttp2_incoming_metadata_buffer_init(&s->state.rs.initial_metadata); unsigned int i = 0; for (i = 0; i < headers->count; i++) { @@ -314,6 +335,9 @@ static void on_response_headers_received( execute_from_storage(s); } +/* + Cronet callback +*/ static void on_write_completed(cronet_bidirectional_stream *stream, const char *data) { stream_obj *s = (stream_obj *)stream->annotation; @@ -326,17 +350,23 @@ static void on_write_completed(cronet_bidirectional_stream *stream, execute_from_storage(s); } +/* + Cronet callback +*/ static void on_read_completed(cronet_bidirectional_stream *stream, char *data, int count) { stream_obj *s = (stream_obj *)stream->annotation; - CRONET_LOG(GPR_DEBUG, "R: on_read_completed(%p, %p, %d)", stream, data, count); + CRONET_LOG(GPR_DEBUG, "R: on_read_completed(%p, %p, %d)", stream, data, + count); if (count > 0) { s->state.rs.received_bytes += count; s->state.rs.remaining_bytes -= count; if (s->state.rs.remaining_bytes > 0) { - //CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); - s->state.state_op_done[OP_READ_REQ_MADE] = true; // If at least one read request has been made - cronet_bidirectional_stream_read(s->cbs, s->state.rs.read_buffer + s->state.rs.received_bytes, s->state.rs.remaining_bytes); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); + s->state.state_op_done[OP_READ_REQ_MADE] = true; + cronet_bidirectional_stream_read( + s->cbs, s->state.rs.read_buffer + s->state.rs.received_bytes, + s->state.rs.remaining_bytes); } else { execute_from_storage(s); } @@ -344,76 +374,82 @@ static void on_read_completed(cronet_bidirectional_stream *stream, char *data, } } +/* + Cronet callback +*/ static void on_response_trailers_received( cronet_bidirectional_stream *stream, const cronet_bidirectional_stream_header_array *trailers) { CRONET_LOG(GPR_DEBUG, "R: on_response_trailers_received(%p,%p)", stream, - trailers); + trailers); stream_obj *s = (stream_obj *)stream->annotation; - memset(&s->state.rs.trailing_metadata, 0, sizeof(s->state.rs.trailing_metadata)); + memset(&s->state.rs.trailing_metadata, 0, + sizeof(s->state.rs.trailing_metadata)); s->state.rs.trailing_metadata_valid = false; grpc_chttp2_incoming_metadata_buffer_init(&s->state.rs.trailing_metadata); unsigned int i = 0; for (i = 0; i < trailers->count; i++) { CRONET_LOG(GPR_DEBUG, "trailer key=%s, value=%s", trailers->headers[i].key, - trailers->headers[i].value); + trailers->headers[i].value); grpc_chttp2_incoming_metadata_buffer_add( - &s->state.rs.trailing_metadata, grpc_mdelem_from_metadata_strings( - grpc_mdstr_from_string(trailers->headers[i].key), - grpc_mdstr_from_string(trailers->headers[i].value))); + &s->state.rs.trailing_metadata, + grpc_mdelem_from_metadata_strings( + grpc_mdstr_from_string(trailers->headers[i].key), + grpc_mdstr_from_string(trailers->headers[i].value))); s->state.rs.trailing_metadata_valid = true; } s->state.state_callback_received[OP_RECV_TRAILING_METADATA] = true; execute_from_storage(s); } -/************************************************************* -Utility functions. Can be in their own file +/* + Utility function that takes the data from s->write_slice_buffer and assembles + into a contiguous byte stream with 5 byte gRPC header prepended. */ -// This function takes the data from s->write_slice_buffer and assembles into -// a contiguous byte stream with 5 byte gRPC header prepended. static void create_grpc_frame(gpr_slice_buffer *write_slice_buffer, - char **pp_write_buffer, int *p_write_buffer_size) { + char **pp_write_buffer, + int *p_write_buffer_size) { gpr_slice slice = gpr_slice_buffer_take_first(write_slice_buffer); size_t length = GPR_SLICE_LENGTH(slice); - // TODO (makdharma): FREE THIS!! HACK! *p_write_buffer_size = (int)length + GRPC_HEADER_SIZE_IN_BYTES; + /* This is freed in the on_write_completed callback */ char *write_buffer = gpr_malloc(length + GRPC_HEADER_SIZE_IN_BYTES); *pp_write_buffer = write_buffer; uint8_t *p = (uint8_t *)write_buffer; - // Append 5 byte header + /* Append 5 byte header */ *p++ = 0; *p++ = (uint8_t)(length >> 24); *p++ = (uint8_t)(length >> 16); *p++ = (uint8_t)(length >> 8); *p++ = (uint8_t)(length); - // append actual data + /* append actual data */ memcpy(p, GPR_SLICE_START_PTR(slice), length); } +/* + Convert metadata in a format that Cronet can consume +*/ static void convert_metadata_to_cronet_headers( - grpc_linked_mdelem *head, - const char *host, - char **pp_url, - cronet_bidirectional_stream_header **pp_headers, - size_t *p_num_headers) { + grpc_linked_mdelem *head, const char *host, char **pp_url, + cronet_bidirectional_stream_header **pp_headers, size_t *p_num_headers) { grpc_linked_mdelem *curr = head; - // Walk the linked list and get number of header fields + /* Walk the linked list and get number of header fields */ uint32_t num_headers_available = 0; while (curr != NULL) { curr = curr->next; num_headers_available++; } - // Allocate enough memory. TODO (makdharma): FREE MEMORY! HACK HACK - cronet_bidirectional_stream_header *headers = - (cronet_bidirectional_stream_header *)gpr_malloc( - sizeof(cronet_bidirectional_stream_header) * num_headers_available); + /* Allocate enough memory. It is freed in the on_request_headers_sent callback + */ + cronet_bidirectional_stream_header *headers = + (cronet_bidirectional_stream_header *)gpr_malloc( + sizeof(cronet_bidirectional_stream_header) * num_headers_available); *pp_headers = headers; - // Walk the linked list again, this time copying the header fields. - // s->num_headers - // can be less than num_headers_available, as some headers are not used for - // cronet + /* Walk the linked list again, this time copying the header fields. + s->num_headers can be less than num_headers_available, as some headers + are not used for cronet + */ curr = head; int num_headers = 0; while (num_headers < num_headers_available) { @@ -423,15 +459,15 @@ static void convert_metadata_to_cronet_headers( const char *value = grpc_mdstr_as_c_string(mdelem->value); if (strcmp(key, ":scheme") == 0 || strcmp(key, ":method") == 0 || strcmp(key, ":authority") == 0) { - // Cronet populates these fields on its own. + /* Cronet populates these fields on its own */ continue; } if (strcmp(key, ":path") == 0) { - // Create URL by appending :path value to the hostname + /* Create URL by appending :path value to the hostname */ gpr_asprintf(pp_url, "https://%s%s", host, value); continue; } - gpr_log(GPR_DEBUG, "header %s = %s", key, value); + CRONET_LOG(GPR_DEBUG, "header %s = %s", key, value); headers[num_headers].key = key; headers[num_headers].value = value; num_headers++; @@ -453,261 +489,342 @@ static int parse_grpc_header(const uint8_t *data) { } /* -Op Execution + Op Execution: Decide if one of the actions contained in the stream op can be + executed. This is the heart of the state machine. */ -static bool op_can_be_run(grpc_transport_stream_op *curr_op, struct op_state *stream_state, struct op_state *op_state, enum OP_ID op_id) { - // We use op's own state for most state, except for metadata related callbacks, which - // are at the stream level. TODO: WTF does this comment mean? +static bool op_can_be_run(grpc_transport_stream_op *curr_op, + struct op_state *stream_state, + struct op_state *op_state, enum OP_ID op_id) { bool result = true; - // When call is canceled, every op can be run - if (stream_state->state_op_done[OP_CANCEL_ERROR] || stream_state->state_callback_received[OP_FAILED]) { + /* When call is canceled, every op can be run, except under following + conditions + */ + if (stream_state->state_op_done[OP_CANCEL_ERROR] || + stream_state->state_callback_received[OP_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; if (op_id == OP_CANCEL_ERROR) result = false; - // already executed - if (op_id == OP_RECV_INITIAL_METADATA && stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) result = false; - if (op_id == OP_RECV_MESSAGE && stream_state->state_op_done[OP_RECV_MESSAGE]) result = false; - if (op_id == OP_RECV_TRAILING_METADATA && stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) result = false; + /* already executed */ + if (op_id == OP_RECV_INITIAL_METADATA && + stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) + result = false; + if (op_id == OP_RECV_MESSAGE && + stream_state->state_op_done[OP_RECV_MESSAGE]) + result = false; + if (op_id == OP_RECV_TRAILING_METADATA && + stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) + result = false; } else if (op_id == OP_SEND_INITIAL_METADATA) { - // already executed + /* already executed */ if (stream_state->state_op_done[OP_SEND_INITIAL_METADATA]) result = false; } else if (op_id == OP_RECV_INITIAL_METADATA) { - // already executed + /* already executed */ if (stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) result = false; - // we haven't sent headers yet. - else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; - // we haven't received headers yet. - else if (!stream_state->state_callback_received[OP_RECV_INITIAL_METADATA]) result = false; + /* we haven't sent headers yet. */ + else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) + result = false; + /* we haven't received headers yet. */ + else if (!stream_state->state_callback_received[OP_RECV_INITIAL_METADATA]) + result = false; } else if (op_id == OP_SEND_MESSAGE) { - // already executed (note we're checking op specific state, not stream state) + /* already executed (note we're checking op specific state, not stream + state) */ if (op_state->state_op_done[OP_SEND_MESSAGE]) result = false; - // we haven't sent headers yet. - else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; + /* we haven't sent headers yet. */ + else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) + result = false; } else if (op_id == OP_RECV_MESSAGE) { - // already executed + /* already executed */ if (op_state->state_op_done[OP_RECV_MESSAGE]) result = false; - // we haven't received headers yet. - else if (!stream_state->state_callback_received[OP_RECV_INITIAL_METADATA]) result = false; + /* we haven't received headers yet. */ + else if (!stream_state->state_callback_received[OP_RECV_INITIAL_METADATA]) + result = false; } else if (op_id == OP_RECV_TRAILING_METADATA) { - // already executed + /* already executed */ if (stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) result = false; - // we have asked for but haven't received message yet. - else if (stream_state->state_op_done[OP_READ_REQ_MADE] && !stream_state->state_op_done[OP_RECV_MESSAGE]) result = false; - // we haven't received trailers yet. - else if (!stream_state->state_callback_received[OP_RECV_TRAILING_METADATA]) result = false; + /* we have asked for but haven't received message yet. */ + else if (stream_state->state_op_done[OP_READ_REQ_MADE] && + !stream_state->state_op_done[OP_RECV_MESSAGE]) + result = false; + /* we haven't received trailers yet. */ + else if (!stream_state->state_callback_received[OP_RECV_TRAILING_METADATA]) + result = false; } else if (op_id == OP_SEND_TRAILING_METADATA) { - // already executed + /* already executed */ if (stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) result = false; - // we haven't sent initial metadata yet - else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; - // we haven't sent message yet - // TODO: Streaming Write case is a problem. What if there is an outstanding write (2nd, 3rd,..) present. - else if (curr_op->send_message && !stream_state->state_op_done[OP_SEND_MESSAGE]) result = false; - // we haven't got on_write_completed for the send yet - else if (stream_state->state_op_done[OP_SEND_MESSAGE] && !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; + /* we haven't sent initial metadata yet */ + else if (!stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) + result = false; + /* we haven't sent message yet */ + else if (curr_op->send_message && + !stream_state->state_op_done[OP_SEND_MESSAGE]) + result = false; + /* we haven't got on_write_completed for the send yet */ + else if (stream_state->state_op_done[OP_SEND_MESSAGE] && + !stream_state->state_callback_received[OP_SEND_MESSAGE]) + result = false; } else if (op_id == OP_CANCEL_ERROR) { - // already executed + /* already executed */ if (stream_state->state_op_done[OP_CANCEL_ERROR]) result = false; } else if (op_id == OP_ON_COMPLETE) { - // already executed (note we're checking op specific state, not stream state) + /* already executed (note we're checking op specific state, not stream + state) */ if (op_state->state_op_done[OP_ON_COMPLETE]) result = false; - // Check if every op that was asked for is done. - else if (curr_op->send_initial_metadata && !stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) result = false; - else if (curr_op->send_message && !stream_state->state_op_done[OP_SEND_MESSAGE]) result = false; - else if (curr_op->send_message && !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; - else if (curr_op->send_trailing_metadata && !stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) result = false; - else if (curr_op->recv_initial_metadata && !stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) result = false; - else if (curr_op->recv_message && !stream_state->state_op_done[OP_RECV_MESSAGE]) result = false; + /* Check if every op that was asked for is done. */ + else if (curr_op->send_initial_metadata && + !stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) + result = false; + else if (curr_op->send_message && + !stream_state->state_op_done[OP_SEND_MESSAGE]) + result = false; + else if (curr_op->send_message && + !stream_state->state_callback_received[OP_SEND_MESSAGE]) + result = false; + else if (curr_op->send_trailing_metadata && + !stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) + result = false; + else if (curr_op->recv_initial_metadata && + !stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) + result = false; + else if (curr_op->recv_message && + !stream_state->state_op_done[OP_RECV_MESSAGE]) + result = false; else if (curr_op->recv_trailing_metadata) { - //if (!stream_state->state_op_done[OP_SUCCEEDED]) result = false; gpr_log(GPR_DEBUG, "HACK!!"); - // We aren't done with trailing metadata yet - if (!stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) result = false; - // We've asked for actual message in an earlier op, and it hasn't been delivered yet. - // (TODO: What happens when multiple messages are asked for? How do you know when last message arrived?) + /* We aren't done with trailing metadata yet */ + if (!stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) + result = false; + /* We've asked for actual message in an earlier op, and it hasn't been + delivered yet. */ else if (stream_state->state_op_done[OP_READ_REQ_MADE]) { - // If this op is not the one asking for read, (which means some earlier op has asked), and the - // read hasn't been delivered. - if(!curr_op->recv_message && !stream_state->state_op_done[OP_SUCCEEDED]) result = false; + /* If this op is not the one asking for read, (which means some earlier + op has asked), and the read hasn't been delivered. */ + if (!curr_op->recv_message && + !stream_state->state_op_done[OP_SUCCEEDED]) + result = false; } } - // We should see at least one on_write_completed for the trailers that we sent - else if (curr_op->send_trailing_metadata && !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; + /* We should see at least one on_write_completed for the trailers that we + sent */ + else if (curr_op->send_trailing_metadata && + !stream_state->state_callback_received[OP_SEND_MESSAGE]) + result = false; } - CRONET_LOG(GPR_DEBUG, "op_can_be_run %s : %s", op_id_string[op_id], result? "YES":"NO"); + CRONET_LOG(GPR_DEBUG, "op_can_be_run %s : %s", op_id_string[op_id], + result ? "YES" : "NO"); return result; } static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { - // TODO TODO : This can be called from network thread and main thread. add a mutex. grpc_transport_stream_op *stream_op = &oas->op; struct stream_obj *s = oas->s; struct op_state *stream_state = &s->state; - //CRONET_LOG(GPR_DEBUG, "execute_stream_op"); enum OP_RESULT result = NO_ACTION_POSSIBLE; - if (stream_op->send_initial_metadata && op_can_be_run(stream_op, stream_state, &oas->state, OP_SEND_INITIAL_METADATA)) { + if (stream_op->send_initial_metadata && + 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 + /* This OP is the beginning. Reset various states */ 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 + 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); CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_create"); - s->cbs = cronet_bidirectional_stream_create(s->curr_ct.engine, s->curr_gs, &cronet_callbacks); + s->cbs = cronet_bidirectional_stream_create(s->curr_ct.engine, s->curr_gs, + &cronet_callbacks); char *url; - convert_metadata_to_cronet_headers(stream_op->send_initial_metadata->list.head, - s->curr_ct.host, &url, &header_array.headers, &header_array.count); - header_array.capacity = header_array.count; + convert_metadata_to_cronet_headers( + stream_op->send_initial_metadata->list.head, s->curr_ct.host, &url, + &s->header_array.headers, &s->header_array.count); + s->header_array.capacity = s->header_array.count; CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_start %s", url); - cronet_bidirectional_stream_start(s->cbs, url, 0, "POST", &header_array, false); + cronet_bidirectional_stream_start(s->cbs, url, 0, "POST", &s->header_array, + false); stream_state->state_op_done[OP_SEND_INITIAL_METADATA] = true; result = ACTION_TAKEN_WITH_CALLBACK; } else if (stream_op->recv_initial_metadata && - op_can_be_run(stream_op, stream_state, &oas->state, OP_RECV_INITIAL_METADATA)) { + 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]) { - grpc_chttp2_incoming_metadata_buffer_publish(&oas->s->state.rs.initial_metadata, - stream_op->recv_initial_metadata); - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_initial_metadata_ready, GRPC_ERROR_NONE, NULL); + grpc_chttp2_incoming_metadata_buffer_publish( + &oas->s->state.rs.initial_metadata, stream_op->recv_initial_metadata); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_initial_metadata_ready, + GRPC_ERROR_NONE, NULL); } else { - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_initial_metadata_ready, GRPC_ERROR_CANCELLED, NULL); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_initial_metadata_ready, + GRPC_ERROR_CANCELLED, NULL); } stream_state->state_op_done[OP_RECV_INITIAL_METADATA] = true; result = ACTION_TAKEN_NO_CALLBACK; - } else if (stream_op->send_message && op_can_be_run(stream_op, stream_state, &oas->state, OP_SEND_MESSAGE)) { + } else if (stream_op->send_message && + op_can_be_run(stream_op, stream_state, &oas->state, + OP_SEND_MESSAGE)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_SEND_MESSAGE", oas); - // TODO (makdharma): Make into a standalone function gpr_slice_buffer write_slice_buffer; gpr_slice slice; gpr_slice_buffer_init(&write_slice_buffer); grpc_byte_stream_next(NULL, stream_op->send_message, &slice, stream_op->send_message->length, NULL); - // Check that compression flag is not ON. We don't support compression yet. - // TODO (makdharma): add compression support + /* Check that compression flag is OFF. We don't support compression yet. */ GPR_ASSERT(stream_op->send_message->flags == 0); gpr_slice_buffer_add(&write_slice_buffer, slice); - GPR_ASSERT(write_slice_buffer.count == 1); // Empty request not handled yet + GPR_ASSERT(write_slice_buffer.count == + 1); /* Empty request not handled yet */ if (write_slice_buffer.count > 0) { int write_buffer_size; - create_grpc_frame(&write_slice_buffer, &stream_state->ws.write_buffer, &write_buffer_size); - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p)", stream_state->ws.write_buffer); + create_grpc_frame(&write_slice_buffer, &stream_state->ws.write_buffer, + &write_buffer_size); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p)", + stream_state->ws.write_buffer); stream_state->state_callback_received[OP_SEND_MESSAGE] = false; cronet_bidirectional_stream_write(s->cbs, stream_state->ws.write_buffer, - write_buffer_size, false); // TODO: What if this is not the last write? + write_buffer_size, false); result = ACTION_TAKEN_WITH_CALLBACK; } stream_state->state_op_done[OP_SEND_MESSAGE] = true; oas->state.state_op_done[OP_SEND_MESSAGE] = true; - } else if (stream_op->recv_message && op_can_be_run(stream_op, stream_state, &oas->state, OP_RECV_MESSAGE)) { + } else if (stream_op->recv_message && + 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]) { - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_CANCELLED, NULL); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, + GRPC_ERROR_CANCELLED, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; } 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) { - // Start a read operation for data + if (stream_state->rs.received_bytes == GRPC_HEADER_SIZE_IN_BYTES && + stream_state->rs.remaining_bytes == 0) { + /* Start a read operation for data */ stream_state->rs.length_field_received = true; stream_state->rs.length_field = stream_state->rs.remaining_bytes = - parse_grpc_header((const uint8_t *)stream_state->rs.read_buffer); - CRONET_LOG(GPR_DEBUG, "length field = %d", stream_state->rs.length_field); + parse_grpc_header((const uint8_t *)stream_state->rs.read_buffer); + CRONET_LOG(GPR_DEBUG, "length field = %d", + stream_state->rs.length_field); if (stream_state->rs.length_field > 0) { - stream_state->rs.read_buffer = gpr_malloc(stream_state->rs.length_field); + stream_state->rs.read_buffer = + gpr_malloc(stream_state->rs.length_field); GPR_ASSERT(stream_state->rs.read_buffer); stream_state->rs.remaining_bytes = stream_state->rs.length_field; stream_state->rs.received_bytes = 0; CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); - stream_state->state_op_done[OP_READ_REQ_MADE] = true; // If at least one read request has been made + stream_state->state_op_done[OP_READ_REQ_MADE] = + true; /* Indicates that at least one read request has been made */ cronet_bidirectional_stream_read(s->cbs, stream_state->rs.read_buffer, - stream_state->rs.remaining_bytes); + stream_state->rs.remaining_bytes); result = ACTION_TAKEN_WITH_CALLBACK; } else { stream_state->rs.remaining_bytes = 0; CRONET_LOG(GPR_DEBUG, "read operation complete. Empty response."); gpr_slice_buffer_init(&stream_state->rs.read_slice_buffer); - grpc_slice_buffer_stream_init(&stream_state->rs.sbs, &stream_state->rs.read_slice_buffer, 0); - *((grpc_byte_buffer **)stream_op->recv_message) = (grpc_byte_buffer *)&stream_state->rs.sbs; - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_NONE, NULL); + grpc_slice_buffer_stream_init(&stream_state->rs.sbs, + &stream_state->rs.read_slice_buffer, 0); + *((grpc_byte_buffer **)stream_op->recv_message) = + (grpc_byte_buffer *)&stream_state->rs.sbs; + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, + GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; - oas->state.state_op_done[OP_RECV_MESSAGE] = true; // Also set per op state. + oas->state.state_op_done[OP_RECV_MESSAGE] = true; result = ACTION_TAKEN_NO_CALLBACK; } } else if (stream_state->rs.remaining_bytes == 0) { - // Start a read operation for first 5 bytes (GRPC header) + /* Start a read operation for first 5 bytes (GRPC header) */ stream_state->rs.read_buffer = stream_state->rs.grpc_header_bytes; stream_state->rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; stream_state->rs.received_bytes = 0; CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); - stream_state->state_op_done[OP_READ_REQ_MADE] = true; // If at least one read request has been made + stream_state->state_op_done[OP_READ_REQ_MADE] = + true; /* Indicates that at least one read request has been made */ cronet_bidirectional_stream_read(s->cbs, stream_state->rs.read_buffer, stream_state->rs.remaining_bytes); } result = ACTION_TAKEN_WITH_CALLBACK; } else if (stream_state->rs.remaining_bytes == 0) { CRONET_LOG(GPR_DEBUG, "read operation complete"); - gpr_slice read_data_slice = gpr_slice_malloc((uint32_t)stream_state->rs.length_field); + gpr_slice read_data_slice = + gpr_slice_malloc((uint32_t)stream_state->rs.length_field); uint8_t *dst_p = GPR_SLICE_START_PTR(read_data_slice); - memcpy(dst_p, stream_state->rs.read_buffer, (size_t)stream_state->rs.length_field); + memcpy(dst_p, stream_state->rs.read_buffer, + (size_t)stream_state->rs.length_field); gpr_slice_buffer_init(&stream_state->rs.read_slice_buffer); - gpr_slice_buffer_add(&stream_state->rs.read_slice_buffer, read_data_slice); - grpc_slice_buffer_stream_init(&stream_state->rs.sbs, &stream_state->rs.read_slice_buffer, 0); - *((grpc_byte_buffer **)stream_op->recv_message) = (grpc_byte_buffer *)&stream_state->rs.sbs; - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_NONE, NULL); + gpr_slice_buffer_add(&stream_state->rs.read_slice_buffer, + read_data_slice); + grpc_slice_buffer_stream_init(&stream_state->rs.sbs, + &stream_state->rs.read_slice_buffer, 0); + *((grpc_byte_buffer **)stream_op->recv_message) = + (grpc_byte_buffer *)&stream_state->rs.sbs; + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, + GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; - oas->state.state_op_done[OP_RECV_MESSAGE] = true; // Also set per op state. - // 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; + 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; } } else if (stream_op->recv_trailing_metadata && - op_can_be_run(stream_op, stream_state, &oas->state, OP_RECV_TRAILING_METADATA)) { + op_can_be_run(stream_op, stream_state, &oas->state, + OP_RECV_TRAILING_METADATA)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_RECV_TRAILING_METADATA", oas); if (oas->s->state.rs.trailing_metadata_valid) { grpc_chttp2_incoming_metadata_buffer_publish( - &oas->s->state.rs.trailing_metadata, stream_op->recv_trailing_metadata); + &oas->s->state.rs.trailing_metadata, + stream_op->recv_trailing_metadata); stream_state->rs.trailing_metadata_valid = false; } stream_state->state_op_done[OP_RECV_TRAILING_METADATA] = true; result = ACTION_TAKEN_NO_CALLBACK; - } else if (stream_op->send_trailing_metadata && op_can_be_run(stream_op, stream_state, &oas->state, OP_SEND_TRAILING_METADATA)) { + } else if (stream_op->send_trailing_metadata && + op_can_be_run(stream_op, stream_state, &oas->state, + OP_SEND_TRAILING_METADATA)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_SEND_TRAILING_METADATA", oas); CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (0)"); stream_state->state_callback_received[OP_SEND_MESSAGE] = false; cronet_bidirectional_stream_write(s->cbs, "", 0, true); stream_state->state_op_done[OP_SEND_TRAILING_METADATA] = true; result = ACTION_TAKEN_WITH_CALLBACK; - } else if (stream_op->cancel_error && op_can_be_run(stream_op, stream_state, &oas->state, OP_CANCEL_ERROR)) { + } else if (stream_op->cancel_error && + op_can_be_run(stream_op, stream_state, &oas->state, + OP_CANCEL_ERROR)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_CANCEL_ERROR", oas); CRONET_LOG(GPR_DEBUG, "W: cronet_bidirectional_stream_cancel(%p)", s->cbs); - // Cancel might have come before creation of stream if (s->cbs) { cronet_bidirectional_stream_cancel(s->cbs); } stream_state->state_op_done[OP_CANCEL_ERROR] = true; result = ACTION_TAKEN_WITH_CALLBACK; - } else if (stream_op->on_complete && op_can_be_run(stream_op, stream_state, &oas->state, OP_ON_COMPLETE)) { - // All ops are complete. Call the on_complete callback + } else if (stream_op->on_complete && + op_can_be_run(stream_op, stream_state, &oas->state, + OP_ON_COMPLETE)) { + /* All actions in this stream_op are complete. Call the on_complete callback + */ CRONET_LOG(GPR_DEBUG, "running: %p OP_ON_COMPLETE", oas); - //CRONET_LOG(GPR_DEBUG, "calling on_complete"); - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->on_complete, GRPC_ERROR_NONE, NULL); - // Instead of setting stream state, use the op state as on_complete is on per op basis + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->on_complete, GRPC_ERROR_NONE, + NULL); oas->state.state_op_done[OP_ON_COMPLETE] = true; - oas->done = true; // Mark this op as completed - // reset any send or receive message state. + oas->done = true; + /* reset any send or receive message state. */ stream_state->state_callback_received[OP_SEND_MESSAGE] = false; stream_state->state_op_done[OP_SEND_MESSAGE] = false; result = ACTION_TAKEN_NO_CALLBACK; - // If this is the on_complete callback being called for a received message - make a note - if (stream_op->recv_message) stream_state->state_op_done[OP_RECV_MESSAGE_AND_ON_COMPLETE] = true; + /* If this is the on_complete callback being called for a received message - + make a note */ + if (stream_op->recv_message) + stream_state->state_op_done[OP_RECV_MESSAGE_AND_ON_COMPLETE] = true; } else { - //CRONET_LOG(GPR_DEBUG, "No op ready to run"); result = NO_ACTION_POSSIBLE; } return result; } -//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +/* + Functions used by upper layers to access transport functionality. +*/ static int init_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, grpc_stream *gs, grpc_stream_refcount *refcount, @@ -720,7 +837,8 @@ static int init_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, memset(&s->state.rs, 0, sizeof(s->state.rs)); memset(&s->state.ws, 0, sizeof(s->state.ws)); 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)); + memset(s->state.state_callback_received, 0, + sizeof(s->state.state_callback_received)); gpr_mu_init(&s->mu); s->exec_ctx = *exec_ctx; return 0; @@ -744,19 +862,16 @@ 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) {} -static void destroy_transport(grpc_exec_ctx *exec_ctx, grpc_transport *gt) { -} +static void destroy_transport(grpc_exec_ctx *exec_ctx, grpc_transport *gt) {} static char *get_peer(grpc_exec_ctx *exec_ctx, grpc_transport *gt) { return NULL; } static void perform_op(grpc_exec_ctx *exec_ctx, grpc_transport *gt, - grpc_transport_op *op) { -} + grpc_transport_op *op) {} const grpc_transport_vtable grpc_cronet_vtable = {sizeof(stream_obj), "cronet_http", From 280885eaa5e48912fe623dcbad8ad398d8eb405d Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Thu, 11 Aug 2016 14:16:36 -0700 Subject: [PATCH 09/14] WIP --- .../cronet/transport/cronet_transport.c | 177 +++++++++++++----- 1 file changed, 128 insertions(+), 49 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 1d756039809..8f11ef73792 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -50,16 +50,13 @@ #include "third_party/objective_c/Cronet/cronet_c_for_grpc.h" #define GRPC_HEADER_SIZE_IN_BYTES 5 -// maximum ops in a batch. There is not much thinking behind this limit, except -// that it seems to be enough for most use cases. -#define MAX_PENDING_OPS 100 #define CRONET_LOG(...) \ { \ if (grpc_cronet_trace) gpr_log(__VA_ARGS__); \ } -// TODO (makdharma): Hook up into the wider tracing mechanism +/* TODO (makdharma): Hook up into the wider tracing mechanism */ int grpc_cronet_trace = 1; enum OP_RESULT { @@ -68,7 +65,7 @@ enum OP_RESULT { NO_ACTION_POSSIBLE }; -// Used for printing debug +/* Used for printing debug */ const char *op_result_string[] = {"ACTION_TAKEN_WITH_CALLBACK", "ACTION_TAKEN_NO_CALLBACK", "NO_ACTION_POSSIBLE"}; @@ -129,7 +126,7 @@ static cronet_bidirectional_stream_callback cronet_callbacks = { on_failed, on_canceled}; -// Cronet transport object +/* Cronet transport object */ struct grpc_cronet_transport { grpc_transport base; /* must be first element in this structure */ cronet_engine *engine; @@ -146,6 +143,7 @@ struct read_state { int length_field; char grpc_header_bytes[GRPC_HEADER_SIZE_IN_BYTES]; char *payload_field; + bool read_stream_closed; /* vars for holding data destined for the application */ struct grpc_slice_buffer_stream sbs; @@ -177,14 +175,13 @@ struct op_and_state { grpc_transport_stream_op op; struct op_state state; bool done; - struct stream_obj *s; /* Pointer back to the stream object */ + struct stream_obj *s; /* Pointer back to the stream object */ + struct op_and_state *next; /* next op_and_state in the linked list */ }; struct op_storage { - struct op_and_state pending_ops[MAX_PENDING_OPS]; - int wrptr; - int rdptr; int num_pending_ops; + struct op_and_state *head; }; struct stream_obj { @@ -217,20 +214,52 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas); static void add_to_storage(struct stream_obj *s, grpc_transport_stream_op *op) { gpr_mu_lock(&s->mu); struct op_storage *storage = &s->storage; - GPR_ASSERT(storage->num_pending_ops < MAX_PENDING_OPS); + /* add new op at the beginning of the linked list. The memory is freed + in remove_from_storage */ + struct op_and_state *new_op = gpr_malloc(sizeof(struct op_and_state)); + memcpy(&new_op->op, op, sizeof(grpc_transport_stream_op)); + memset(&new_op->state, 0, sizeof(new_op->state)); + new_op->s = s; + new_op->done = false; + new_op->next = storage->head; + storage->head = new_op; storage->num_pending_ops++; - CRONET_LOG(GPR_DEBUG, "adding new op @wrptr=%d. %d in the queue.", - storage->wrptr, storage->num_pending_ops); - memcpy(&storage->pending_ops[storage->wrptr].op, op, - sizeof(grpc_transport_stream_op)); - memset(&storage->pending_ops[storage->wrptr].state, 0, - sizeof(storage->pending_ops[storage->wrptr].state)); - storage->pending_ops[storage->wrptr].done = false; - storage->pending_ops[storage->wrptr].s = s; - storage->wrptr = (storage->wrptr + 1) % MAX_PENDING_OPS; + CRONET_LOG(GPR_DEBUG, "adding new op %p. %d in the queue.", new_op, + storage->num_pending_ops); gpr_mu_unlock(&s->mu); } +/* + Traverse the linked list and delete op and free memory +*/ +static void remove_from_storage(struct stream_obj *s, + struct op_and_state *oas) { + struct op_and_state *curr; + if (s->storage.head == NULL || oas == NULL) { + return; + } + if (s->storage.head == oas) { + s->storage.head = oas->next; + gpr_free(oas); + s->storage.num_pending_ops--; + CRONET_LOG(GPR_DEBUG, "Freed %p. Now %d in the queue", oas, + s->storage.num_pending_ops); + } else { + for (curr = s->storage.head; curr != NULL; curr = curr->next) { + if (curr->next == oas) { + curr->next = oas->next; + s->storage.num_pending_ops--; + CRONET_LOG(GPR_DEBUG, "Freed %p. Now %d in the queue", oas, + s->storage.num_pending_ops); + gpr_free(oas); + break; + } else if (curr->next == NULL) { + CRONET_LOG(GPR_ERROR, "Reached end of LL and did not find op to free"); + } + } + } +} + /* Cycle through ops and try to take next action. Break when either an action with callback is taken, or no action is possible. @@ -239,18 +268,21 @@ static void add_to_storage(struct stream_obj *s, grpc_transport_stream_op *op) { */ static void execute_from_storage(stream_obj *s) { gpr_mu_lock(&s->mu); - for (int i = 0; i < s->storage.wrptr;) { - CRONET_LOG(GPR_DEBUG, "calling execute_stream_op[%d]. done = %d", i, - s->storage.pending_ops[i].done); - if (s->storage.pending_ops[i].done) { - i++; - continue; + for (struct op_and_state *curr = s->storage.head; curr != NULL;) { + CRONET_LOG(GPR_DEBUG, "calling op at %p. done = %d", curr, curr->done); + GPR_ASSERT(curr->done == 0); + enum OP_RESULT result = execute_stream_op(curr); + CRONET_LOG(GPR_DEBUG, "execute_stream_op[%p] returns %s", curr, + op_result_string[result]); + /* if this op is done, then remove it and free memory */ + if (curr->done) { + struct op_and_state *next = curr->next; + remove_from_storage(s, curr); + curr = next; } - enum OP_RESULT result = execute_stream_op(&s->storage.pending_ops[i]); - CRONET_LOG(GPR_DEBUG, "%s = execute_stream_op[%d]", - op_result_string[result], i); + /* continue processing the same op if ACTION_TAKEN_WITHOUT_CALLBACK */ if (result == NO_ACTION_POSSIBLE) { - i++; + curr = curr->next; } else if (result == ACTION_TAKEN_WITH_CALLBACK) { break; } @@ -268,6 +300,14 @@ static void on_failed(cronet_bidirectional_stream *stream, int net_error) { cronet_bidirectional_stream_destroy(s->cbs); s->state.state_callback_received[OP_FAILED] = true; s->cbs = NULL; + if (s->header_array.headers) { + gpr_free(s->header_array.headers); + s->header_array.headers = NULL; + } + if (s->state.ws.write_buffer) { + gpr_free(s->state.ws.write_buffer); + s->state.ws.write_buffer = NULL; + } execute_from_storage(s); } @@ -280,6 +320,14 @@ static void on_canceled(cronet_bidirectional_stream *stream) { cronet_bidirectional_stream_destroy(s->cbs); s->state.state_callback_received[OP_CANCELED] = true; s->cbs = NULL; + if (s->header_array.headers) { + gpr_free(s->header_array.headers); + s->header_array.headers = NULL; + } + if (s->state.ws.write_buffer) { + gpr_free(s->state.ws.write_buffer); + s->state.ws.write_buffer = NULL; + } execute_from_storage(s); } @@ -306,6 +354,7 @@ static void on_request_headers_sent(cronet_bidirectional_stream *stream) { /* Free the memory allocated for headers */ if (s->header_array.headers) { gpr_free(s->header_array.headers); + s->header_array.headers = NULL; } execute_from_storage(s); } @@ -358,6 +407,7 @@ static void on_read_completed(cronet_bidirectional_stream *stream, char *data, stream_obj *s = (stream_obj *)stream->annotation; CRONET_LOG(GPR_DEBUG, "R: on_read_completed(%p, %p, %d)", stream, data, count); + s->state.state_callback_received[OP_RECV_MESSAGE] = true; if (count > 0) { s->state.rs.received_bytes += count; s->state.rs.remaining_bytes -= count; @@ -370,7 +420,9 @@ static void on_read_completed(cronet_bidirectional_stream *stream, char *data, } else { execute_from_storage(s); } - s->state.state_callback_received[OP_RECV_MESSAGE] = true; + } else { + s->state.rs.read_stream_closed = true; + execute_from_storage(s); } } @@ -570,38 +622,51 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, } else if (op_id == OP_ON_COMPLETE) { /* already executed (note we're checking op specific state, not stream state) */ - if (op_state->state_op_done[OP_ON_COMPLETE]) result = false; + if (op_state->state_op_done[OP_ON_COMPLETE]) { + CRONET_LOG(GPR_DEBUG, "Because"); + result = false; + } /* Check if every op that was asked for is done. */ else if (curr_op->send_initial_metadata && - !stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) + !stream_state->state_callback_received[OP_SEND_INITIAL_METADATA]) { + CRONET_LOG(GPR_DEBUG, "Because"); result = false; - else if (curr_op->send_message && - !stream_state->state_op_done[OP_SEND_MESSAGE]) + } else if (curr_op->send_message && + !op_state->state_op_done[OP_SEND_MESSAGE]) { + CRONET_LOG(GPR_DEBUG, "Because"); result = false; - else if (curr_op->send_message && - !stream_state->state_callback_received[OP_SEND_MESSAGE]) + } else if (curr_op->send_message && + !stream_state->state_callback_received[OP_SEND_MESSAGE]) { + CRONET_LOG(GPR_DEBUG, "Because"); result = false; - else if (curr_op->send_trailing_metadata && - !stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) + } else if (curr_op->send_trailing_metadata && + !stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) { + CRONET_LOG(GPR_DEBUG, "Because"); result = false; - else if (curr_op->recv_initial_metadata && - !stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) + } else if (curr_op->recv_initial_metadata && + !stream_state->state_op_done[OP_RECV_INITIAL_METADATA]) { + CRONET_LOG(GPR_DEBUG, "Because"); result = false; - else if (curr_op->recv_message && - !stream_state->state_op_done[OP_RECV_MESSAGE]) + } else if (curr_op->recv_message && + !stream_state->state_op_done[OP_RECV_MESSAGE]) { + CRONET_LOG(GPR_DEBUG, "Because"); result = false; - else if (curr_op->recv_trailing_metadata) { + } else if (curr_op->recv_trailing_metadata) { /* We aren't done with trailing metadata yet */ - if (!stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) + if (!stream_state->state_op_done[OP_RECV_TRAILING_METADATA]) { + CRONET_LOG(GPR_DEBUG, "Because"); result = false; + } /* We've asked for actual message in an earlier op, and it hasn't been delivered yet. */ else if (stream_state->state_op_done[OP_READ_REQ_MADE]) { /* If this op is not the one asking for read, (which means some earlier op has asked), and the read hasn't been delivered. */ if (!curr_op->recv_message && - !stream_state->state_op_done[OP_SUCCEEDED]) + !stream_state->state_callback_received[OP_SUCCEEDED]) { + CRONET_LOG(GPR_DEBUG, "Because"); result = false; + } } } /* We should see at least one on_write_completed for the trailers that we @@ -625,6 +690,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { OP_SEND_INITIAL_METADATA)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_SEND_INITIAL_METADATA", oas); /* This OP is the beginning. Reset various states */ + 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)); @@ -637,6 +703,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { s->cbs = cronet_bidirectional_stream_create(s->curr_ct.engine, s->curr_gs, &cronet_callbacks); char *url; + s->header_array.headers = NULL; convert_metadata_to_cronet_headers( stream_op->send_initial_metadata->list.head, s->curr_ct.host, &url, &s->header_array.headers, &s->header_array.count); @@ -696,6 +763,13 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_CANCELLED, 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"); + grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, + GRPC_ERROR_NONE, NULL); + stream_state->state_op_done[OP_RECV_MESSAGE] = true; + oas->state.state_op_done[OP_RECV_MESSAGE] = true; } 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) { @@ -808,9 +882,12 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { NULL); oas->state.state_op_done[OP_ON_COMPLETE] = true; oas->done = true; - /* reset any send or receive message state. */ - stream_state->state_callback_received[OP_SEND_MESSAGE] = false; - stream_state->state_op_done[OP_SEND_MESSAGE] = false; + /* reset any send message state, only if this ON_COMPLETE is about a send. + */ + if (stream_op->send_message) { + stream_state->state_callback_received[OP_SEND_MESSAGE] = false; + stream_state->state_op_done[OP_SEND_MESSAGE] = false; + } result = ACTION_TAKEN_NO_CALLBACK; /* If this is the on_complete callback being called for a received message - make a note */ @@ -831,9 +908,11 @@ static int init_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, const void *server_data) { stream_obj *s = (stream_obj *)gs; memset(&s->storage, 0, sizeof(s->storage)); + s->storage.head = NULL; memset(&s->state, 0, sizeof(s->state)); s->curr_op = NULL; s->cbs = NULL; + memset(&s->header_array, 0, sizeof(s->header_array)); memset(&s->state.rs, 0, sizeof(s->state.rs)); memset(&s->state.ws, 0, sizeof(s->state.ws)); memset(s->state.state_op_done, 0, sizeof(s->state.state_op_done)); From 91d519532a79e7309e0d63a246c064398eec5288 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Thu, 11 Aug 2016 15:30:52 -0700 Subject: [PATCH 10/14] removed file from commit --- .../CoreCronetEnd2EndTests.m | 403 ------------------ 1 file changed, 403 deletions(-) delete mode 100644 src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m diff --git a/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m b/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m deleted file mode 100644 index d2181120e93..00000000000 --- a/src/objective-c/tests/CoreCronetEnd2EndTests/CoreCronetEnd2EndTests.m +++ /dev/null @@ -1,403 +0,0 @@ -/* - * - * 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. - * - */ - -/* - * This test file is derived from fixture h2_ssl.c in core end2end test - * (test/core/end2end/fixture/h2_ssl.c). The structure of the fixture is - * preserved as much as possible - * - * This fixture creates a server full stack using chttp2 and a client - * full stack using Cronet. End-to-end tests are run against this - * configuration - * - */ - - -#import -#include "test/core/end2end/end2end_tests.h" - -#include -#include - -#include -#include -#include - -#include "src/core/lib/channel/channel_args.h" -#include "src/core/lib/security/credentials/credentials.h" -#include "src/core/lib/support/env.h" -#include "src/core/lib/support/string.h" -#include "src/core/lib/support/tmpfile.h" -#include "test/core/end2end/data/ssl_test_data.h" -#include "test/core/util/port.h" -#include "test/core/util/test_config.h" - -#include -#import - -typedef struct fullstack_secure_fixture_data { - char *localaddr; -} fullstack_secure_fixture_data; - -static grpc_end2end_test_fixture chttp2_create_fixture_secure_fullstack( - grpc_channel_args *client_args, grpc_channel_args *server_args) { - grpc_end2end_test_fixture f; - int port = grpc_pick_unused_port_or_die(); - fullstack_secure_fixture_data *ffd = - gpr_malloc(sizeof(fullstack_secure_fixture_data)); - memset(&f, 0, sizeof(f)); - - gpr_join_host_port(&ffd->localaddr, "127.0.0.1", port); - - f.fixture_data = ffd; - f.cq = grpc_completion_queue_create(NULL); - - return f; -} - -static void process_auth_failure(void *state, grpc_auth_context *ctx, - const grpc_metadata *md, size_t md_count, - grpc_process_auth_metadata_done_cb cb, - void *user_data) { - GPR_ASSERT(state == NULL); - cb(user_data, NULL, 0, NULL, 0, GRPC_STATUS_UNAUTHENTICATED, NULL); -} - -static void cronet_init_client_secure_fullstack( - grpc_end2end_test_fixture *f, grpc_channel_args *client_args, - cronet_engine *cronetEngine) { - fullstack_secure_fixture_data *ffd = f->fixture_data; - f->client = - grpc_cronet_secure_channel_create(cronetEngine, ffd->localaddr, client_args, NULL); - GPR_ASSERT(f->client != NULL); -} - -static void chttp2_init_server_secure_fullstack( - grpc_end2end_test_fixture *f, grpc_channel_args *server_args, - grpc_server_credentials *server_creds) { - fullstack_secure_fixture_data *ffd = f->fixture_data; - if (f->server) { - grpc_server_destroy(f->server); - } - f->server = grpc_server_create(server_args, NULL); - grpc_server_register_completion_queue(f->server, f->cq, NULL); - GPR_ASSERT(grpc_server_add_secure_http2_port(f->server, ffd->localaddr, - server_creds)); - grpc_server_credentials_release(server_creds); - grpc_server_start(f->server); -} - -static void chttp2_tear_down_secure_fullstack(grpc_end2end_test_fixture *f) { - fullstack_secure_fixture_data *ffd = f->fixture_data; - gpr_free(ffd->localaddr); - gpr_free(ffd); -} - -static void cronet_init_client_simple_ssl_secure_fullstack( - grpc_end2end_test_fixture *f, grpc_channel_args *client_args) { - grpc_arg ssl_name_override = {GRPC_ARG_STRING, - GRPC_SSL_TARGET_NAME_OVERRIDE_ARG, - {"foo.test.google.fr"}}; - - grpc_channel_args *new_client_args = - grpc_channel_args_copy_and_add(client_args, &ssl_name_override, 1); - static bool done = false; - // TODO (makdharma): DO NOT CHECK IN THIS HACK!!! - if (!done) { - done = true; - [Cronet setHttp2Enabled:YES]; - NSURL *url = [[[NSFileManager defaultManager] - URLsForDirectory:NSDocumentDirectory inDomains:NSUserDomainMask] lastObject]; - NSLog(@"Documents directory: %@", url); - [Cronet start]; - [Cronet startNetLogToFile: @"Documents/cronet_netlog.json" logBytes:YES]; - } - cronet_engine *cronetEngine = [Cronet getGlobalEngine]; - - cronet_init_client_secure_fullstack(f, new_client_args, cronetEngine); - grpc_channel_args_destroy(new_client_args); -} - -static int fail_server_auth_check(grpc_channel_args *server_args) { - size_t i; - if (server_args == NULL) return 0; - for (i = 0; i < server_args->num_args; i++) { - if (strcmp(server_args->args[i].key, FAIL_AUTH_CHECK_SERVER_ARG_NAME) == - 0) { - return 1; - } - } - return 0; -} - -static void chttp2_init_server_simple_ssl_secure_fullstack( - grpc_end2end_test_fixture *f, grpc_channel_args *server_args) { - grpc_ssl_pem_key_cert_pair pem_cert_key_pair = {test_server1_key, - test_server1_cert}; - grpc_server_credentials *ssl_creds = - grpc_ssl_server_credentials_create(NULL, &pem_cert_key_pair, 1, 0, NULL); - if (fail_server_auth_check(server_args)) { - grpc_auth_metadata_processor processor = {process_auth_failure, NULL, NULL}; - grpc_server_credentials_set_auth_metadata_processor(ssl_creds, processor); - } - chttp2_init_server_secure_fullstack(f, server_args, ssl_creds); -} - -/* All test configurations */ - -static grpc_end2end_test_config configs[] = { - {"chttp2/simple_ssl_fullstack", - FEATURE_MASK_SUPPORTS_DELAYED_CONNECTION | - FEATURE_MASK_SUPPORTS_PER_CALL_CREDENTIALS, - chttp2_create_fixture_secure_fullstack, - cronet_init_client_simple_ssl_secure_fullstack, - chttp2_init_server_simple_ssl_secure_fullstack, - chttp2_tear_down_secure_fullstack}, -}; - - - -static char *roots_filename; - -@interface CoreCronetEnd2EndTests : XCTestCase - -@end - -@implementation CoreCronetEnd2EndTests - - -// The setUp() function is run before the test cases run and only run once -+ (void)setUp { - [super setUp]; - - FILE *roots_file; - size_t roots_size = strlen(test_root_cert); - - char *argv[] = {"CoreCronetEnd2EndTests"}; - grpc_test_init(1, argv); - grpc_end2end_tests_pre_init(); - - /* Set the SSL roots env var. */ - roots_file = gpr_tmpfile("chttp2_simple_ssl_fullstack_test", &roots_filename); - GPR_ASSERT(roots_filename != NULL); - GPR_ASSERT(roots_file != NULL); - GPR_ASSERT(fwrite(test_root_cert, 1, roots_size, roots_file) == roots_size); - fclose(roots_file); - gpr_setenv(GRPC_DEFAULT_SSL_ROOTS_FILE_PATH_ENV_VAR, roots_filename); - - grpc_init(); - -} - -// The tearDown() function is run after all test cases finish running -+ (void)tearDown { - grpc_shutdown(); - - /* Cleanup. */ - remove(roots_filename); - gpr_free(roots_filename); - - [super tearDown]; -} - -- (void)testIndividualCase:(char*)test_case { - char *argv[] = {"h2_ssl", test_case}; - - for (int i = 0; i < sizeof(configs) / sizeof(*configs); i++) { - grpc_end2end_tests(sizeof(argv) / sizeof(argv[0]), argv, configs[i]); - } -} - -// TODO(mxyan): Use NSStringFromSelector(_cmd) to acquire test name from the -// test case method name, so that bodies of test cases can stay identical -- (void)testBadHostname { - [self testIndividualCase:"bad_hostname"]; -} - -- (void)testBinaryMetadata { - //[self testIndividualCase:"binary_metadata"]; -} - -- (void)testCallCreds { - [self testIndividualCase:"call_creds"]; -} - -- (void)testCancelAfterAccept { - [self testIndividualCase:"cancel_after_accept"]; -} - -- (void)testCancelAfterClientDone { - [self testIndividualCase:"cancel_after_client_done"]; -} - -- (void)testCancelAfterInvoke { - [self testIndividualCase:"cancel_after_invoke"]; -} - -- (void)testCancelBeforeInvoke { - [self testIndividualCase:"cancel_before_invoke"]; -} - -- (void)testCancelInAVacuum { - [self testIndividualCase:"cancel_in_a_vacuum"]; -} - -- (void)testCancelWithStatus { - [self testIndividualCase:"cancel_with_status"]; -} - -- (void)testCompressedPayload { - [self testIndividualCase:"compressed_payload"]; -} - -- (void)testConnectivity { - [self testIndividualCase:"connectivity"]; -} - -- (void)testDefaultHost { - [self testIndividualCase:"default_host"]; -} - -- (void)testDisappearingServer { - [self testIndividualCase:"disappearing_server"]; -} - -- (void)testEmptyBatch { - [self testIndividualCase:"empty_batch"]; -} - -- (void)testFilterCausesClose { - [self testIndividualCase:"filter_causes_close"]; -} - -- (void)testGracefulServerShutdown { - [self testIndividualCase:"graceful_server_shutdown"]; -} - -- (void)testHighInitialSeqno { - [self testIndividualCase:"high_initial_seqno"]; -} - -- (void)testHpackSize { - [self testIndividualCase:"hpack_size"]; -} - -- (void)testIdempotentRequest { - [self testIndividualCase:"idempotent_request"]; -} - -- (void)testInvokeLargeRequest { - [self testIndividualCase:"invoke_large_request"]; -} - -- (void)testLargeMetadata { - [self testIndividualCase:"large_metadata"]; -} - -- (void)testMaxConcurrentStreams { - [self testIndividualCase:"max_concurrent_streams"]; -} - -- (void)testMaxMessageLength { - [self testIndividualCase:"max_message_length"]; -} - -- (void)testNegativeDeadline { - [self testIndividualCase:"negative_deadline"]; -} - -- (void)testNetworkStatusChange { - [self testIndividualCase:"network_status_change"]; -} - -- (void)testNoOp { - [self testIndividualCase:"no_op"]; -} - -- (void)testPayload { - [self testIndividualCase:"payload"]; -} - -- (void)testPing { - [self testIndividualCase:"ping"]; -} - -- (void)testPingPongStreaming { - [self testIndividualCase:"ping_pong_streaming"]; -} - -- (void)testRegisteredCall { - [self testIndividualCase:"registered_call"]; -} - -- (void)testRequestWithFlags { - [self testIndividualCase:"request_with_flags"]; -} - -- (void)testRequestWithPayload { - [self testIndividualCase:"request_with_payload"]; -} - -- (void)testServerFinishesRequest { - [self testIndividualCase:"server_finishes_request"]; -} - -- (void)testShutdownFinishesCalls { - [self testIndividualCase:"shutdown_finishes_calls"]; -} - -- (void)testShutdownFinishesTags { - [self testIndividualCase:"shutdown_finishes_tags"]; -} - -- (void)testSimpleDelayedRequest { - [self testIndividualCase:"simple_delayed_request"]; -} - -- (void)testSimpleMetadata { - [self testIndividualCase:"simple_metadata"]; -} - -- (void)testSimpleRequest { - [self testIndividualCase:"simple_request"]; -} - -- (void)testStreamingErrorResponse { - [self testIndividualCase:"streaming_error_response"]; -} - -- (void)testTrailingMetadata { - [self testIndividualCase:"trailing_metadata"]; -} - -@end From 3578c5e592e168b64c64d72d9c2af1dba7ec0812 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Mon, 15 Aug 2016 15:13:54 -0700 Subject: [PATCH 11/14] bug fix for fireball app modified condition for trailing metadata. added more information to log message. --- .../cronet/transport/cronet_transport.c | 20 +++++++++++-------- 1 file changed, 12 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 8f11ef73792..50f8df52434 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -412,7 +412,7 @@ static void on_read_completed(cronet_bidirectional_stream *stream, char *data, s->state.rs.received_bytes += count; s->state.rs.remaining_bytes -= count; if (s->state.rs.remaining_bytes > 0) { - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); s->state.state_op_done[OP_READ_REQ_MADE] = true; cronet_bidirectional_stream_read( s->cbs, s->state.rs.read_buffer + s->state.rs.received_bytes, @@ -602,6 +602,9 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, /* we haven't received trailers yet. */ else if (!stream_state->state_callback_received[OP_RECV_TRAILING_METADATA]) result = false; + /* we haven't received on_succeeded yet. */ + else if (!stream_state->state_callback_received[OP_SUCCEEDED]) + result = false; } else if (op_id == OP_SEND_TRAILING_METADATA) { /* already executed */ if (stream_state->state_op_done[OP_SEND_TRAILING_METADATA]) result = false; @@ -699,16 +702,17 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { /* Start new cronet stream. It is destroyed in on_succeeded, on_canceled, * on_failed */ GPR_ASSERT(s->cbs == NULL); - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_create"); 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); char *url; s->header_array.headers = NULL; convert_metadata_to_cronet_headers( stream_op->send_initial_metadata->list.head, s->curr_ct.host, &url, &s->header_array.headers, &s->header_array.count); s->header_array.capacity = s->header_array.count; - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_start %s", url); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_start(%p, %s)", s->cbs, + url); cronet_bidirectional_stream_start(s->cbs, url, 0, "POST", &s->header_array, false); stream_state->state_op_done[OP_SEND_INITIAL_METADATA] = true; @@ -746,8 +750,8 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { int write_buffer_size; create_grpc_frame(&write_slice_buffer, &stream_state->ws.write_buffer, &write_buffer_size); - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p)", - stream_state->ws.write_buffer); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p, %p)", + s->cbs, stream_state->ws.write_buffer); stream_state->state_callback_received[OP_SEND_MESSAGE] = false; cronet_bidirectional_stream_write(s->cbs, stream_state->ws.write_buffer, write_buffer_size, false); @@ -785,7 +789,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { GPR_ASSERT(stream_state->rs.read_buffer); stream_state->rs.remaining_bytes = stream_state->rs.length_field; stream_state->rs.received_bytes = 0; - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); stream_state->state_op_done[OP_READ_REQ_MADE] = true; /* Indicates that at least one read request has been made */ cronet_bidirectional_stream_read(s->cbs, stream_state->rs.read_buffer, @@ -810,7 +814,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { stream_state->rs.read_buffer = stream_state->rs.grpc_header_bytes; stream_state->rs.remaining_bytes = GRPC_HEADER_SIZE_IN_BYTES; stream_state->rs.received_bytes = 0; - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read"); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_read(%p)", s->cbs); stream_state->state_op_done[OP_READ_REQ_MADE] = true; /* Indicates that at least one read request has been made */ cronet_bidirectional_stream_read(s->cbs, stream_state->rs.read_buffer, @@ -857,7 +861,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { op_can_be_run(stream_op, stream_state, &oas->state, OP_SEND_TRAILING_METADATA)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_SEND_TRAILING_METADATA", oas); - CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (0)"); + CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p, 0)", s->cbs); stream_state->state_callback_received[OP_SEND_MESSAGE] = false; cronet_bidirectional_stream_write(s->cbs, "", 0, true); stream_state->state_op_done[OP_SEND_TRAILING_METADATA] = true; From 1b8deaa407e46a32559ad220c58b26f68e14c1dc Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Tue, 16 Aug 2016 16:29:08 -0700 Subject: [PATCH 12/14] addressed review feedback. --- .../cronet/transport/cronet_transport.c | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 50f8df52434..0fa79870760 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -52,12 +52,12 @@ #define GRPC_HEADER_SIZE_IN_BYTES 5 #define CRONET_LOG(...) \ - { \ + do { \ if (grpc_cronet_trace) gpr_log(__VA_ARGS__); \ - } + } while (0) /* TODO (makdharma): Hook up into the wider tracing mechanism */ -int grpc_cronet_trace = 1; +int grpc_cronet_trace = 0; enum OP_RESULT { ACTION_TAKEN_WITH_CALLBACK, @@ -192,8 +192,6 @@ struct stream_obj { cronet_bidirectional_stream *cbs; cronet_bidirectional_stream_header_array header_array; - /* Used for executing callbacks for ops */ - grpc_exec_ctx exec_ctx; /* Stream level state. Some state will be tracked both at stream and stream_op * level */ struct op_state state; @@ -206,7 +204,8 @@ struct stream_obj { }; typedef struct stream_obj stream_obj; -static enum OP_RESULT execute_stream_op(struct op_and_state *oas); +static enum OP_RESULT execute_stream_op(grpc_exec_ctx *exec_ctx, + struct op_and_state *oas); /* Add a new stream op to op storage. @@ -267,11 +266,12 @@ static void remove_from_storage(struct stream_obj *s, or on the application supplied thread via the perform_stream_op function. */ static void execute_from_storage(stream_obj *s) { + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; gpr_mu_lock(&s->mu); for (struct op_and_state *curr = s->storage.head; curr != NULL;) { CRONET_LOG(GPR_DEBUG, "calling op at %p. done = %d", curr, curr->done); GPR_ASSERT(curr->done == 0); - enum OP_RESULT result = execute_stream_op(curr); + enum OP_RESULT result = execute_stream_op(&exec_ctx, curr); CRONET_LOG(GPR_DEBUG, "execute_stream_op[%p] returns %s", curr, op_result_string[result]); /* if this op is done, then remove it and free memory */ @@ -288,7 +288,7 @@ static void execute_from_storage(stream_obj *s) { } } gpr_mu_unlock(&s->mu); - grpc_exec_ctx_finish(&s->exec_ctx); + grpc_exec_ctx_finish(&exec_ctx); } /* @@ -683,7 +683,8 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, return result; } -static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { +static enum OP_RESULT execute_stream_op(grpc_exec_ctx *exec_ctx, + struct op_and_state *oas) { grpc_transport_stream_op *stream_op = &oas->op; struct stream_obj *s = oas->s; struct op_state *stream_state = &s->state; @@ -724,10 +725,10 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { if (!stream_state->state_op_done[OP_CANCEL_ERROR]) { grpc_chttp2_incoming_metadata_buffer_publish( &oas->s->state.rs.initial_metadata, stream_op->recv_initial_metadata); - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_initial_metadata_ready, + grpc_exec_ctx_sched(exec_ctx, stream_op->recv_initial_metadata_ready, GRPC_ERROR_NONE, NULL); } else { - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_initial_metadata_ready, + grpc_exec_ctx_sched(exec_ctx, stream_op->recv_initial_metadata_ready, GRPC_ERROR_CANCELLED, NULL); } stream_state->state_op_done[OP_RECV_INITIAL_METADATA] = true; @@ -764,13 +765,13 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { OP_RECV_MESSAGE)) { CRONET_LOG(GPR_DEBUG, "running: %p OP_RECV_MESSAGE", oas); if (stream_state->state_op_done[OP_CANCEL_ERROR]) { - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, + 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->rs.read_stream_closed == true) { /* No more data will be received */ CRONET_LOG(GPR_DEBUG, "read stream closed"); - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, + grpc_exec_ctx_sched(exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; oas->state.state_op_done[OP_RECV_MESSAGE] = true; @@ -803,7 +804,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { &stream_state->rs.read_slice_buffer, 0); *((grpc_byte_buffer **)stream_op->recv_message) = (grpc_byte_buffer *)&stream_state->rs.sbs; - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, + grpc_exec_ctx_sched(exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; oas->state.state_op_done[OP_RECV_MESSAGE] = true; @@ -835,7 +836,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { &stream_state->rs.read_slice_buffer, 0); *((grpc_byte_buffer **)stream_op->recv_message) = (grpc_byte_buffer *)&stream_state->rs.sbs; - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->recv_message_ready, + grpc_exec_ctx_sched(exec_ctx, stream_op->recv_message_ready, GRPC_ERROR_NONE, NULL); stream_state->state_op_done[OP_RECV_MESSAGE] = true; oas->state.state_op_done[OP_RECV_MESSAGE] = true; @@ -882,7 +883,7 @@ static enum OP_RESULT execute_stream_op(struct op_and_state *oas) { /* All actions in this stream_op are complete. Call the on_complete callback */ CRONET_LOG(GPR_DEBUG, "running: %p OP_ON_COMPLETE", oas); - grpc_exec_ctx_sched(&s->exec_ctx, stream_op->on_complete, GRPC_ERROR_NONE, + grpc_exec_ctx_sched(exec_ctx, stream_op->on_complete, GRPC_ERROR_NONE, NULL); oas->state.state_op_done[OP_ON_COMPLETE] = true; oas->done = true; @@ -923,7 +924,6 @@ static int init_stream(grpc_exec_ctx *exec_ctx, grpc_transport *gt, memset(s->state.state_callback_received, 0, sizeof(s->state.state_callback_received)); gpr_mu_init(&s->mu); - s->exec_ctx = *exec_ctx; return 0; } From f8f8f5a2ebad29108b53896e06b354e8d60e4705 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Wed, 17 Aug 2016 09:55:46 -0700 Subject: [PATCH 13/14] more review feedback addressed --- .../cronet/transport/cronet_transport.c | 130 +++++++++++------- 1 file changed, 83 insertions(+), 47 deletions(-) diff --git a/src/core/ext/transport/cronet/transport/cronet_transport.c b/src/core/ext/transport/cronet/transport/cronet_transport.c index 0fa79870760..ea131dbc043 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -46,6 +46,7 @@ #include "src/core/lib/support/string.h" #include "src/core/lib/surface/channel.h" #include "src/core/lib/transport/metadata_batch.h" +#include "src/core/lib/transport/static_metadata.h" #include "src/core/lib/transport/transport_impl.h" #include "third_party/objective_c/Cronet/cronet_c_for_grpc.h" @@ -59,18 +60,13 @@ /* TODO (makdharma): Hook up into the wider tracing mechanism */ int grpc_cronet_trace = 0; -enum OP_RESULT { +enum e_op_result { ACTION_TAKEN_WITH_CALLBACK, ACTION_TAKEN_NO_CALLBACK, NO_ACTION_POSSIBLE }; -/* Used for printing debug */ -const char *op_result_string[] = {"ACTION_TAKEN_WITH_CALLBACK", - "ACTION_TAKEN_NO_CALLBACK", - "NO_ACTION_POSSIBLE"}; - -enum OP_ID { +enum e_op_id { OP_SEND_INITIAL_METADATA = 0, OP_SEND_MESSAGE, OP_SEND_TRAILING_METADATA, @@ -87,22 +83,7 @@ enum OP_ID { OP_NUM_OPS }; -const char *op_id_string[] = {"OP_SEND_INITIAL_METADATA", - "OP_SEND_MESSAGE", - "OP_SEND_TRAILING_METADATA", - "OP_RECV_MESSAGE", - "OP_RECV_INITIAL_METADATA", - "OP_RECV_TRAILING_METADATA", - "OP_CANCEL_ERROR", - "OP_ON_COMPLETE", - "OP_FAILED", - "OP_SUCCEEDED", - "OP_CANCELED", - "OP_RECV_MESSAGE_AND_ON_COMPLETE", - "OP_READ_REQ_MADE", - "OP_NUM_OPS"}; - -/* Cronet callbacks */ +/* Cronet callbacks. See cronet_c_for_grpc.h for documentation for each. */ static void on_request_headers_sent(cronet_bidirectional_stream *); static void on_response_headers_received( @@ -134,6 +115,8 @@ struct grpc_cronet_transport { }; typedef struct grpc_cronet_transport grpc_cronet_transport; +/* TODO (makdharma): reorder structure for memory efficiency per + http://www.catb.org/esr/structure-packing/#_structure_reordering: */ struct read_state { /* vars to store data coming from server */ char *read_buffer; @@ -204,14 +187,61 @@ struct stream_obj { }; typedef struct stream_obj stream_obj; -static enum OP_RESULT execute_stream_op(grpc_exec_ctx *exec_ctx, - struct op_and_state *oas); +static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, + struct op_and_state *oas); + +/* + Utility function to translate enum into string for printing +*/ +static const char *op_result_string(enum e_op_result i) { + switch (i) { + case ACTION_TAKEN_WITH_CALLBACK: + return "ACTION_TAKEN_WITH_CALLBACK"; + case ACTION_TAKEN_NO_CALLBACK: + return "ACTION_TAKEN_NO_CALLBACK"; + case NO_ACTION_POSSIBLE: + return "NO_ACTION_POSSIBLE"; + } + GPR_UNREACHABLE_CODE(return "UNKNOWN"); +} + +static const char *op_id_string(enum e_op_id i) { + switch (i) { + case OP_SEND_INITIAL_METADATA: + return "OP_SEND_INITIAL_METADATA"; + case OP_SEND_MESSAGE: + return "OP_SEND_MESSAGE"; + case OP_SEND_TRAILING_METADATA: + return "OP_SEND_TRAILING_METADATA"; + case OP_RECV_MESSAGE: + return "OP_RECV_MESSAGE"; + case OP_RECV_INITIAL_METADATA: + return "OP_RECV_INITIAL_METADATA"; + case OP_RECV_TRAILING_METADATA: + return "OP_RECV_TRAILING_METADATA"; + case OP_CANCEL_ERROR: + return "OP_CANCEL_ERROR"; + case OP_ON_COMPLETE: + return "OP_ON_COMPLETE"; + case OP_FAILED: + return "OP_FAILED"; + case OP_SUCCEEDED: + return "OP_SUCCEEDED"; + case OP_CANCELED: + return "OP_CANCELED"; + case OP_RECV_MESSAGE_AND_ON_COMPLETE: + return "OP_RECV_MESSAGE_AND_ON_COMPLETE"; + case OP_READ_REQ_MADE: + return "OP_READ_REQ_MADE"; + case OP_NUM_OPS: + return "OP_NUM_OPS"; + } +} /* Add a new stream op to op storage. */ static void add_to_storage(struct stream_obj *s, grpc_transport_stream_op *op) { - gpr_mu_lock(&s->mu); struct op_storage *storage = &s->storage; /* add new op at the beginning of the linked list. The memory is freed in remove_from_storage */ @@ -220,6 +250,7 @@ static void add_to_storage(struct stream_obj *s, grpc_transport_stream_op *op) { memset(&new_op->state, 0, sizeof(new_op->state)); new_op->s = s; new_op->done = false; + gpr_mu_lock(&s->mu); new_op->next = storage->head; storage->head = new_op; storage->num_pending_ops++; @@ -271,9 +302,9 @@ static void execute_from_storage(stream_obj *s) { for (struct op_and_state *curr = s->storage.head; curr != NULL;) { CRONET_LOG(GPR_DEBUG, "calling op at %p. done = %d", curr, curr->done); GPR_ASSERT(curr->done == 0); - enum OP_RESULT result = execute_stream_op(&exec_ctx, curr); + enum e_op_result result = execute_stream_op(&exec_ctx, curr); CRONET_LOG(GPR_DEBUG, "execute_stream_op[%p] returns %s", curr, - op_result_string[result]); + op_result_string(result)); /* if this op is done, then remove it and free memory */ if (curr->done) { struct op_and_state *next = curr->next; @@ -372,8 +403,7 @@ static void on_response_headers_received( memset(&s->state.rs.initial_metadata, 0, sizeof(s->state.rs.initial_metadata)); grpc_chttp2_incoming_metadata_buffer_init(&s->state.rs.initial_metadata); - unsigned int i = 0; - for (i = 0; i < headers->count; i++) { + for (size_t i = 0; i < headers->count; i++) { grpc_chttp2_incoming_metadata_buffer_add( &s->state.rs.initial_metadata, grpc_mdelem_from_metadata_strings( @@ -439,8 +469,7 @@ static void on_response_trailers_received( sizeof(s->state.rs.trailing_metadata)); s->state.rs.trailing_metadata_valid = false; grpc_chttp2_incoming_metadata_buffer_init(&s->state.rs.trailing_metadata); - unsigned int i = 0; - for (i = 0; i < trailers->count; i++) { + for (size_t i = 0; i < trailers->count; i++) { CRONET_LOG(GPR_DEBUG, "trailer key=%s, value=%s", trailers->headers[i].key, trailers->headers[i].value); grpc_chttp2_incoming_metadata_buffer_add( @@ -460,10 +489,10 @@ static void on_response_trailers_received( */ static void create_grpc_frame(gpr_slice_buffer *write_slice_buffer, char **pp_write_buffer, - int *p_write_buffer_size) { + size_t *p_write_buffer_size) { gpr_slice slice = gpr_slice_buffer_take_first(write_slice_buffer); size_t length = GPR_SLICE_LENGTH(slice); - *p_write_buffer_size = (int)length + GRPC_HEADER_SIZE_IN_BYTES; + *p_write_buffer_size = length + GRPC_HEADER_SIZE_IN_BYTES; /* This is freed in the on_write_completed callback */ char *write_buffer = gpr_malloc(length + GRPC_HEADER_SIZE_IN_BYTES); *pp_write_buffer = write_buffer; @@ -500,7 +529,8 @@ static void convert_metadata_to_cronet_headers( /* Walk the linked list again, this time copying the header fields. s->num_headers can be less than num_headers_available, as some headers - are not used for cronet + are not used for cronet. + TODO (makdharma): Eliminate need to traverse the LL second time for perf. */ curr = head; int num_headers = 0; @@ -509,12 +539,12 @@ static void convert_metadata_to_cronet_headers( curr = curr->next; const char *key = grpc_mdstr_as_c_string(mdelem->key); const char *value = grpc_mdstr_as_c_string(mdelem->value); - if (strcmp(key, ":scheme") == 0 || strcmp(key, ":method") == 0 || - strcmp(key, ":authority") == 0) { + if (mdelem->key == GRPC_MDSTR_METHOD || mdelem->key == GRPC_MDSTR_SCHEME || + mdelem->key == GRPC_MDSTR_AUTHORITY) { /* Cronet populates these fields on its own */ continue; } - if (strcmp(key, ":path") == 0) { + if (mdelem->key == GRPC_MDSTR_PATH) { /* Create URL by appending :path value to the hostname */ gpr_asprintf(pp_url, "https://%s%s", host, value); continue; @@ -546,13 +576,14 @@ static int parse_grpc_header(const uint8_t *data) { */ static bool op_can_be_run(grpc_transport_stream_op *curr_op, struct op_state *stream_state, - struct op_state *op_state, enum OP_ID op_id) { + struct op_state *op_state, enum e_op_id op_id) { bool result = true; /* When call is canceled, every op can be run, except under following conditions */ - if (stream_state->state_op_done[OP_CANCEL_ERROR] || - stream_state->state_callback_received[OP_FAILED]) { + bool is_canceled_of_failed = stream_state->state_op_done[OP_CANCEL_ERROR] || + stream_state->state_callback_received[OP_FAILED]; + if (is_canceled_of_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; @@ -678,17 +709,20 @@ static bool op_can_be_run(grpc_transport_stream_op *curr_op, !stream_state->state_callback_received[OP_SEND_MESSAGE]) result = false; } - CRONET_LOG(GPR_DEBUG, "op_can_be_run %s : %s", op_id_string[op_id], + CRONET_LOG(GPR_DEBUG, "op_can_be_run %s : %s", op_id_string(op_id), result ? "YES" : "NO"); return result; } -static enum OP_RESULT execute_stream_op(grpc_exec_ctx *exec_ctx, - struct op_and_state *oas) { +/* + TODO (makdharma): Break down this function in smaller chunks for readability. +*/ +static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, + struct op_and_state *oas) { grpc_transport_stream_op *stream_op = &oas->op; struct stream_obj *s = oas->s; struct op_state *stream_state = &s->state; - enum OP_RESULT result = NO_ACTION_POSSIBLE; + enum e_op_result result = NO_ACTION_POSSIBLE; if (stream_op->send_initial_metadata && op_can_be_run(stream_op, stream_state, &oas->state, OP_SEND_INITIAL_METADATA)) { @@ -743,19 +777,21 @@ static enum OP_RESULT execute_stream_op(grpc_exec_ctx *exec_ctx, grpc_byte_stream_next(NULL, stream_op->send_message, &slice, stream_op->send_message->length, NULL); /* Check that compression flag is OFF. We don't support compression yet. */ + gpr_log(GPR_ERROR, "Compression is not supported"); GPR_ASSERT(stream_op->send_message->flags == 0); gpr_slice_buffer_add(&write_slice_buffer, slice); + gpr_log(GPR_ERROR, "Empty request is not supported"); GPR_ASSERT(write_slice_buffer.count == 1); /* Empty request not handled yet */ if (write_slice_buffer.count > 0) { - int write_buffer_size; + size_t write_buffer_size; create_grpc_frame(&write_slice_buffer, &stream_state->ws.write_buffer, &write_buffer_size); CRONET_LOG(GPR_DEBUG, "cronet_bidirectional_stream_write (%p, %p)", s->cbs, stream_state->ws.write_buffer); stream_state->state_callback_received[OP_SEND_MESSAGE] = false; cronet_bidirectional_stream_write(s->cbs, stream_state->ws.write_buffer, - write_buffer_size, false); + (int)write_buffer_size, false); result = ACTION_TAKEN_WITH_CALLBACK; } stream_state->state_op_done[OP_SEND_MESSAGE] = true; From d8004a86fa4b5b329454701af22238861097736f Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Wed, 17 Aug 2016 10:01:36 -0700 Subject: [PATCH 14/14] minor tweak --- .../transport/cronet/transport/cronet_transport.c | 14 +++++++++----- 1 file changed, 9 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 ea131dbc043..f7726a971e9 100644 --- a/src/core/ext/transport/cronet/transport/cronet_transport.c +++ b/src/core/ext/transport/cronet/transport/cronet_transport.c @@ -777,12 +777,16 @@ static enum e_op_result execute_stream_op(grpc_exec_ctx *exec_ctx, grpc_byte_stream_next(NULL, stream_op->send_message, &slice, stream_op->send_message->length, NULL); /* Check that compression flag is OFF. We don't support compression yet. */ - gpr_log(GPR_ERROR, "Compression is not supported"); - GPR_ASSERT(stream_op->send_message->flags == 0); + if (stream_op->send_message->flags != 0) { + gpr_log(GPR_ERROR, "Compression is not supported"); + GPR_ASSERT(stream_op->send_message->flags == 0); + } gpr_slice_buffer_add(&write_slice_buffer, slice); - gpr_log(GPR_ERROR, "Empty request is not supported"); - GPR_ASSERT(write_slice_buffer.count == - 1); /* Empty request not handled yet */ + if (write_slice_buffer.count != 1) { + /* Empty request not handled yet */ + gpr_log(GPR_ERROR, "Empty request is not supported"); + GPR_ASSERT(write_slice_buffer.count == 1); + } if (write_slice_buffer.count > 0) { size_t write_buffer_size; create_grpc_frame(&write_slice_buffer, &stream_state->ws.write_buffer,