nats-io / nats.net.v1

The official C# Client for NATS
Apache License 2.0
646 stars 153 forks source link

Int32 Overflow Exception when trying to upate a Stream's Configuration #923

Open andronachev opened 6 days ago

andronachev commented 6 days ago

Observed behavior

Hi there,

We noticed an error in our code starting happening recently. Basically an int32 overflow exception happening at serialization time. The stack trace points us to where we update our Stream configuration (subjects etc).

   at System.Convert.ThrowInt32OverflowException()
   at System.Convert.ToInt32(Int64 value)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.GetMemberAndWriteJson(Object obj, WriteStack& state, Utf8JsonWriter writer)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.GetMemberAndWriteJson(Object obj, WriteStack& state, Utf8JsonWriter writer)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.JsonSerializer.WriteUsingSerializer[TValue](Utf8JsonWriter writer, TValue& value, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.WriteStringUsingSerializer[TValue](TValue& value, JsonTypeInfo jsonTypeInfo)
   at Services.Subscriptions.NatsConnectionManager.GetStreamAndUpdateSubjects(String streamName, String[] subjects, StreamConfiguration& streamConfiguration, JetStream& jetStream) in /src/Services/Subscriptions/NatsConnectionManager.cs:line 104

In this method we call IJetStreamManagement.GetStreamInfo and after that we call IJetStreamManagement.UpdateStream(StreamConfiguration streamConfiguration)... passing in an object of type StreamConfiguration which I noticed while viewing the source code of the NATS.Client nugget version 1.1.6

has: internal int _maxMsgSize = -1;

Meanwhile another thing that has changed in our system is another micro-service who updates this stream, has been released, and it has NATS.Client version 1.1.5 . When checking this version's type StreamConfiguration, it has this maxMsgSize type long. internal long _maxMsgSize = -1L;

So, to summarize:

Stream created using NATS.Client versoin 1.1.5 and then updated using NATS.Client version 1.1.6 is causing int32 overflow exceptions, supposedly because of data-type change of the _maxMsgSize from the type StreamConfiguration in the NATS.Client nugget.

Thanks

Expected behavior

No int32 overflow exceptions

Server and client version

Nats server: 0.0.33

NATS.Client nuget in service with error: 1.1.6 NATS.Client nuget in the other service which seems to be working fine: 1.1.5

Host environment

No response

Steps to reproduce

Presumably: create a Stream with NATS.Client 1.1.5 and then try to update the stream with NATS.Client 1.1.6

scottf commented 6 days ago

@andronachev This change was for a bug in the client that misrepresented the size of that value to the server, allowing for the client user to mistakenly put an incorrect value. I assumed that this was a safe change considering that it was unlikely that a user did this in the first place, considering the maximum signed integer is considerably higher than any reasonable value. (I suppose the same logic can be used to suggest there was no need to change it.)

As noted in the code that makes this change,

[Obsolete("The server value is a 32-bit signed value. Use MaximumMessageSize.", false)]
public long MaxMsgSize => MaximumMessageSize;

And from code in the server:

type StreamConfig struct {
...
    MaxMsgSize int32 `json:"max_msg_size,omitempty"`

I suppose, under the covers I could use a long and just coerce it's value, but it's preferred that the client not modify user input.

andronachev commented 6 days ago

Hi @scottf , I am not sure I understand all the details. But I can confirm I downgraded to 1.1.5 in the service that was having the error, and the errors stopped.

So now all the services update the server stream with NATS.Client 1.1.5

I did not try/test to upgrade the other one to 1.1.6 ( so to have both set to 1.1.6), I chose to downgrade the 1.1.6 to 1.1.5 because 1.1.5 still has the long member in it.

What has changed to our stream was indeed the max message size, it is now 181MB but that value even in bytes is not bigger than int32, so in the end I didn't understand fully where the error came from, hence your message as well, sorry

Thanks Vlad

edit: would it be safe if we used the same version of the client nugget in all services, even if it's 1.1.6 with the int member there, or will it be a fix in 1.1.8, or should we change the max msg size even though it's not over int32 in bytes ?

scottf commented 6 days ago

@andronachev What is this used for in your system: System.Text.Json.Serialization.JsonConverter ? I don't see this as part of the client lib. Maybe you can serialize / deserialize differently, this seems to be the cause of the issue.

This does make sense. If you serialize something from 1.1.5, the value is a long. But when you go to 1.1.6 the value is an int. So the internal storage of what is serialized in 1.1.5 is not going to match 1.1.6.

You might need to write a converter. I could make this method public instead of internal to support that.

internal StreamConfiguration(string json)

You can call StreamConfiguration ToJsonString() to serialize

scottf commented 6 days ago

https://github.com/nats-io/nats.net.v1/pull/924

andronachev commented 5 days ago

hi @scottf , System.Text.Json is from the .NET Core framework, we do not use it explicitly, we just call IJetStreamManagement.UpdateStream(StreamConfiguration streamConfiguration)

Although serialization/deserialization should not have been an issue with 1.1.5 and/or 1.1.6 because the value itself is not over int32, unless if in the internals of the stream config, also the datatype is serialized together with the field information and value, this would make sense.

Then it means we should have the same versions in all of our services, and if this is right, then when we update to 1.1.6 eventually, we will need to redo the stream configuration, presumably it has the JSON configuration serialized with the old datatype (long) in it. This is what I make of this so far

Thanks !

scottf commented 4 days ago

Services.Subscriptions.NatsConnectionManager is not part of the client. The way binary serialization works as far as I know, is it it gives type information as part of the serialized data, or a version or something. It does not surprise me that this would fail. I would identify where that NatsConnectionManager is and address it there. I would also consider avoiding updating the stream as part of the connection management. Stream Management should be a management function outside of the application if possible.

I'll be closing this issue when #924 is released.