Consider adding an error event handler for HubConnection

[AndersMalmgren]

This works

connection.On<object>("onEvent, obj => System.WriteLine("triggers");

This does not

connection.On<Message>("onEvent, obj => System.WriteLine("triggers");

It used fine in 2.2. Thanks, Anders

[AndersMalmgren]

Ops, I had a reference to Newton soft JToken in the Message type. I think its why it does not trigger. Why not throw error instead of doing nothing? I will rewrite to use System.Text.Json and get back here if it does not work

[AndersMalmgren]

Replaced JToken with JsonElement and it triggers. But please, do not silent swallow errors ;)

[AndersMalmgren]

Also JToken has a ToOjbect which System.Text.Json does not. In System.Text.Json I need todo

var json = message.Event.GetRawText();
return JsonSerializer.Deserialize(json, type, Options);

[BrennanConroy]

Why not throw error instead of doing nothing?

There isn't really anywhere we can throw an error unless we close the connection and provide the error in the closed handler.

We instead log when a users .on(...) handler either can't be found, or fails to run. You can enable logs fairly easily by following the docs https://docs.microsoft.com/aspnet/core/signalr/diagnostics?view=aspnetcore-3.0#net-client-logging

[AndersMalmgren]

Can't you throw when you receive the message and can't translate it to the requested type. Or even pass null to the event handler so we get a null reference in event handler.

When nothing happens it takes longer to debug :)

[dbacher]

@BrennanConroy @AndersMalmgren There should be an Action for general parse failures, and then if feasible, a separate one for each type matching the On handler.

When the client or server sends a message, the intent is a change of program state on the other side. In a simple case, it's add a line to a chat room. In a more complicated case, it's some transaction impacting a data element in persistent storage. In any case, the intent was to change state on the other side of the wire - not to have the message ignored. But the outcome is that the message was ignored.

This potentially impacts every subsequent state change for the lifespan of the process, which is probably a bad thing. And so there should be a notification, to the process, "hey this didn't work." It's an error case, so there shouldn't be any impact to whatever throughput requirements you have.

In particular, client side, with MVVM - if the updates are intended to change the View Model, you have UI elements in an inconsistent state and that is... we'll call it bad.

Server side, client told you to change some row in some database and the row never changed. But you didn't get back a success or a failure message, since the server just ignored your request.

I'd think there's never an outcome from having a notification method available that is worse than not having a notification available.

[analogrelay]

Can't you throw when you receive the message and can't translate it to the requested type.

Throw where? The dispatching is happening on a background thread. Where would you catch this exception? That would be no better than logging (which you could already capture with a custom sink).

If an error occurs while parsing, it's not recoverable, so we log it. If you want to customize serialization to better handle this kind of parse failure, that's something you should do in the serializer, since that's where the error occurs. SignalR is very intentionally abstracted away from that. For example, in the JSON serializer, you can customize the JsonSerializerOptions and register custom converters that give you much more control over serialization if you want to do this.

[dbacher]

@anurse Logging is an amplifier for denial of service.

If I connect and spool bad messages, I'll burn your CPU and whatever logging resources you're using, eventually your process will crash if network is greater than disk. Any logging based solution makes that worse, not better.

And so what we really want is a notification we can tie to a counter, so that after 10 tries or w/e, we can disconnect a peer. Also, in a professional app versus a 6 line chat demo for Build, we'd likely want to assemble a message and signal the peer "hey, something bad is happening here."

That allows the peer to cancel user interface, for example.

Anyway, the point is - replacing the serializer is not a discoverable error handling mechanism. Doing it that way is going to cause issues here, it's going to cause questions on stack overflow, and 100% of the people who go that route are going to complain about it.

And so maybe adding two unit tests - which is really likely what we're talking - and a zero cost delegate call on failures, which you expect to be rare when SignalR is working correctly in the absence of a hostile peer, so that apps can respond to the problem instead of just logging it somewhere might make a bit of sense?

Or is the argument that SignalR will never have an error processing messages that an app using it might want to respond to in some way other than log and continue?

[HeikoBoettger]

@dbacher

Logging is an amplifier for denial of service.

Not sure why this should be an argument against implementing a good logging system. As a developer I would only enable the logging while investigating a problem. There is a reason why there is a env.IsDevelopment() extension for IWebHostEnvironment.

Anyway having also used other frameworks as well, I would prefer adding some notification to do the logging by myself or whatever I like to do in such a case.

Is it also possible to notify that a member of class wasn't deserialized because it wasn´t contained in the json-message? I just violated the DRY-priciple while experimenting when deserializing a message on the client side and discovered that I mispelled a member-name (instead reusing the type via a shared library). Got no hint from the system about this. Would the notifcation also help to find such a problem?

[davidfowl]

We could call an OnUnhandledError handler that gets called on the client. It’s harder to do on the server depending on how much we actually parsed

[dbacher]

@davidfowl I would like to use SignalR on a US DOD system (cue "omg deathmonger and w/e comments"), and so I need to integrate it with systems written in 1995, that were an original cause for the Personnel folks even getting desktop computers, versus shuffling actual papers.

The attacks I'm trying to deal with run like this...

An attacker connects to a hub, and starts just spooling valid messages that route successfully as fast as they can. There is no circumstance in which the framework can deal with this, but since I am being notified every time a message successfully routes - I can take that on at the app level, and act on it if I need to. This is very app dependent.

An attacker connects to a hub and spools messages that don't deserialize successfully. Right now this is a silent failure case, and so an attacker can use it to burn resources. Adding any delegate based approach helps, however if you assume the attacker is spooling messages as fast as possible, the event pattern is likely to cause more allocation and garbage collection, and so it'd be smarter to just call a delegate and then let the user route that to a pub/sub pattern if they need multicast, since I'd expect that to be the exception not the rule (pun intended).

An attacker connects to a hub and sends messages the serializer does successfully deserialize, but which cannot be successfully routed. This could be intentional - we did it all the time in Gasper Vantage with our internal library, back when I worked for NCR (cue: "oh and you stole from poor people too" comments). It also could be a bug. Having a notification - again, any delegate based mechanism works but event might cause excessive allocations - would be helpful. In this case, since there is already garbage since we deserialized an object successfully, the event pattern wouldn't add much to garbage collection on paper. But "not adding much" might be relevant.

There wouldn't be any impact to code that doesn't want / need to know from having the event - but it'd help people dealing with the one bad person on all of Internet. ;)

With Gasper Vantage, we had to maintain a million message an hour target on 1995 era hardware. When I started, we had to do it on Windows 3.1 (the DOS product had a lower threshold). That was rough - real rough - with the entire operating system operating against us.

@HeikoBoettger You can have logging, in your production system - it's saved me more times than I can count. Focus it, be able to turn your instrumentation on and off in production without restarting your entire farm of containers.

However, be aware the disk can't be in two places at once, and if I'm trying to burn your resources, I can leverage your logging to potentially burn more resources. E.G. if I know you're writing entire messages to disk every time, I can make the messages as large as possible. Generally, the issue (and this goes for memory and network approaches as well) is that the disk has a finite capacity, and you can exhaust that and cause writes to start backing up.

If you look at the Windows event log, when Windows sees this - it eventually, in some subsystems, starts logging "the previous event repeats 100 times."

If you look at ASP.NET Health, it did the same thing - before it raised a health event it would wait some period of time, and if you needed it, you could have it batch them down to "this event repeats a thousand times" and get one notification instead of a thousand.

The issue, in both cases, is it is really hard to deal with a problem with a hub without a reference for the hub, and so once you're in the standard logging or event infrastructure, your receiving a messgae or event to log, and aren't able to easily get back to the ASP.NET connection or hub.

And so ASP.NET, nearly everywhere, has some error notification separate from health and logging.

I can't today, but if you ping me a couple times, I can provide a deserializer that requires all non-nullable (C# 8 rules) properties to be populated or raises a notification. If there's a field that explicitly states it can be null, I wouldn't enforce it being set (makes for smaller messages).

[ghost]

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[davidfowl]

This looks like a good issue to tackle. We need to figure out the scope, is it client or server or both. What types of errors are customers trying to handle. The main thing here seems to be malformed payloads that stop us from invoking your method. Assuming it's just the payload and not the outer framing, it should be possible on both server and client to have an error handler that gets called if dispatch fails altogether. It won't be called if an exception is thrown after dispatch, this is strictly when we can't never call your handler.

We also need to decide if we log when the unhandled error method gets called. Maybe we skip it assuming you might be doing logging in there an therefore it's handled.

[dbacher]

A simple multicast event would be sufficient from my perspective. At that point, I can use System.Reactive, LINQ, etc. to do the rest.

It'd be fine, from my perspective, to punt logging to the app if it's registered a handler.

Another way to structure it would be a reactive extensions point - but I don't know if that's a good dependency to take. But that'd allow roughly: myHub.Exceptions.Where(condition).Threshold(100, 1000).Each(reportingFunc);

I believe this needs to be supported on both the server and the client.

Argument for server: If I have a client or a group of clients connecting to me and sending me bad data, I am burning network and CPU. If I have authentication - say a bearer token - being used in a DDOS, the app server alone is in a position to definitively know the bad credential and sever the access. Especially with console or file-based logs that may contain carriage returns, line feeds, etc. embedded in them it is unreasonable to expect higher level software or hardware to identify and block the connection. The application on the server alone is positioned to reliably revoke a JWT or similar credential.

Argument for client: I have a peer on Android and a peer on the Microsoft Store both talking to each other across a Hub. I control the version of the Hub, but once I submit a change to Microsoft and Google, I don't necessarily control that review process. End user devices won't download the update until they see wifi. I make a change to the app that results in an exception on down-level versions. And so the app appears to be launched and running on the user's device, but nothing at all is happening. The user's access to logs is hampered because they're buried deep on both these OSes.

Argument for web browser clients: This impacts JavaScript clients as well, but to a lesser extent - primarily when fields that were primitive are moved to using structures and a CDN is at play, but a few other cases as well. It's less of a problem for a Microsoft or a Facebook, more of a problem for smaller teams. There tends to be a (wrong) thought that JSON and JavaScript's object solves all these problems, when in fact - it is the same as GWBASIC or Visual Basic with Option Strict off, and just pushes the Error 5, 7, 9 or 13 to runtime instead. F12 tools can be disabled, and so console logging isn't necessarily available when there's an issue.

[ghost]

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

[smoogipoo]

Hi, is this the appropriate place to report the following, or should I make another issue? There's a few related and I'm not sure if it would duplicate this one:

Right now if messagepack fails to bind method arguments, it triggers the following exception:

dbug: Microsoft.AspNetCore.SignalR.Internal.DefaultHubDispatcher[22]
      Parameters to hub method 'EndPlaySession' are incorrect.
      System.IO.InvalidDataException: Error binding arguments. Make sure that the types of the provided values match the types of the hub method being invoked.
       ---> System.IO.InvalidDataException: Deserializing object of the `SpectatorState` type for 'argument' failed.
       ---> MessagePack.MessagePackSerializationException: Failed to deserialize osu.Game.Online.Spectator.SpectatorState value.
       ---> MessagePack.MessagePackSerializationException: Unexpected msgpack code 203 (float 64) encountered.
         at MessagePack.MessagePackReader.ThrowInvalidCode(Byte code)
         at MessagePack.MessagePackReader.ReadInt64()
         at MessagePack.Formatters.osu_Game_Scoring_ScoringValuesFormatter3.Deserialize(MessagePackReader& reader, MessagePackSerializerOptions options)
         at MessagePack.Formatters.osu_Game_Online_Spectator_SpectatorStateFormatter1.Deserialize(MessagePackReader& reader, MessagePackSerializerOptions options)
         at MessagePack.MessagePackSerializer.Deserialize[T](MessagePackReader& reader, MessagePackSerializerOptions options)
         --- End of inner exception stack trace ---
         at MessagePack.MessagePackSerializer.Deserialize[T](MessagePackReader& reader, MessagePackSerializerOptions options)
         at lambda_method157(Closure , MessagePackReader& , MessagePackSerializerOptions )
         at MessagePack.MessagePackSerializer.Deserialize(Type type, MessagePackReader& reader, MessagePackSerializerOptions options)
         at Microsoft.AspNetCore.SignalR.Protocol.DefaultMessagePackHubProtocolWorker.DeserializeObject(MessagePackReader& reader, Type type, String field)
         --- End of inner exception stack trace ---
         at Microsoft.AspNetCore.SignalR.Protocol.DefaultMessagePackHubProtocolWorker.DeserializeObject(MessagePackReader& reader, Type type, String field)
         at Microsoft.AspNetCore.SignalR.Protocol.MessagePackHubProtocolWorker.BindArguments(MessagePackReader& reader, IReadOnlyList`1 parameterTypes)
         --- End of inner exception stack trace ---
         at Microsoft.AspNetCore.SignalR.Protocol.MessagePackHubProtocolWorker.BindArguments(MessagePackReader& reader, IReadOnlyList`1 parameterTypes)
         at Microsoft.AspNetCore.SignalR.Protocol.MessagePackHubProtocolWorker.CreateInvocationMessage(MessagePackReader& reader, IInvocationBinder binder, Int32 itemCount)

This can happen if the client and server don't have the same object model - we currently don't have a handshake mechanism for our project yet to prevent these errors.

This is a debug-level exception, which is unreasonable to enable filter for as this is a potential attack vector. Furthermore, the client receives these exceptions when calling InvokeAsync() in a way that is completely devoid of any information other than that it has failed to invoke the method.

Is the above related to this issue?