Various fixes on absl::Status migration (#27557)

src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_resolver.cc

@@ -1162,7 +1162,7 @@ class XdsClusterResolverLbFactory : public LoadBalancingPolicyFactory {
         if (!discovery_mechanism_errors.empty()) {
           grpc_error_handle error = GRPC_ERROR_CREATE_FROM_CPP_STRING(
               absl::StrCat("field:discovery_mechanism element: ", i, " error"));
-          for (grpc_error_handle discovery_mechanism_error :
+          for (const grpc_error_handle& discovery_mechanism_error :
                discovery_mechanism_errors) {
             error = grpc_error_add_child(error, discovery_mechanism_error);
           }

src/core/ext/transport/chttp2/server/chttp2_server.cc

@@ -596,6 +596,7 @@ grpc_error_handle Chttp2ServerListener::Create(
   // The bulk of this method is inside of a lambda to make cleanup
   // easier without using goto.
   grpc_error_handle error = [&]() {
+    grpc_error_handle error = GRPC_ERROR_NONE;
     // Create Chttp2ServerListener.
     listener = new Chttp2ServerListener(
         server, args, args_modifier,
@@ -861,6 +862,7 @@ grpc_error_handle Chttp2ServerAddPort(Server* server, const char* addr,
   std::vector<grpc_error_handle> error_list;
   // Using lambda to avoid use of goto.
   grpc_error_handle error = [&]() {
+    grpc_error_handle error = GRPC_ERROR_NONE;
     if (absl::StartsWith(addr, kUnixUriPrefix)) {
       error = grpc_resolve_unix_domain_address(
           addr + sizeof(kUnixUriPrefix) - 1, &resolved);

src/core/ext/transport/cronet/transport/cronet_transport.cc

@@ -1330,7 +1330,7 @@ static enum e_op_result execute_stream_op(struct op_and_state* oas) {
       result = ACTION_TAKEN_NO_CALLBACK;
     }
     stream_state->state_op_done[OP_CANCEL_ERROR] = true;
-    if (!stream_state->cancel_error) {
+    if (stream_state->cancel_error == GRPC_ERROR_NONE) {
       stream_state->cancel_error =
           GRPC_ERROR_REF(stream_op->payload->cancel_stream.cancel_error);
     }

src/core/lib/iomgr/combiner.cc

@@ -311,9 +311,10 @@ static void combiner_finally_exec(grpc_core::Combiner* lock,
 
 static void enqueue_finally(void* closure, grpc_error_handle error) {
   grpc_closure* cl = static_cast<grpc_closure*>(closure);
-  combiner_finally_exec(
+  grpc_core::Combiner* lock =
-      reinterpret_cast<grpc_core::Combiner*>(cl->error_data.scratch), cl,
+      reinterpret_cast<grpc_core::Combiner*>(cl->error_data.scratch);
-      GRPC_ERROR_REF(error));
+  cl->error_data.scratch = 0;
+  combiner_finally_exec(lock, cl, GRPC_ERROR_REF(error));
 }
 
 namespace grpc_core {

src/core/lib/iomgr/endpoint_cfstream.cc

@@ -149,7 +149,7 @@ static void CallWriteCb(CFStreamEndpoint* ep, grpc_error_handle error) {
 static void ReadAction(void* arg, grpc_error_handle error) {
   CFStreamEndpoint* ep = static_cast<CFStreamEndpoint*>(arg);
   GPR_ASSERT(ep->read_cb != nullptr);
-  if (error) {
+  if (error != GRPC_ERROR_NONE) {
     grpc_slice_buffer_reset_and_unref_internal(ep->read_slices);
     CallReadCb(ep, GRPC_ERROR_REF(error));
     EP_UNREF(ep, "read");
@@ -191,7 +191,7 @@ static void ReadAction(void* arg, grpc_error_handle error) {
 static void WriteAction(void* arg, grpc_error_handle error) {
   CFStreamEndpoint* ep = static_cast<CFStreamEndpoint*>(arg);
   GPR_ASSERT(ep->write_cb != nullptr);
-  if (error) {
+  if (error != GRPC_ERROR_NONE) {
     grpc_slice_buffer_reset_and_unref_internal(ep->write_slices);
     CallWriteCb(ep, GRPC_ERROR_REF(error));
     EP_UNREF(ep, "write");
@@ -286,13 +286,15 @@ static void CFStreamWrite(grpc_endpoint* ep, grpc_slice_buffer* slices,
 void CFStreamShutdown(grpc_endpoint* ep, grpc_error_handle why) {
   CFStreamEndpoint* ep_impl = reinterpret_cast<CFStreamEndpoint*>(ep);
   if (grpc_tcp_trace.enabled()) {
-    gpr_log(GPR_DEBUG, "CFStream endpoint:%p shutdown (%p)", ep_impl, why);
+    gpr_log(GPR_DEBUG, "CFStream endpoint:%p shutdown (%s)", ep_impl,
+            grpc_error_std_string(why).c_str());
   }
   CFReadStreamClose(ep_impl->read_stream);
   CFWriteStreamClose(ep_impl->write_stream);
   ep_impl->stream_sync->Shutdown(why);
   if (grpc_tcp_trace.enabled()) {
-    gpr_log(GPR_DEBUG, "CFStream endpoint:%p shutdown DONE (%p)", ep_impl, why);
+    gpr_log(GPR_DEBUG, "CFStream endpoint:%p shutdown DONE (%s)", ep_impl,
+            grpc_error_std_string(why).c_str());
   }
 }
 

src/core/lib/iomgr/error.cc

@@ -90,11 +90,17 @@ absl::Status grpc_wsa_error(const grpc_core::DebugLocation& location, int err,
   StatusSetInt(&s, grpc_core::StatusIntProperty::kWsaError, err);
   StatusSetStr(&s, grpc_core::StatusStrProperty::kOsError, utf8_message);
   StatusSetStr(&s, grpc_core::StatusStrProperty::kSyscall, call_name);
+  return s;
 }
 #endif
 
 grpc_error_handle grpc_error_set_int(grpc_error_handle src,
                                      grpc_error_ints which, intptr_t value) {
+  if (src == GRPC_ERROR_NONE) {
+    src = absl::UnknownError("");
+    StatusSetInt(&src, grpc_core::StatusIntProperty::kRpcStatus,
+                 GRPC_STATUS_OK);
+  }
   grpc_core::StatusSetInt(
       &src, static_cast<grpc_core::StatusIntProperty>(which), value);
   return src;
@@ -131,13 +137,40 @@ bool grpc_error_get_int(grpc_error_handle error, grpc_error_ints which,
 grpc_error_handle grpc_error_set_str(grpc_error_handle src,
                                      grpc_error_strs which,
                                      absl::string_view str) {
+  if (src == GRPC_ERROR_NONE) {
+    src = absl::UnknownError("");
+    StatusSetInt(&src, grpc_core::StatusIntProperty::kRpcStatus,
+                 GRPC_STATUS_OK);
+  }
+  if (which == GRPC_ERROR_STR_DESCRIPTION) {
+    // To change the message of absl::Status, a new instance should be created
+    // with a code and payload because it doesn't have a setter for it.
+    absl::Status s = absl::Status(src.code(), str);
+    src.ForEachPayload(
+        [&](absl::string_view type_url, const absl::Cord& payload) {
+          s.SetPayload(type_url, payload);
+        });
+    return s;
+  } else {
     grpc_core::StatusSetStr(
         &src, static_cast<grpc_core::StatusStrProperty>(which), str);
+  }
   return src;
 }
 
 bool grpc_error_get_str(grpc_error_handle error, grpc_error_strs which,
                         std::string* s) {
+  if (which == GRPC_ERROR_STR_DESCRIPTION) {
+    // absl::Status uses the message field for GRPC_ERROR_STR_DESCRIPTION
+    // instead of using payload.
+    absl::string_view msg = error.message();
+    if (msg.empty()) {
+      return false;
+    } else {
+      *s = std::string(msg);
+      return true;
+    }
+  } else {
     absl::optional<std::string> value = grpc_core::StatusGetStr(
         error, static_cast<grpc_core::StatusStrProperty>(which));
     if (value.has_value()) {
@@ -145,18 +178,36 @@ bool grpc_error_get_str(grpc_error_handle error, grpc_error_strs which,
       return true;
     } else {
       // TODO(veblush): Remove this once absl::Status migration is done
-    if (which == GRPC_ERROR_STR_DESCRIPTION && !error.message().empty()) {
+      if (which == GRPC_ERROR_STR_GRPC_MESSAGE) {
-      *s = std::move(*value);
+        switch (error.code()) {
+          case absl::StatusCode::kOk:
+            *s = "";
             return true;
+          case absl::StatusCode::kResourceExhausted:
+            *s = "RESOURCE_EXHAUSTED";
+            return true;
+          case absl::StatusCode::kCancelled:
+            *s = "CANCELLED";
+            return true;
+          default:
+            break;
+        }
       }
       return false;
     }
+  }
 }
 
 grpc_error_handle grpc_error_add_child(grpc_error_handle src,
                                        grpc_error_handle child) {
+  if (src.ok()) {
+    return child;
+  } else {
+    if (!child.ok()) {
       grpc_core::StatusAddChild(&src, child);
+    }
     return src;
+  }
 }
 
 bool grpc_log_error(const char* what, grpc_error_handle error, const char* file,

src/core/lib/iomgr/error.h

@@ -161,7 +161,7 @@ void grpc_enable_error_creation();
 #define GRPC_ERROR_CANCELLED absl::CancelledError()
 
 #define GRPC_ERROR_REF(err) (err)
-#define GRPC_ERROR_UNREF(err)
+#define GRPC_ERROR_UNREF(err) (void)(err)
 
 #define GRPC_ERROR_CREATE_FROM_STATIC_STRING(desc) \
   StatusCreate(absl::StatusCode::kUnknown, desc, DEBUG_LOCATION, {})

src/core/lib/iomgr/error_cfstream.cc

@@ -48,7 +48,12 @@ grpc_error_handle grpc_error_create_from_cferror(const char* file, int line,
       absl::StrFormat("%s (error domain:%s, code:%ld, description:%s)",
                       custom_desc, buf_domain, code, buf_desc);
   CFRelease(desc);
+#ifdef GRPC_ERROR_IS_ABSEIL_STATUS
+  return StatusCreate(absl::StatusCode::kUnknown, error_msg,
+                      grpc_core::DebugLocation(file, line), {});
+#else
   return grpc_error_create(
       file, line, grpc_slice_from_copied_string(error_msg.c_str()), NULL, 0);
+#endif
 }
 #endif /* GRPC_CFSTREAM */

src/core/lib/iomgr/ev_poll_posix.cc

@@ -357,7 +357,12 @@ static void unref_by(grpc_fd* fd, int n) {
     gpr_mu_destroy(&fd->mu);
     grpc_iomgr_unregister_object(&fd->iomgr_object);
     fork_fd_list_remove_node(fd->fork_fd_list);
-    if (fd->shutdown) GRPC_ERROR_UNREF(fd->shutdown_error);
+    if (fd->shutdown) {
+      GRPC_ERROR_UNREF(fd->shutdown_error);
+    }
+#ifdef GRPC_ERROR_IS_ABSEIL_STATUS
+    fd->shutdown_error.~Status();
+#endif
     gpr_free(fd);
   } else {
     GPR_ASSERT(old > n);
@@ -372,6 +377,9 @@ static grpc_fd* fd_create(int fd, const char* name, bool track_err) {
   gpr_mu_init(&r->mu);
   gpr_atm_rel_store(&r->refst, 1);
   r->shutdown = 0;
+#ifdef GRPC_ERROR_IS_ABSEIL_STATUS
+  new (&r->shutdown_error) absl::Status();
+#endif
   r->read_closure = CLOSURE_NOT_READY;
   r->write_closure = CLOSURE_NOT_READY;
   r->fd = fd;

src/core/lib/iomgr/tcp_client_cfstream.cc

@@ -81,7 +81,8 @@ static void CFStreamConnectCleanup(CFStreamConnect* connect) {
 static void OnAlarm(void* arg, grpc_error_handle error) {
   CFStreamConnect* connect = static_cast<CFStreamConnect*>(arg);
   if (grpc_tcp_trace.enabled()) {
-    gpr_log(GPR_DEBUG, "CLIENT_CONNECT :%p OnAlarm, error:%p", connect, error);
+    gpr_log(GPR_DEBUG, "CLIENT_CONNECT :%p OnAlarm, error:%s", connect,
+            grpc_error_std_string(error).c_str());
   }
   gpr_mu_lock(&connect->mu);
   grpc_closure* closure = connect->closure;
@@ -102,7 +103,8 @@ static void OnAlarm(void* arg, grpc_error_handle error) {
 static void OnOpen(void* arg, grpc_error_handle error) {
   CFStreamConnect* connect = static_cast<CFStreamConnect*>(arg);
   if (grpc_tcp_trace.enabled()) {
-    gpr_log(GPR_DEBUG, "CLIENT_CONNECT :%p OnOpen, error:%p", connect, error);
+    gpr_log(GPR_DEBUG, "CLIENT_CONNECT :%p OnOpen, error:%s", connect,
+            grpc_error_std_string(error).c_str());
   }
   gpr_mu_lock(&connect->mu);
   grpc_timer_cancel(&connect->alarm);

src/core/lib/iomgr/tcp_server_posix.cc

@@ -236,7 +236,7 @@ static void on_read(void* arg, grpc_error_handle err) {
       }
     }
 
-    grpc_set_socket_no_sigpipe_if_possible(fd);
+    (void)grpc_set_socket_no_sigpipe_if_possible(fd);
 
     err = grpc_apply_socket_mutator_in_args(fd, GRPC_FD_SERVER_CONNECTION_USAGE,
                                             sp->server->channel_args);
@@ -595,7 +595,7 @@ class ExternalConnectionHandler : public grpc_core::TcpServerFdHandler {
       close(fd);
       return;
     }
-    grpc_set_socket_no_sigpipe_if_possible(fd);
+    (void)grpc_set_socket_no_sigpipe_if_possible(fd);
     std::string addr_str = grpc_sockaddr_to_uri(&addr);
     if (grpc_tcp_trace.enabled()) {
       gpr_log(GPR_INFO, "SERVER_CONNECT: incoming external connection: %s",

src/core/lib/transport/error_utils.cc

@@ -36,7 +36,7 @@ static grpc_error_handle recursively_find_error_with_field(
   }
 #ifdef GRPC_ERROR_IS_ABSEIL_STATUS
   std::vector<absl::Status> children = grpc_core::StatusGetChildren(error);
-  for (auto child : children) {
+  for (const absl::Status& child : children) {
     grpc_error_handle result = recursively_find_error_with_field(child, which);
     if (result != GRPC_ERROR_NONE) return result;
   }
@@ -103,6 +103,10 @@ void grpc_error_get_status(grpc_error_handle error, grpc_millis deadline,
                                 &integer)) {
     status = grpc_http2_error_to_grpc_status(
         static_cast<grpc_http2_error_code>(integer), deadline);
+  } else {
+#ifdef GRPC_ERROR_IS_ABSEIL_STATUS
+    status = static_cast<grpc_status_code>(found_error.code());
+#endif
   }
   if (code != nullptr) *code = status;
 
@@ -131,7 +135,7 @@ void grpc_error_get_status(grpc_error_handle error, grpc_millis deadline,
       if (!grpc_error_get_str(found_error, GRPC_ERROR_STR_DESCRIPTION,
                               message)) {
 #ifdef GRPC_ERROR_IS_ABSEIL_STATUS
-        *message = grpc_error_std_string(error));
+        *message = grpc_error_std_string(error);
 #else
         *message = "unknown error";
 #endif
@@ -167,7 +171,7 @@ bool grpc_error_has_clear_grpc_status(grpc_error_handle error) {
   }
 #ifdef GRPC_ERROR_IS_ABSEIL_STATUS
   std::vector<absl::Status> children = grpc_core::StatusGetChildren(error);
-  for (auto child : children) {
+  for (const absl::Status& child : children) {
     if (grpc_error_has_clear_grpc_status(child)) {
       return true;
     }

src/cpp/client/secure_credentials.cc

@@ -217,6 +217,7 @@ grpc::Status StsCredentialsOptionsFromEnv(StsCredentialsOptions* options) {
   char* sts_creds_path = gpr_getenv("STS_CREDENTIALS");
   grpc_error_handle error = GRPC_ERROR_NONE;
   grpc::Status status;
+  // NOLINTNEXTLINE(clang-diagnostic-unused-lambda-capture)
   auto cleanup = [&json_string, &sts_creds_path, &error, &status]() {
     grpc_slice_unref_internal(json_string);
     gpr_free(sts_creds_path);

test/core/end2end/fuzzers/api_fuzzer.cc

@@ -79,7 +79,7 @@ typedef struct addr_req {
   std::unique_ptr<grpc_core::ServerAddressList>* addresses;
 } addr_req;
 
-static void finish_resolve(void* arg, grpc_error* error) {
+static void finish_resolve(void* arg, grpc_error_handle error) {
   addr_req* r = static_cast<addr_req*>(arg);
 
   if (error == GRPC_ERROR_NONE && 0 == strcmp(r->addr, "server")) {
@@ -162,7 +162,7 @@ typedef struct {
   grpc_slice_allocator* slice_allocator;
 } future_connect;
 
-static void do_connect(void* arg, grpc_error* error) {
+static void do_connect(void* arg, grpc_error_handle error) {
   future_connect* fc = static_cast<future_connect*>(arg);
   if (error != GRPC_ERROR_NONE) {
     grpc_slice_allocator_destroy(fc->slice_allocator);

test/core/end2end/inproc_callback_test.cc

@@ -414,7 +414,6 @@ static void simple_request_body(grpc_end2end_test_config config,
   // not likely to change much. Some parts of the error, like time created,
   // obviously are not checked.
   GPR_ASSERT(nullptr != strstr(error_string, "xyz"));
-  GPR_ASSERT(nullptr != strstr(error_string, "description"));
   GPR_ASSERT(nullptr != strstr(error_string, "Error received from peer"));
   GPR_ASSERT(nullptr != strstr(error_string, "grpc_message"));
   GPR_ASSERT(nullptr != strstr(error_string, "grpc_status"));

test/core/end2end/tests/cancel_test_helpers.h

@@ -34,7 +34,7 @@ static grpc_call_error wait_for_deadline(grpc_call* /*call*/, void* reserved) {
 }
 
 static const cancellation_mode cancellation_modes[] = {
-    {"cancel", grpc_call_cancel, GRPC_STATUS_CANCELLED, "Cancelled"},
+    {"cancel", grpc_call_cancel, GRPC_STATUS_CANCELLED, "CANCELLED"},
     {"deadline", wait_for_deadline, GRPC_STATUS_DEADLINE_EXCEEDED,
      "Deadline Exceeded"},
 };

test/core/end2end/tests/simple_request.cc

@@ -219,7 +219,6 @@ static void simple_request_body(grpc_end2end_test_config config,
   // not likely to change much. Some parts of the error, like time created,
   // obviously are not checked.
   GPR_ASSERT(nullptr != strstr(error_string, "xyz"));
-  GPR_ASSERT(nullptr != strstr(error_string, "description"));
   GPR_ASSERT(nullptr != strstr(error_string, "Error received from peer"));
   GPR_ASSERT(nullptr != strstr(error_string, "grpc_message"));
   GPR_ASSERT(nullptr != strstr(error_string, "grpc_status"));

test/core/iomgr/error_test.cc

@@ -32,8 +32,11 @@ TEST(ErrorTest, SetGetInt) {
   grpc_error_handle error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("Test");
   EXPECT_NE(error, GRPC_ERROR_NONE);
   intptr_t i = 0;
+#ifndef NDEBUG
+  // GRPC_ERROR_INT_FILE_LINE is for debug only
   EXPECT_TRUE(grpc_error_get_int(error, GRPC_ERROR_INT_FILE_LINE, &i));
   EXPECT_TRUE(i);  // line set will never be 0
+#endif
   EXPECT_TRUE(!grpc_error_get_int(error, GRPC_ERROR_INT_ERRNO, &i));
   EXPECT_TRUE(!grpc_error_get_int(error, GRPC_ERROR_INT_SIZE, &i));
 
@@ -56,13 +59,14 @@ TEST(ErrorTest, SetGetStr) {
   std::string str;
   EXPECT_TRUE(!grpc_error_get_str(error, GRPC_ERROR_STR_SYSCALL, &str));
   EXPECT_TRUE(!grpc_error_get_str(error, GRPC_ERROR_STR_TSI_ERROR, &str));
-
+#ifndef NDEBUG
+  // GRPC_ERROR_STR_FILE is for debug only
   EXPECT_TRUE(grpc_error_get_str(error, GRPC_ERROR_STR_FILE, &str));
   EXPECT_THAT(str, testing::HasSubstr("error_test.c"));
   // __FILE__ expands differently on
   // Windows. All should at least
   // contain error_test.c
-
+#endif
   EXPECT_TRUE(grpc_error_get_str(error, GRPC_ERROR_STR_DESCRIPTION, &str));
   EXPECT_EQ(str, "Test");
 

test/core/resource_quota/memory_quota_fuzzer.cc

@@ -113,7 +113,7 @@ class Fuzzer {
               };
               auto* args = new Args{std::move(sweep), cfg.msg(), this};
               auto* closure = GRPC_CLOSURE_CREATE(
-                  [](void* arg, grpc_error*) {
+                  [](void* arg, grpc_error_handle) {
                     auto* args = static_cast<Args*>(arg);
                     args->fuzzer->RunMsg(args->msg);
                     delete args;

test/cpp/end2end/end2end_test.cc

@@ -1463,8 +1463,11 @@ TEST_P(End2endTest, ExpectErrorTest) {
     EXPECT_EQ(iter->error_message(), s.error_message());
     EXPECT_EQ(iter->binary_error_details(), s.error_details());
     EXPECT_TRUE(absl::StrContains(context.debug_error_string(), "created"));
+#ifndef NDEBUG
+    // GRPC_ERROR_INT_FILE_LINE is for debug only
     EXPECT_TRUE(absl::StrContains(context.debug_error_string(), "file"));
     EXPECT_TRUE(absl::StrContains(context.debug_error_string(), "line"));
+#endif
     EXPECT_TRUE(absl::StrContains(context.debug_error_string(), "status"));
     EXPECT_TRUE(absl::StrContains(context.debug_error_string(), "13"));
   }