Error handling cleanup

7 files changed, 100 insertions, 71 deletions

diff --git a/src/core/lib/transport/error_utils.c b/src/core/lib/transport/error_utils.c
index 6403cf80d8..da77828d9c 100644
--- a/src/core/lib/transport/error_utils.c
+++ b/src/core/lib/transport/error_utils.c
@@ -32,12 +32,14 @@
  */
 
 #include "src/core/lib/transport/error_utils.h"
+
 #include "src/core/lib/iomgr/error_internal.h"
+#include "src/core/lib/transport/status_conversion.h"
 
-static grpc_error *recursively_find_error_with_status(grpc_error *error,
-                                                      intptr_t *status) {
+static grpc_error *recursively_find_error_with_field(grpc_error *error,
+                                                     grpc_error_ints which) {
   // If the error itself has a status code, return it.
-  if (grpc_error_get_int(error, GRPC_ERROR_INT_GRPC_STATUS, status)) {
+  if (grpc_error_get_int(error, which, NULL)) {
     return error;
   }
   if (grpc_error_is_special(error)) return NULL;
@@ -46,32 +48,64 @@ static grpc_error *recursively_find_error_with_status(grpc_error *error,
   while (true) {
     grpc_error *child_error = gpr_avl_get(error->errs, (void *)key++);
     if (child_error == NULL) break;
-    grpc_error *result =
-        recursively_find_error_with_status(child_error, status);
+    grpc_error *result = recursively_find_error_with_field(child_error, which);
     if (result != NULL) return result;
   }
   return NULL;
 }
 
-void grpc_error_get_status(grpc_error *error, grpc_status_code *code,
-                           const char **msg) {
-  // Populate code.
+void grpc_error_get_status(grpc_error *error, gpr_timespec deadline,
+                           grpc_status_code *code, const char **msg,
+                           grpc_http2_error_code *http_error) {
   // Start with the parent error and recurse through the tree of children
   // until we find the first one that has a status code.
-  intptr_t status = GRPC_STATUS_UNKNOWN;  // Default in case we don't find one.
-  grpc_error *found_error = recursively_find_error_with_status(error, &status);
-  *code = (grpc_status_code)status;
-  // Now populate msg.
+  grpc_error *found_error =
+      recursively_find_error_with_field(error, GRPC_ERROR_INT_GRPC_STATUS);
+  if (found_error == NULL) {
+    /// If no grpc-status exists, retry through the tree to find a http2 error
+    /// code
+    found_error =
+        recursively_find_error_with_field(error, GRPC_ERROR_INT_HTTP2_ERROR);
+  }
+
   // If we found an error with a status code above, use that; otherwise,
   // fall back to using the parent error.
   if (found_error == NULL) found_error = error;
+
+  grpc_status_code status = GRPC_STATUS_UNKNOWN;
+  intptr_t integer;
+  if (grpc_error_get_int(found_error, GRPC_ERROR_INT_GRPC_STATUS, &integer)) {
+    status = (grpc_status_code)integer;
+  } else if (grpc_error_get_int(found_error, GRPC_ERROR_INT_HTTP2_ERROR,
+                                &integer)) {
+    status = grpc_http2_error_to_grpc_status((grpc_http2_error_code)integer,
+                                             deadline);
+  }
+  if (code != NULL) *code = status;
+
+  if (http_error != NULL) {
+    if (grpc_error_get_int(found_error, GRPC_ERROR_INT_HTTP2_ERROR, &integer)) {
+      *http_error = (grpc_http2_error_code)integer;
+    } else if (grpc_error_get_int(found_error, GRPC_ERROR_INT_GRPC_STATUS,
+                                  &integer)) {
+      *http_error = grpc_status_to_http2_error((grpc_status_code)integer);
+    } else {
+      *http_error = found_error == GRPC_ERROR_NONE ? GRPC_HTTP2_NO_ERROR
+                                                   : GRPC_HTTP2_INTERNAL_ERROR;
+    }
+  }
+
   // If the error has a status message, use it.  Otherwise, fall back to
   // the error description.
-  *msg = grpc_error_get_str(found_error, GRPC_ERROR_STR_GRPC_MESSAGE);
-  if (*msg == NULL && status != GRPC_STATUS_OK) {
-    *msg = grpc_error_get_str(found_error, GRPC_ERROR_STR_DESCRIPTION);
-    if (*msg == NULL) *msg = "unknown error";  // Just in case.
+  if (msg != NULL) {
+    *msg = grpc_error_get_str(found_error, GRPC_ERROR_STR_GRPC_MESSAGE);
+    if (*msg == NULL && error != GRPC_ERROR_NONE) {
+      *msg = grpc_error_get_str(found_error, GRPC_ERROR_STR_DESCRIPTION);
+      if (*msg == NULL) *msg = "unknown error";  // Just in case.
+    }
   }
+
+  if (found_error == NULL) found_error = error;
 }
 
 bool grpc_error_has_clear_grpc_status(grpc_error *error) {
diff --git a/src/core/lib/transport/error_utils.h b/src/core/lib/transport/error_utils.h
index 541dba852f..f72c3dca0e 100644
--- a/src/core/lib/transport/error_utils.h
+++ b/src/core/lib/transport/error_utils.h
@@ -35,12 +35,18 @@
 #define GRPC_ERROR_UTILS_H
 
 #include "src/core/lib/iomgr/error.h"
+#include "src/core/lib/transport/http2_errors.h"
 
 /// A utility function to get the status code and message to be returned
 /// to the application.  If not set in the top-level message, looks
 /// through child errors until it finds the first one with these attributes.
-void grpc_error_get_status(grpc_error *error, grpc_status_code *code,
-                           const char **msg);
+/// All attributes are pulled from the same child error. If any of the
+/// attributes (code, msg, http_status) are unneeded, they can be passed as
+/// NULL.
+void grpc_error_get_status(grpc_error *error, gpr_timespec deadline,
+                           grpc_status_code *code, const char **msg,
+                           grpc_http2_error_code *http_status);
+
 /// A utility function to check whether there is a clear status code that
 /// doesn't need to be guessed in \a error. This means that \a error or some
 /// child has GRPC_ERROR_INT_GRPC_STATUS set, or that it is GRPC_ERROR_NONE or
diff --git a/src/core/lib/transport/http2_errors.h b/src/core/lib/transport/http2_errors.h
index deab2b7e3e..bf24438dde 100644
--- a/src/core/lib/transport/http2_errors.h
+++ b/src/core/lib/transport/http2_errors.h
@@ -36,21 +36,21 @@
 
 /* error codes for RST_STREAM from http2 draft 14 section 7 */
 typedef enum {
-  GRPC_CHTTP2_NO_ERROR = 0x0,
-  GRPC_CHTTP2_PROTOCOL_ERROR = 0x1,
-  GRPC_CHTTP2_INTERNAL_ERROR = 0x2,
-  GRPC_CHTTP2_FLOW_CONTROL_ERROR = 0x3,
-  GRPC_CHTTP2_SETTINGS_TIMEOUT = 0x4,
-  GRPC_CHTTP2_STREAM_CLOSED = 0x5,
-  GRPC_CHTTP2_FRAME_SIZE_ERROR = 0x6,
-  GRPC_CHTTP2_REFUSED_STREAM = 0x7,
-  GRPC_CHTTP2_CANCEL = 0x8,
-  GRPC_CHTTP2_COMPRESSION_ERROR = 0x9,
-  GRPC_CHTTP2_CONNECT_ERROR = 0xa,
-  GRPC_CHTTP2_ENHANCE_YOUR_CALM = 0xb,
-  GRPC_CHTTP2_INADEQUATE_SECURITY = 0xc,
+  GRPC_HTTP2_NO_ERROR = 0x0,
+  GRPC_HTTP2_PROTOCOL_ERROR = 0x1,
+  GRPC_HTTP2_INTERNAL_ERROR = 0x2,
+  GRPC_HTTP2_FLOW_CONTROL_ERROR = 0x3,
+  GRPC_HTTP2_SETTINGS_TIMEOUT = 0x4,
+  GRPC_HTTP2_STREAM_CLOSED = 0x5,
+  GRPC_HTTP2_FRAME_SIZE_ERROR = 0x6,
+  GRPC_HTTP2_REFUSED_STREAM = 0x7,
+  GRPC_HTTP2_CANCEL = 0x8,
+  GRPC_HTTP2_COMPRESSION_ERROR = 0x9,
+  GRPC_HTTP2_CONNECT_ERROR = 0xa,
+  GRPC_HTTP2_ENHANCE_YOUR_CALM = 0xb,
+  GRPC_HTTP2_INADEQUATE_SECURITY = 0xc,
   /* force use of a default clause */
-  GRPC_CHTTP2__ERROR_DO_NOT_USE = -1
-} grpc_chttp2_error_code;
+  GRPC_HTTP2__ERROR_DO_NOT_USE = -1
+} grpc_http2_error_code;
 
 #endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_HTTP2_ERRORS_H */
diff --git a/src/core/lib/transport/status_conversion.c b/src/core/lib/transport/status_conversion.c
index eb1d53c8d1..af0ac89db7 100644
--- a/src/core/lib/transport/status_conversion.c
+++ b/src/core/lib/transport/status_conversion.c
@@ -33,49 +33,49 @@
 
 #include "src/core/lib/transport/status_conversion.h"
 
-int grpc_chttp2_grpc_status_to_http2_error(grpc_status_code status) {
+int grpc_status_to_http2_error(grpc_status_code status) {
   switch (status) {
     case GRPC_STATUS_OK:
-      return GRPC_CHTTP2_NO_ERROR;
+      return GRPC_HTTP2_NO_ERROR;
     case GRPC_STATUS_CANCELLED:
-      return GRPC_CHTTP2_CANCEL;
+      return GRPC_HTTP2_CANCEL;
     case GRPC_STATUS_DEADLINE_EXCEEDED:
-      return GRPC_CHTTP2_CANCEL;
+      return GRPC_HTTP2_CANCEL;
     case GRPC_STATUS_RESOURCE_EXHAUSTED:
-      return GRPC_CHTTP2_ENHANCE_YOUR_CALM;
+      return GRPC_HTTP2_ENHANCE_YOUR_CALM;
     case GRPC_STATUS_PERMISSION_DENIED:
-      return GRPC_CHTTP2_INADEQUATE_SECURITY;
+      return GRPC_HTTP2_INADEQUATE_SECURITY;
     case GRPC_STATUS_UNAVAILABLE:
-      return GRPC_CHTTP2_REFUSED_STREAM;
+      return GRPC_HTTP2_REFUSED_STREAM;
     default:
-      return GRPC_CHTTP2_INTERNAL_ERROR;
+      return GRPC_HTTP2_INTERNAL_ERROR;
   }
 }
 
-grpc_status_code grpc_chttp2_http2_error_to_grpc_status(
-    grpc_chttp2_error_code error, gpr_timespec deadline) {
+grpc_status_code grpc_http2_error_to_grpc_status(grpc_http2_error_code error,
+                                                 gpr_timespec deadline) {
   switch (error) {
-    case GRPC_CHTTP2_NO_ERROR:
+    case GRPC_HTTP2_NO_ERROR:
       /* should never be received */
       return GRPC_STATUS_INTERNAL;
-    case GRPC_CHTTP2_CANCEL:
+    case GRPC_HTTP2_CANCEL:
       /* http2 cancel translates to STATUS_CANCELLED iff deadline hasn't been
        * exceeded */
       return gpr_time_cmp(gpr_now(deadline.clock_type), deadline) >= 0
                  ? GRPC_STATUS_DEADLINE_EXCEEDED
                  : GRPC_STATUS_CANCELLED;
-    case GRPC_CHTTP2_ENHANCE_YOUR_CALM:
+    case GRPC_HTTP2_ENHANCE_YOUR_CALM:
       return GRPC_STATUS_RESOURCE_EXHAUSTED;
-    case GRPC_CHTTP2_INADEQUATE_SECURITY:
+    case GRPC_HTTP2_INADEQUATE_SECURITY:
       return GRPC_STATUS_PERMISSION_DENIED;
-    case GRPC_CHTTP2_REFUSED_STREAM:
+    case GRPC_HTTP2_REFUSED_STREAM:
       return GRPC_STATUS_UNAVAILABLE;
     default:
       return GRPC_STATUS_INTERNAL;
   }
 }
 
-grpc_status_code grpc_chttp2_http2_status_to_grpc_status(int status) {
+grpc_status_code grpc_http2_status_to_grpc_status(int status) {
   switch (status) {
     /* these HTTP2 status codes are called out explicitly in status.proto */
     case 200:
@@ -110,6 +110,4 @@ grpc_status_code grpc_chttp2_http2_status_to_grpc_status(int status) {
   }
 }
 
-int grpc_chttp2_grpc_status_to_http2_status(grpc_status_code status) {
-  return 200;
-}
+int grpc_status_to_http2_status(grpc_status_code status) { return 200; }
diff --git a/src/core/lib/transport/status_conversion.h b/src/core/lib/transport/status_conversion.h
index 592411529d..3885ac90a5 100644
--- a/src/core/lib/transport/status_conversion.h
+++ b/src/core/lib/transport/status_conversion.h
@@ -38,13 +38,12 @@
 #include "src/core/lib/transport/http2_errors.h"
 
 /* Conversion of grpc status codes to http2 error codes (for RST_STREAM) */
-grpc_chttp2_error_code grpc_chttp2_grpc_status_to_http2_error(
-    grpc_status_code status);
-grpc_status_code grpc_chttp2_http2_error_to_grpc_status(
-    grpc_chttp2_error_code error, gpr_timespec deadline);
+grpc_http2_error_code grpc_status_to_http2_error(grpc_status_code status);
+grpc_status_code grpc_http2_error_to_grpc_status(grpc_http2_error_code error,
+                                                 gpr_timespec deadline);
 
 /* Conversion of HTTP status codes (:status) to grpc status codes */
-grpc_status_code grpc_chttp2_http2_status_to_grpc_status(int status);
-int grpc_chttp2_grpc_status_to_http2_status(grpc_status_code status);
+grpc_status_code grpc_http2_status_to_grpc_status(int status);
+int grpc_status_to_http2_status(grpc_status_code status);
 
 #endif /* GRPC_CORE_EXT_TRANSPORT_CHTTP2_TRANSPORT_STATUS_CONVERSION_H */
diff --git a/src/core/lib/transport/transport.h b/src/core/lib/transport/transport.h
index 2dbb842a53..9a0abe1ca4 100644
--- a/src/core/lib/transport/transport.h
+++ b/src/core/lib/transport/transport.h
@@ -181,13 +181,8 @@ typedef struct grpc_transport_op {
   grpc_connectivity_state *connectivity_state;
   /** should the transport be disconnected */
   grpc_error *disconnect_with_error;
-  /** should we send a goaway?
-      after a goaway is sent, once there are no more active calls on
-      the transport, the transport should disconnect */
-  bool send_goaway;
   /** what should the goaway contain? */
-  grpc_status_code goaway_status;
-  grpc_slice *goaway_message;
+  grpc_error *goaway_error;
   /** set the callback for accepting new streams;
       this is a permanent callback, unlike the other one-shot closures.
       If true, the callback is set to set_accept_stream_fn, with its
diff --git a/src/core/lib/transport/transport_op_string.c b/src/core/lib/transport/transport_op_string.c
index 33bd35bd45..8810a6bbda 100644
--- a/src/core/lib/transport/transport_op_string.c
+++ b/src/core/lib/transport/transport_op_string.c
@@ -163,15 +163,12 @@ char *grpc_transport_op_string(grpc_transport_op *op) {
     grpc_error_free_string(err);
   }
 
-  if (op->send_goaway) {
+  if (op->goaway_error) {
     if (!first) gpr_strvec_add(&b, gpr_strdup(" "));
     first = false;
-    char *msg = op->goaway_message == NULL
-                    ? "null"
-                    : grpc_dump_slice(*op->goaway_message,
-                                      GPR_DUMP_ASCII | GPR_DUMP_HEX);
-    gpr_asprintf(&tmp, "SEND_GOAWAY:status=%d:msg=%s", op->goaway_status, msg);
-    if (op->goaway_message != NULL) gpr_free(msg);
+    const char *msg = grpc_error_string(op->goaway_error);
+    gpr_asprintf(&tmp, "SEND_GOAWAY:%s", msg);
+    grpc_error_free_string(msg);
     gpr_strvec_add(&b, tmp);
   }