Try harder to return DEADLINE_EXCEEDED when we should

Do this by ensuring that the alarm callback has had a chance to run on a
call before returning status to the application.

If we do not do this:
- the server alarm could be scheduled and run
- it will write a RST_STREAM with a status that loses the deadline
  exceededness (because that is unexpressable in HTTP2 error codes)
- it will be received by the client and processed
- the client will return an INTERNAL error (the lossy re-encoding of the
  server status), and then run its alarm handler to set the status to
  something else

1 files changed, 17 insertions, 11 deletions

diff --git a/src/core/surface/call.c b/src/core/surface/call.c
index 02e0e59cad..181617fff8 100644
--- a/src/core/surface/call.c
+++ b/src/core/surface/call.c
@@ -160,6 +160,8 @@ struct grpc_call {
   gpr_uint8 bound_pollset;
   /* is an error status set */
   gpr_uint8 error_status_set;
+  /** should the alarm be cancelled */
+  gpr_uint8 cancel_alarm;
 
   /* flags with bits corresponding to write states allowing us to determine
      what was sent */
@@ -472,6 +474,7 @@ static void unlock(grpc_call *call) {
   int completing_requests = 0;
   int start_op = 0;
   int i;
+  int cancel_alarm = 0;
 
   memset(&op, 0, sizeof(op));
 
@@ -479,6 +482,9 @@ static void unlock(grpc_call *call) {
   start_op = op.cancel_with_status != GRPC_STATUS_OK;
   call->cancel_with_status = GRPC_STATUS_OK; /* reset */
 
+  cancel_alarm = call->cancel_alarm;
+  call->cancel_alarm = 0;
+
   if (!call->receiving && need_more_data(call)) {
     op.recv_ops = &call->recv_ops;
     op.recv_state = &call->recv_state;
@@ -513,6 +519,10 @@ static void unlock(grpc_call *call) {
 
   gpr_mu_unlock(&call->mu);
 
+  if (cancel_alarm) {
+    grpc_alarm_cancel(&call->alarm);
+  }
+
   if (start_op) {
     execute_op(call, &op);
   }
@@ -805,10 +815,7 @@ static void call_on_done_recv(void *pc, int success) {
     if (call->recv_state == GRPC_STREAM_CLOSED) {
       GPR_ASSERT(call->read_state <= READ_STATE_STREAM_CLOSED);
       call->read_state = READ_STATE_STREAM_CLOSED;
-      if (call->have_alarm) {
-        grpc_alarm_cancel(&call->alarm);
-        call->have_alarm = 0;
-      }
+      call->cancel_alarm |= call->have_alarm;
       GRPC_CALL_INTERNAL_UNREF(call, "closed", 0);
     }
     finish_read_ops(call);
@@ -987,7 +994,7 @@ static void finish_read_ops(grpc_call *call) {
 
   switch (call->read_state) {
     case READ_STATE_STREAM_CLOSED:
-      if (empty) {
+      if (empty && !call->have_alarm) {
         finish_ioreq_op(call, GRPC_IOREQ_RECV_CLOSE, 1);
       }
     /* fallthrough */
@@ -1085,10 +1092,7 @@ void grpc_call_destroy(grpc_call *c) {
   lock(c);
   GPR_ASSERT(!c->destroy_called);
   c->destroy_called = 1;
-  if (c->have_alarm) {
-    grpc_alarm_cancel(&c->alarm);
-    c->have_alarm = 0;
-  }
+  c->cancel_alarm |= c->have_alarm;
   cancel = c->read_state != READ_STATE_STREAM_CLOSED;
   unlock(c);
   if (cancel) grpc_call_cancel(c);
@@ -1167,12 +1171,14 @@ grpc_call *grpc_call_from_top_element(grpc_call_element *elem) {
 
 static void call_alarm(void *arg, int success) {
   grpc_call *call = arg;
+  lock(call);
+  call->have_alarm = 0;
   if (success) {
-    lock(call);
     cancel_with_status(call, GRPC_STATUS_DEADLINE_EXCEEDED,
                        "Deadline Exceeded");
-    unlock(call);
   }
+  finish_read_ops(call);
+  unlock(call);
   GRPC_CALL_INTERNAL_UNREF(call, "alarm", 1);
 }