What happens
MsgPackMessageSerializer.DeserializePayload<T> and DeserializeValuePayload<T>, and the equivalents in JsonMessageSerializer, catch every exception, log, and return default:
catch (Exception ex)
{
_logger.LogMessagepackFailedToDeserializePayloadObjectTo(ex, typeof(TPayload).Name, rawPayloadData.GetType().Name);
return default;
}
The caller then sees a null payload and has no idea why. In #21 this turned a total transport failure into a message that blamed OBS:
FormatterNotRegisteredException: List<JsonElement> is not registered in resolver
-> "OBS reported success for 'GetCanvasList' but returned no payload."
GetCanvasList was unreadable over MessagePack for every user, and the reported symptom pointed at the server. Finding the real cause needed a hand written probe that called MessagePackSerializer.Deserialize directly.
Is it bad practice
Yes, on two counts. It discards the diagnosis, and it converts a hard failure into a value that flows on and fails somewhere unrelated. The library already has ObsWebSocketSerializationException for exactly this, and uses it on the serialize side, so the two directions are inconsistent.
There is a legitimate reason it was written this way, which is why the fix is not a blanket rethrow: not every caller is a caller.
The two kinds of caller
Awaited request paths. CallAsync, CallAsyncValue, CallRequiredAsync, and the batch response paths. A caller is awaiting a Task, so an exception reaches them and is the right outcome. Returning null here loses the reason and produces a misleading downstream error.
The receive loop. Event payload deserialization, and the outer envelope. Throwing here would tear down the connection for one malformed or unrecognised event, which is worse than dropping it. A newer OBS sending an event this build cannot model must not disconnect the client.
Suggested shape
- Add a
bool throwOnFailure to the serializer's deserialize methods, or split into Deserialize... (throws) and TryDeserialize... (returns false). The split reads better and cannot be got wrong by defaulting.
- Request and batch response paths use the throwing form, wrapping in
ObsWebSocketSerializationException with the payload type name and the inner exception, matching what the serialize side already does.
- The event path keeps the tolerant form, but the log has to be at
Error with the event type and the exception, so a dropped event is visible rather than merely absent. Worth checking the current level, since a swallow at Debug is effectively silent.
- Add a test that a payload which cannot be deserialized surfaces as
ObsWebSocketSerializationException on the request path, and as a dropped event with a logged error on the event path.
Note
The payload shape check added in #21 catches one common case, a payload read as the wrong record, before deserialization is attempted. It does not help here: a missing formatter fails after the shape check passes, and any other serializer fault is still swallowed.
What happens
MsgPackMessageSerializer.DeserializePayload<T>andDeserializeValuePayload<T>, and the equivalents inJsonMessageSerializer, catch every exception, log, andreturn default:The caller then sees a null payload and has no idea why. In #21 this turned a total transport failure into a message that blamed OBS:
GetCanvasListwas unreadable over MessagePack for every user, and the reported symptom pointed at the server. Finding the real cause needed a hand written probe that calledMessagePackSerializer.Deserializedirectly.Is it bad practice
Yes, on two counts. It discards the diagnosis, and it converts a hard failure into a value that flows on and fails somewhere unrelated. The library already has
ObsWebSocketSerializationExceptionfor exactly this, and uses it on the serialize side, so the two directions are inconsistent.There is a legitimate reason it was written this way, which is why the fix is not a blanket rethrow: not every caller is a caller.
The two kinds of caller
Awaited request paths.
CallAsync,CallAsyncValue,CallRequiredAsync, and the batch response paths. A caller is awaiting aTask, so an exception reaches them and is the right outcome. Returning null here loses the reason and produces a misleading downstream error.The receive loop. Event payload deserialization, and the outer envelope. Throwing here would tear down the connection for one malformed or unrecognised event, which is worse than dropping it. A newer OBS sending an event this build cannot model must not disconnect the client.
Suggested shape
bool throwOnFailureto the serializer's deserialize methods, or split intoDeserialize...(throws) andTryDeserialize...(returns false). The split reads better and cannot be got wrong by defaulting.ObsWebSocketSerializationExceptionwith the payload type name and the inner exception, matching what the serialize side already does.Errorwith the event type and the exception, so a dropped event is visible rather than merely absent. Worth checking the current level, since a swallow atDebugis effectively silent.ObsWebSocketSerializationExceptionon the request path, and as a dropped event with a logged error on the event path.Note
The payload shape check added in #21 catches one common case, a payload read as the wrong record, before deserialization is attempted. It does not help here: a missing formatter fails after the shape check passes, and any other serializer fault is still swallowed.