Aspnetcore: SignalR HubException does not send innerException property

[Efreeto]

Describe the bug

HubException class has a constructor with innerException, but providing it does nothing, and client only sees message.

To Reproduce

Steps to reproduce the behavior:

1. Using this version of ASP.NET Core 2.2
2. It doesn't matter whether EnableDetailedErrors option is true or false
3. C# Server: throw new HubException("I am error", new DivideByZeroException("infinitely"));
4. JS Client: Receives an error, which only has message and stack

Expected behavior

1. JS Client: Receives an error, which has message, stack, innerException.

Additional context

I've seen many discussions from code owners saying "We've considered customized objects before, but we only send messages now", so maybe this is part of that decision. So is innerException just there as a noop, or can I send it somehow? Actually I want to send any custom object as an error.

message alone is not enough for me. Even though I own both server and client, I want to serve different cultures

For example,

1. JS Client 1: Does something bad.
2. C# Server: throw new MyException(123, 'A', 'B');
3. JS Client 1: Receives error -> Looks up error code 123 for Language 1 -> Tells user "'A' is not found in 'B'"

But I also want to do,

1. JS Client 2: Does something equally bad.
2. C# Server: throw new MyException(123, 'A', 'B'); // same as above
3. JS Client 2: Receives error -> Looks up error code 123 for Language 2 -> Tells user "'B' does not contain 'A'... desu"

[Efreeto]

Let me know if you want me to file the part in Additional context as a seperate feature request.

[anurse]

This is by design since the protocol is explicitly decoupled from the implementation. Either of the client or server could be sending messages from non-.NET applications (Java Client, Azure Functions server, etc.) so mapping things beyond the message is challenging. Our current design is to strongly advise that HubException be for truely exceptional circumstances that the client can't react to and should only report the issue. If you need to return structured error data, this is better suited to a return value with structured results indicating the error details.

Returning the stack trace is definitely not in our plan as there are security risks to exposing stack traces.

[Efreeto]

Thank you for your explanation. But I do not understand the decision behind
the design. We have a server and a client closely linked, and all that
client has is UI while the server handles all the requests, and some are
quite heavy. Would it be truly exceptional circumstances if a server throws
an error that a client using user should can know about and react? We have
already considered returning structured results with error, but it felt
hacky before you also recommended this approach. (Other approaches we
considered are serializing all error details into the message string, and
using another hub method exclusively for sending errors, but all feels very
hacky.)
Also, I think whether to return the stack trace should be a choice for the
developers. I understand the danger as the old SignalR document states (
https://docs.microsoft.com/en-us/aspnet/signalr/overview/guide-to-the-api/hubs-api-guide-server#how-to-handle-errors-in-the-hub-class
),
it shouldn't be exposed for 'production' build for reasons, but this will
be very useful for debug / dev build.

On Wed, Jul 31, 2019 at 2:50 PM Andrew Stanton-Nurse <
[email protected]> wrote:

This is by design since the protocol is explicitly decoupled from the
implementation. Either of the client or server could be sending messages
from non-.NET applications (Java Client, Azure Functions server, etc.) so
mapping things beyond the message is challenging. Our current design is to
strongly advise that HubException be for truely exceptional circumstances
that the client can't react to and should only report the issue. If you
need to return structured error data, this is better suited to a return
value with structured results indicating the error details.

Returning the stack trace is definitely not in our plan as there are
security risks to exposing stack traces.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/aspnet/AspNetCore/issues/12633?email_source=notifications&email_token=ABAJHHXY24LIHCLH3CMD5BLQCICK5A5CNFSM4IHHQSI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3IVJTA#issuecomment-517035212,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABAJHHWI3PPRIDPNKTNX4VDQCICK5ANCNFSM4IHHQSIQ
.

[anurse]

Would it be truly exceptional circumstances if a server throws an error that a client using user should can know about and react?

Of course this is a design choice but in general the answer is no, this isn't an exceptional condition. In general, if the caller is expected to be able to react and recover, an Exception shouldn't be used. The .NET Best Practices for exceptions specifically try to avoid using Exceptions for cases the caller should be able to handle (https://docs.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions#handle-common-conditions-without-throwing-exceptions).

We're also looking at adding the "pipeline" concept described in that doc (https://github.com/aspnet/AspNetCore/issues/5353) which will help with error handling and management. You wouldn't have to wrap every method in a try..catch for example, and would have some degree of customization over how the error is rendered.

We don't plan to serialize the entire exception though, to be clear. We can't do so in a way that guarantees the client will be able to deserialize it properly. While your application may be a tightly coupled .NET Server and .NET Client, this isn't something SignalR can expect in general. Adding things like the Hub Pipeline will enable you more control over this when you do have a tightly coupled app, but SignalR shouldn't be doing it in the general case.

[Efreeto]

Thank you so much for your explanation again Andrew. The design decision became more clear to me, and it will prepare us as we consider other methods to do this. We ultimately decided to send serialized error as a string.

By the way, the "pipeline" concept is a very welcome feature that was removed in the new version. For us, to at least remove the string "An error occurred on the server while streaming results.", I'd need to send the message with HubConnectionContext and invocation id. So I overrode DefaultHubDispatcher like this...:

    internal class MyHubDispatcher<T> : DefaultHubDispatcher<T>
    {
        public override Task DispatchMessageAsync(HubConnectionContext connection, HubMessage hubMessage)
        {
            if (hubMessage is StreamInvocationMessage streamInvocationMessage)
            {
                var args = streamInvocationMessage.Arguments;    // client sends two dummy args
                args[args.Length - 1] = streamInvocationMessage.InvocationId;
                args[args.Length - 2] = connection;
                return base.DispatchMessageAsync(connection, new StreamInvocationMessage(streamInvocationMessage.InvocationId, streamInvocationMessage.Target, args));    // pass two args to invoked methods so they can send more customized messages
            }

            return base.DispatchMessageAsync(connection, hubMessage);
        }
    }

But it became a little ugly. I will be definitely looking forward to this new feature in 3.0. :)

[BrennanConroy]

@Efreeto Just an FYI, that feature will not be in 3.0. It hasn't been officially planned for a specific release yet.