Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NatsMsg Build should not try to deserialize 0 length payloadBuffer #531

Open
sgwong opened this issue Jul 1, 2024 · 2 comments
Open

NatsMsg Build should not try to deserialize 0 length payloadBuffer #531

sgwong opened this issue Jul 1, 2024 · 2 comments

Comments

@sgwong
Copy link

sgwong commented Jul 1, 2024

Proposed change

In the NatsMsg Build function. It should check the payloadBuffer length and only deserialize when its not empty. else, it should return default value.

      if (headers?.Error == null)
        {
            try
            {
                if (payloadBuffer.Length == 0)
                    data = default;
                else
                    data = serializer.Deserialize(payloadBuffer);
            }
            catch (Exception e)
            {
                headers ??= new NatsHeaders();
                headers.Error = new NatsDeserializeException(payloadBuffer.ToArray(), e);
                data = default;
            }
        }

Use case

I am facing error on latest nats with my msgpack deserializer. I am not sure that is changes from new nats server or the nats.net client. I have to add checking for 0 length buffer as below. I think it should check the buffer length before calling the deserialize function. Not sure there is any use case that some serializer need to construct other return value for 0 length buffer instead of default?

public T? Deserialize(in ReadOnlySequence<byte> buffer)
    {
        if (buffer.Length == 0)
            return default;
        return MessagePackSerializer.Deserialize<T>(buffer);
    }

Contribution

The changes are in this proposal. I am not planning to submit PR for this because changes are trivial.

@mtmk
Copy link
Collaborator

mtmk commented Jul 1, 2024

thanks for the suggestion @sgwong. I believe this used to be the case but someone complained that they wanted to deserialize empty messages in a certain way so we changed it then leaving the decision to the serializers.

@sgwong
Copy link
Author

sgwong commented Jul 1, 2024

Ok, thanks for the info. I have no issue to handle the empty payload in the serializer. Can close this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants