Server stall fix #4453

ctiller · 2015-12-14T21:40:35Z

Built on #4451, #4452 which should be merged first

@sreecha - if I don't get the above merged before you're gone, I'll find another reviewer

sreecha · 2015-12-14T22:02:10Z

src/core/transport/chttp2_transport.c

+      exec_ctx, &stream_global->recv_initial_metadata_finished, 0);
+  grpc_chttp2_complete_closure_step(
+      exec_ctx, &stream_global->recv_trailing_metadata_finished, 0);
+}


Should recv_message_ready closure also be queued here ?

I think so. This is built on top of #4451 - and that's what was required
for those tests. I'm going to start merging your fixes into this and see if
I can come up with a grand unified PR that addresses all the lost callback
bugs.

On Mon, Dec 14, 2015 at 2:02 PM Sree Kuchibhotla notifications@github.com
wrote:

In src/core/transport/chttp2_transport.c
#4453 (comment):

@@ -748,6 +753,21 @@ void grpc_chttp2_complete_closure_step(grpc_exec_ctx *exec_ctx,
*pclosure = NULL;
}

+static void fail_all_outstanding_ops(

grpc_exec_ctx *exec_ctx, grpc_chttp2_transport_global *transport_global,

grpc_chttp2_stream_global *stream_global) {

grpc_chttp2_complete_closure_step(

exec_ctx, &stream_global->send_initial_metadata_finished, 0);

grpc_chttp2_complete_closure_step(

exec_ctx, &stream_global->send_trailing_metadata_finished, 0);

grpc_chttp2_complete_closure_step(exec_ctx,

&stream_global->send_message_finished, 0);

grpc_chttp2_complete_closure_step(

exec_ctx, &stream_global->recv_initial_metadata_finished, 0);

grpc_chttp2_complete_closure_step(

exec_ctx, &stream_global->recv_trailing_metadata_finished, 0);
+}

Should recv_message_ready closure also be queued here ?

—
Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc/pull/4453/files#r47565257.

ctiller · 2015-12-14T23:04:22Z

It'd be worth @sreecha looking at 38edec6 however.

sreecha · 2015-12-14T23:36:42Z

src/core/transport/chttp2_transport.c

+    if (from_parsing_thread) {
+      gpr_mu_lock(&bs->transport->mu);
+    }
+    grpc_exec_ctx_enqueue(exec_ctx, bs->on_next, 0);


Is it guaranteed that bs->on_next would be non-null here ?

grpc_exec_ctx_enqueue accepts a null closure (and does nothing) -- since
it's awfully convenient.

On Mon, Dec 14, 2015 at 3:36 PM Sree Kuchibhotla notifications@github.com
wrote:

In src/core/transport/chttp2_transport.c
#4453 (comment):

@@ -1513,7 +1552,29 @@ void grpc_chttp2_incoming_byte_stream_push(grpc_exec_ctx *exec_ctx,
}

void grpc_chttp2_incoming_byte_stream_finished(

grpc_exec_ctx *exec_ctx, grpc_chttp2_incoming_byte_stream *bs) {

grpc_exec_ctx *exec_ctx, grpc_chttp2_incoming_byte_stream *bs, int success,

int from_parsing_thread) {

if (!success) {

if (from_parsing_thread) {

gpr_mu_lock(&bs->transport->mu);

}

grpc_exec_ctx_enqueue(exec_ctx, bs->on_next, 0);

Is it guaranteed that bs->on_next would be non-null here ?

—
Reply to this email directly or view it on GitHub
https://github.com/grpc/grpc/pull/4453/files#r47576693.

Sounds good.

sreecha · 2015-12-14T23:51:17Z

Looks good but I do not see this addressing the other reason for server stall i.e trying to execute pending requests on a closed transport. i.e in queue_call_request() function

static grpc_call_error queue_call_request(...) {
...
} else {
 GPR_ASSERT(calld->state == PENDING);
        calld->state = ACTIVATED;
        gpr_mu_unlock(&calld->mu_state);
        begin_call(exec_ctx, server, calld,
                   &server->requested_calls[request_id]);

I know that this will eventually call perform_stream_op_locked() function in chttp2_transport.c but that function checks for contains_non_ok_status() in only two cases (i.e op->sending_initial_metadata or op->send_trailing_metadata) and in this particular case, i believe they both would be set to NULL and its the op->recv_message that is non-null.

How will it handle this case ?

ctiller · 2015-12-15T05:25:11Z

Accidentally I think this is covered (although I'd prefer it be called out better: perhaps future work).

We either explicity check read_closed or write_closed and early out the completion, or add the stream to the check_read_ops list... and then do the verification there.

ctiller · 2015-12-15T16:10:58Z

Such green.

sreecha · 2015-12-15T16:55:40Z

The changes LGTM. Btw, do we also need to make changes at the C++ level ? https://github.com/grpc/grpc/pull/4450/files#diff-7b669dd2948b796b465aa95ec1c42962

(when I checked in my testing mrd->Request(server_, cq_.cq()); was not called if ok == false after auto* mrd = SyncRequest::Wait(&cq_, &ok); (which would be the case here).

ctiller · 2015-12-15T17:21:50Z

I'm not sure honestly. I think I'd like to get a test up for this for C++ also.

sreecha · 2015-12-16T02:22:35Z

The interop test failure seems unrelated to this change. It fails even for other pull requests. This change can be merged.
I did not get a chance to test this today (but the instructions are here: #4179 ) although I think unless we make changes in C++ code, we will continue seeing this error..

Server stall fix

ctiller · 2015-12-16T04:27:50Z

I'm going to experiment if I get a chance in the next few days... I expect
you're right, but want to prove it to myself first.

On Tue, Dec 15, 2015, 6:23 PM Sree Kuchibhotla notifications@github.com
wrote:

Merged #4453 #4453.

—
Reply to this email directly or view it on GitHub
#4453 (comment).

sreecha · 2015-12-16T05:51:33Z

Sounds good Craig!.. thanks for taking care of this and see you next year!

ctiller and others added 3 commits December 14, 2015 12:27

Add a registered server method to bad_client

ac3220b

Add a test that reproduces server partial message read bug

83399c9

clang-format

6cb6917

ctiller added the disposition/DO NOT MERGE label Dec 14, 2015

googlebot added the cla: yes label Dec 14, 2015

sreecha reviewed Dec 14, 2015
View reviewed changes

Fix server side handling of incoming partial requests in core

38edec6

ctiller removed the disposition/DO NOT MERGE label Dec 14, 2015

ctiller assigned sreecha Dec 14, 2015

ctiller changed the title ~~Server stall reproduction~~ Server stall fix Dec 14, 2015

Merge branch 'proxy-crash' into server_stall

b54a9cc

sreecha reviewed Dec 14, 2015
View reviewed changes

ctiller and others added 4 commits December 14, 2015 21:51

Merge branch 'proxy-crash' into server_stall

1a423ef

Merge github.com:grpc/grpc into server_stall

7857537

recv_message_ready was already taken care of

31d0d1e

Merge branch 'proxy-crash' into server_stall

f71910c

Merge branch 'proxy-crash' into server_stall

ef1d424

sreecha added a commit that referenced this pull request Dec 16, 2015

Merge pull request #4453 from ctiller/server_stall

e5cdbea

Server stall fix

sreecha merged commit e5cdbea into grpc:master Dec 16, 2015

sreecha mentioned this pull request Jan 7, 2016

Fix server stall issue in case some requests get cancelled midway by the client #4450

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 29, 2019

lock bot unassigned sreecha Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Server stall fix #4453

Server stall fix #4453

ctiller commented Dec 14, 2015

sreecha Dec 14, 2015

ctiller Dec 14, 2015

ctiller commented Dec 14, 2015

sreecha Dec 14, 2015

ctiller Dec 14, 2015

sreecha Dec 14, 2015

sreecha commented Dec 14, 2015

ctiller commented Dec 15, 2015

ctiller commented Dec 15, 2015

sreecha commented Dec 15, 2015

ctiller commented Dec 15, 2015

sreecha commented Dec 16, 2015

ctiller commented Dec 16, 2015

sreecha commented Dec 16, 2015

Server stall fix #4453

Server stall fix #4453

Conversation

ctiller commented Dec 14, 2015

sreecha Dec 14, 2015

Choose a reason for hiding this comment

ctiller Dec 14, 2015

Choose a reason for hiding this comment

ctiller commented Dec 14, 2015

sreecha Dec 14, 2015

Choose a reason for hiding this comment

ctiller Dec 14, 2015

Choose a reason for hiding this comment

sreecha Dec 14, 2015

Choose a reason for hiding this comment

sreecha commented Dec 14, 2015

ctiller commented Dec 15, 2015

ctiller commented Dec 15, 2015

sreecha commented Dec 15, 2015

ctiller commented Dec 15, 2015

sreecha commented Dec 16, 2015

ctiller commented Dec 16, 2015

sreecha commented Dec 16, 2015