Skip to content

Deserialization failures are swallowed and returned as null #24

Description

@Agash

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

  1. 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.
  2. 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.
  3. 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.
  4. 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions