From 4c303a387e271a7797eb9e9e68764b13dba9e6ef Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Fri, 5 Jun 2020 14:59:52 +0200 Subject: [PATCH] Revert "Revert "C# expose C-core's debug error string in Status struct"" This reverts commit 7c30f0c956720af8f6c748ada0f504e055e54d16. --- src/csharp/Grpc.Core.Api/Status.cs | 57 ++++++++++++------- .../Grpc.Core.Tests/ClientServerTest.cs | 20 +++++++ .../Internal/BatchContextSafeHandle.cs | 3 +- .../Internal/CoreErrorDetailException.cs | 35 ++++++++++++ .../Internal/NativeMethods.Generated.cs | 11 ++++ src/csharp/ext/grpc_csharp_ext.c | 17 ++++++ .../runtimes/grpc_csharp_ext_dummy_stubs.c | 4 ++ .../Grpc.Core/Internal/native_methods.include | 1 + 8 files changed, 127 insertions(+), 21 deletions(-) create mode 100644 src/csharp/Grpc.Core/Internal/CoreErrorDetailException.cs diff --git a/src/csharp/Grpc.Core.Api/Status.cs b/src/csharp/Grpc.Core.Api/Status.cs index b1a030b2d1f..c13f9d88133 100644 --- a/src/csharp/Grpc.Core.Api/Status.cs +++ b/src/csharp/Grpc.Core.Api/Status.cs @@ -14,6 +14,8 @@ // limitations under the License. #endregion +using System; + namespace Grpc.Core { /// @@ -31,48 +33,63 @@ namespace Grpc.Core /// public static readonly Status DefaultCancelled = new Status(StatusCode.Cancelled, ""); - readonly StatusCode statusCode; - readonly string detail; - /// /// Creates a new instance of Status. /// /// Status code. /// Detail. - public Status(StatusCode statusCode, string detail) + public Status(StatusCode statusCode, string detail) : this(statusCode, detail, null) { - this.statusCode = statusCode; - this.detail = detail; } /// - /// Gets the gRPC status code. OK indicates success, all other values indicate an error. + /// Creates a new instance of Status. + /// Users should not use this constructor, except for creating instances for testing. + /// The debug error string should only be populated by gRPC internals. + /// Note: experimental API that can change or be removed without any prior notice. /// - public StatusCode StatusCode + /// Status code. + /// Detail. + /// Optional internal error details. + public Status(StatusCode statusCode, string detail, Exception debugException) { - get - { - return statusCode; - } + StatusCode = statusCode; + Detail = detail; + DebugException = debugException; } + /// + /// Gets the gRPC status code. OK indicates success, all other values indicate an error. + /// + public StatusCode StatusCode { get; } + /// /// Gets the detail. /// - public string Detail - { - get - { - return detail; - } - } + public string Detail { get; } + + /// + /// In case of an error, this field may contain additional error details to help with debugging. + /// This field will be only populated on a client and its value is generated locally, + /// based on the internal state of the gRPC client stack (i.e. the value is never sent over the wire). + /// Note that this field is available only for debugging purposes, the application logic should + /// never rely on values of this field (it should use StatusCode and Detail instead). + /// Example: when a client fails to connect to a server, this field may provide additional details + /// why the connection to the server has failed. + /// Note: experimental API that can change or be removed without any prior notice. + /// + public Exception DebugException { get; } /// /// Returns a that represents the current . /// public override string ToString() { - return string.Format("Status(StatusCode={0}, Detail=\"{1}\")", statusCode, detail); + if (DebugException != null) + { + return $"Status(StatusCode=\"{StatusCode}\", Detail=\"{Detail}\", DebugException=\"{DebugException}\")"; + } + return $"Status(StatusCode=\"{StatusCode}\", Detail=\"{Detail}\")"; } } } diff --git a/src/csharp/Grpc.Core.Tests/ClientServerTest.cs b/src/csharp/Grpc.Core.Tests/ClientServerTest.cs index 331c3321e14..7ff639c7ca9 100644 --- a/src/csharp/Grpc.Core.Tests/ClientServerTest.cs +++ b/src/csharp/Grpc.Core.Tests/ClientServerTest.cs @@ -139,6 +139,26 @@ namespace Grpc.Core.Tests Assert.AreEqual(0, ex2.Trailers.Count); } + [Test] + public void UnaryCall_StatusDebugErrorStringNotTransmittedFromServer() + { + helper.UnaryHandler = new UnaryServerMethod((request, context) => + { + context.Status = new Status(StatusCode.Unauthenticated, "", new CoreErrorDetailException("this DebugErrorString value should not be transmitted to the client")); + return Task.FromResult(""); + }); + + var ex = Assert.Throws(() => Calls.BlockingUnaryCall(helper.CreateUnaryCall(), "abc")); + Assert.AreEqual(StatusCode.Unauthenticated, ex.Status.StatusCode); + StringAssert.Contains("Error received from peer", ex.Status.DebugException.Message, "Is \"Error received from peer\" still a valid substring to search for in the client-generated error message from C-core?"); + Assert.AreEqual(0, ex.Trailers.Count); + + var ex2 = Assert.ThrowsAsync(async () => await Calls.AsyncUnaryCall(helper.CreateUnaryCall(), "abc")); + Assert.AreEqual(StatusCode.Unauthenticated, ex2.Status.StatusCode); + StringAssert.Contains("Error received from peer", ex2.Status.DebugException.Message, "Is \"Error received from peer\" still a valid substring to search for in the client-generated error message from C-core?"); + Assert.AreEqual(0, ex2.Trailers.Count); + } + [Test] public void UnaryCall_ServerHandlerSetsStatusAndTrailers() { diff --git a/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs b/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs index 50a626842dd..025f93e86c8 100644 --- a/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs +++ b/src/csharp/Grpc.Core/Internal/BatchContextSafeHandle.cs @@ -92,7 +92,8 @@ namespace Grpc.Core.Internal UIntPtr detailsLength; IntPtr detailsPtr = Native.grpcsharp_batch_context_recv_status_on_client_details(this, out detailsLength); string details = MarshalUtils.PtrToStringUTF8(detailsPtr, (int)detailsLength.ToUInt32()); - var status = new Status(Native.grpcsharp_batch_context_recv_status_on_client_status(this), details); + string debugErrorString = Marshal.PtrToStringAnsi(Native.grpcsharp_batch_context_recv_status_on_client_error_string(this)); + var status = new Status(Native.grpcsharp_batch_context_recv_status_on_client_status(this), details, debugErrorString != null ? new CoreErrorDetailException(debugErrorString) : null); IntPtr metadataArrayPtr = Native.grpcsharp_batch_context_recv_status_on_client_trailing_metadata(this); var metadata = MetadataArraySafeHandle.ReadMetadataFromPtrUnsafe(metadataArrayPtr); diff --git a/src/csharp/Grpc.Core/Internal/CoreErrorDetailException.cs b/src/csharp/Grpc.Core/Internal/CoreErrorDetailException.cs new file mode 100644 index 00000000000..ca688648138 --- /dev/null +++ b/src/csharp/Grpc.Core/Internal/CoreErrorDetailException.cs @@ -0,0 +1,35 @@ +#region Copyright notice and license + +// Copyright 2019 The gRPC Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#endregion + +using System; +using System.Runtime.InteropServices; +using System.Threading; +using Grpc.Core.Utils; + +namespace Grpc.Core.Internal +{ + /// + /// Represents error details provides by C-core's debug_error_string + /// + internal class CoreErrorDetailException : Exception + { + public CoreErrorDetailException(string message) : base(message) + { + } + } +} diff --git a/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs b/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs index c724b30ca8f..8b60268679d 100644 --- a/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs +++ b/src/csharp/Grpc.Core/Internal/NativeMethods.Generated.cs @@ -43,6 +43,7 @@ namespace Grpc.Core.Internal public readonly Delegates.grpcsharp_batch_context_recv_message_next_slice_peek_delegate grpcsharp_batch_context_recv_message_next_slice_peek; public readonly Delegates.grpcsharp_batch_context_recv_status_on_client_status_delegate grpcsharp_batch_context_recv_status_on_client_status; public readonly Delegates.grpcsharp_batch_context_recv_status_on_client_details_delegate grpcsharp_batch_context_recv_status_on_client_details; + public readonly Delegates.grpcsharp_batch_context_recv_status_on_client_error_string_delegate grpcsharp_batch_context_recv_status_on_client_error_string; public readonly Delegates.grpcsharp_batch_context_recv_status_on_client_trailing_metadata_delegate grpcsharp_batch_context_recv_status_on_client_trailing_metadata; public readonly Delegates.grpcsharp_batch_context_recv_close_on_server_cancelled_delegate grpcsharp_batch_context_recv_close_on_server_cancelled; public readonly Delegates.grpcsharp_batch_context_reset_delegate grpcsharp_batch_context_reset; @@ -151,6 +152,7 @@ namespace Grpc.Core.Internal this.grpcsharp_batch_context_recv_message_next_slice_peek = GetMethodDelegate(library); this.grpcsharp_batch_context_recv_status_on_client_status = GetMethodDelegate(library); this.grpcsharp_batch_context_recv_status_on_client_details = GetMethodDelegate(library); + this.grpcsharp_batch_context_recv_status_on_client_error_string = GetMethodDelegate(library); this.grpcsharp_batch_context_recv_status_on_client_trailing_metadata = GetMethodDelegate(library); this.grpcsharp_batch_context_recv_close_on_server_cancelled = GetMethodDelegate(library); this.grpcsharp_batch_context_reset = GetMethodDelegate(library); @@ -258,6 +260,7 @@ namespace Grpc.Core.Internal this.grpcsharp_batch_context_recv_message_next_slice_peek = DllImportsFromStaticLib.grpcsharp_batch_context_recv_message_next_slice_peek; this.grpcsharp_batch_context_recv_status_on_client_status = DllImportsFromStaticLib.grpcsharp_batch_context_recv_status_on_client_status; this.grpcsharp_batch_context_recv_status_on_client_details = DllImportsFromStaticLib.grpcsharp_batch_context_recv_status_on_client_details; + this.grpcsharp_batch_context_recv_status_on_client_error_string = DllImportsFromStaticLib.grpcsharp_batch_context_recv_status_on_client_error_string; this.grpcsharp_batch_context_recv_status_on_client_trailing_metadata = DllImportsFromStaticLib.grpcsharp_batch_context_recv_status_on_client_trailing_metadata; this.grpcsharp_batch_context_recv_close_on_server_cancelled = DllImportsFromStaticLib.grpcsharp_batch_context_recv_close_on_server_cancelled; this.grpcsharp_batch_context_reset = DllImportsFromStaticLib.grpcsharp_batch_context_reset; @@ -365,6 +368,7 @@ namespace Grpc.Core.Internal this.grpcsharp_batch_context_recv_message_next_slice_peek = DllImportsFromSharedLib.grpcsharp_batch_context_recv_message_next_slice_peek; this.grpcsharp_batch_context_recv_status_on_client_status = DllImportsFromSharedLib.grpcsharp_batch_context_recv_status_on_client_status; this.grpcsharp_batch_context_recv_status_on_client_details = DllImportsFromSharedLib.grpcsharp_batch_context_recv_status_on_client_details; + this.grpcsharp_batch_context_recv_status_on_client_error_string = DllImportsFromSharedLib.grpcsharp_batch_context_recv_status_on_client_error_string; this.grpcsharp_batch_context_recv_status_on_client_trailing_metadata = DllImportsFromSharedLib.grpcsharp_batch_context_recv_status_on_client_trailing_metadata; this.grpcsharp_batch_context_recv_close_on_server_cancelled = DllImportsFromSharedLib.grpcsharp_batch_context_recv_close_on_server_cancelled; this.grpcsharp_batch_context_reset = DllImportsFromSharedLib.grpcsharp_batch_context_reset; @@ -475,6 +479,7 @@ namespace Grpc.Core.Internal public delegate int grpcsharp_batch_context_recv_message_next_slice_peek_delegate(BatchContextSafeHandle ctx, out UIntPtr sliceLen, out IntPtr sliceDataPtr); public delegate StatusCode grpcsharp_batch_context_recv_status_on_client_status_delegate(BatchContextSafeHandle ctx); public delegate IntPtr grpcsharp_batch_context_recv_status_on_client_details_delegate(BatchContextSafeHandle ctx, out UIntPtr detailsLength); + public delegate IntPtr grpcsharp_batch_context_recv_status_on_client_error_string_delegate(BatchContextSafeHandle ctx); public delegate IntPtr grpcsharp_batch_context_recv_status_on_client_trailing_metadata_delegate(BatchContextSafeHandle ctx); public delegate int grpcsharp_batch_context_recv_close_on_server_cancelled_delegate(BatchContextSafeHandle ctx); public delegate void grpcsharp_batch_context_reset_delegate(BatchContextSafeHandle ctx); @@ -605,6 +610,9 @@ namespace Grpc.Core.Internal [DllImport(ImportName)] public static extern IntPtr grpcsharp_batch_context_recv_status_on_client_details(BatchContextSafeHandle ctx, out UIntPtr detailsLength); + [DllImport(ImportName)] + public static extern IntPtr grpcsharp_batch_context_recv_status_on_client_error_string(BatchContextSafeHandle ctx); + [DllImport(ImportName)] public static extern IntPtr grpcsharp_batch_context_recv_status_on_client_trailing_metadata(BatchContextSafeHandle ctx); @@ -922,6 +930,9 @@ namespace Grpc.Core.Internal [DllImport(ImportName)] public static extern IntPtr grpcsharp_batch_context_recv_status_on_client_details(BatchContextSafeHandle ctx, out UIntPtr detailsLength); + [DllImport(ImportName)] + public static extern IntPtr grpcsharp_batch_context_recv_status_on_client_error_string(BatchContextSafeHandle ctx); + [DllImport(ImportName)] public static extern IntPtr grpcsharp_batch_context_recv_status_on_client_trailing_metadata(BatchContextSafeHandle ctx); diff --git a/src/csharp/ext/grpc_csharp_ext.c b/src/csharp/ext/grpc_csharp_ext.c index e09ad694328..a48d29af294 100644 --- a/src/csharp/ext/grpc_csharp_ext.c +++ b/src/csharp/ext/grpc_csharp_ext.c @@ -69,6 +69,7 @@ typedef struct grpcsharp_batch_context { grpc_metadata_array trailing_metadata; grpc_status_code status; grpc_slice status_details; + const char* error_string; } recv_status_on_client; int recv_close_on_server_cancelled; @@ -223,6 +224,7 @@ grpcsharp_batch_context_reset(grpcsharp_batch_context* ctx) { grpcsharp_metadata_array_destroy_metadata_only( &(ctx->recv_status_on_client.trailing_metadata)); grpc_slice_unref(ctx->recv_status_on_client.status_details); + gpr_free(ctx->recv_status_on_client.error_string); memset(ctx, 0, sizeof(grpcsharp_batch_context)); } @@ -328,6 +330,12 @@ grpcsharp_batch_context_recv_status_on_client_details( return (char*)GRPC_SLICE_START_PTR(ctx->recv_status_on_client.status_details); } +GPR_EXPORT const char* GPR_CALLTYPE +grpcsharp_batch_context_recv_status_on_client_error_string( + const grpcsharp_batch_context* ctx) { + return ctx->recv_status_on_client.error_string; +} + GPR_EXPORT const grpc_metadata_array* GPR_CALLTYPE grpcsharp_batch_context_recv_status_on_client_trailing_metadata( const grpcsharp_batch_context* ctx) { @@ -631,6 +639,8 @@ GPR_EXPORT grpc_call_error GPR_CALLTYPE grpcsharp_call_start_unary( &(ctx->recv_status_on_client.status); ops[5].data.recv_status_on_client.status_details = &(ctx->recv_status_on_client.status_details); + ops[5].data.recv_status_on_client.error_string = + &(ctx->recv_status_on_client.error_string); ops[5].flags = 0; ops[5].reserved = NULL; @@ -652,6 +662,7 @@ GPR_EXPORT grpc_call_error GPR_CALLTYPE grpcsharp_test_call_start_unary_echo( // received from server. ctx->recv_status_on_client.status = GRPC_STATUS_OK; ctx->recv_status_on_client.status_details = grpc_empty_slice(); + ctx->recv_status_on_client.error_string = NULL; // echo initial metadata as if received from server (as trailing metadata) grpcsharp_metadata_array_move(&(ctx->recv_status_on_client.trailing_metadata), initial_metadata); @@ -691,6 +702,8 @@ GPR_EXPORT grpc_call_error GPR_CALLTYPE grpcsharp_call_start_client_streaming( &(ctx->recv_status_on_client.status); ops[3].data.recv_status_on_client.status_details = &(ctx->recv_status_on_client.status_details); + ops[3].data.recv_status_on_client.error_string = + &(ctx->recv_status_on_client.error_string); ops[3].flags = 0; ops[3].reserved = NULL; @@ -732,6 +745,8 @@ GPR_EXPORT grpc_call_error GPR_CALLTYPE grpcsharp_call_start_server_streaming( &(ctx->recv_status_on_client.status); ops[3].data.recv_status_on_client.status_details = &(ctx->recv_status_on_client.status_details); + ops[3].data.recv_status_on_client.error_string = + &(ctx->recv_status_on_client.error_string); ops[3].flags = 0; ops[3].reserved = NULL; @@ -761,6 +776,8 @@ GPR_EXPORT grpc_call_error GPR_CALLTYPE grpcsharp_call_start_duplex_streaming( &(ctx->recv_status_on_client.status); ops[1].data.recv_status_on_client.status_details = &(ctx->recv_status_on_client.status_details); + ops[1].data.recv_status_on_client.error_string = + &(ctx->recv_status_on_client.error_string); ops[1].flags = 0; ops[1].reserved = NULL; diff --git a/src/csharp/unitypackage/unitypackage_skeleton/Plugins/Grpc.Core/runtimes/grpc_csharp_ext_dummy_stubs.c b/src/csharp/unitypackage/unitypackage_skeleton/Plugins/Grpc.Core/runtimes/grpc_csharp_ext_dummy_stubs.c index 58a7c58e91a..11097e613d8 100644 --- a/src/csharp/unitypackage/unitypackage_skeleton/Plugins/Grpc.Core/runtimes/grpc_csharp_ext_dummy_stubs.c +++ b/src/csharp/unitypackage/unitypackage_skeleton/Plugins/Grpc.Core/runtimes/grpc_csharp_ext_dummy_stubs.c @@ -58,6 +58,10 @@ void grpcsharp_batch_context_recv_status_on_client_details() { fprintf(stderr, "Should never reach here"); abort(); } +void grpcsharp_batch_context_recv_status_on_client_error_string() { + fprintf(stderr, "Should never reach here"); + abort(); +} void grpcsharp_batch_context_recv_status_on_client_trailing_metadata() { fprintf(stderr, "Should never reach here"); abort(); diff --git a/templates/src/csharp/Grpc.Core/Internal/native_methods.include b/templates/src/csharp/Grpc.Core/Internal/native_methods.include index f2b3a165a3b..e6fe0a2dd5e 100644 --- a/templates/src/csharp/Grpc.Core/Internal/native_methods.include +++ b/templates/src/csharp/Grpc.Core/Internal/native_methods.include @@ -9,6 +9,7 @@ native_method_signatures = [ 'int grpcsharp_batch_context_recv_message_next_slice_peek(BatchContextSafeHandle ctx, out UIntPtr sliceLen, out IntPtr sliceDataPtr)', 'StatusCode grpcsharp_batch_context_recv_status_on_client_status(BatchContextSafeHandle ctx)', 'IntPtr grpcsharp_batch_context_recv_status_on_client_details(BatchContextSafeHandle ctx, out UIntPtr detailsLength)', + 'IntPtr grpcsharp_batch_context_recv_status_on_client_error_string(BatchContextSafeHandle ctx)', 'IntPtr grpcsharp_batch_context_recv_status_on_client_trailing_metadata(BatchContextSafeHandle ctx)', 'int grpcsharp_batch_context_recv_close_on_server_cancelled(BatchContextSafeHandle ctx)', 'void grpcsharp_batch_context_reset(BatchContextSafeHandle ctx)',