Message flags

[mtmk]

Added message flags to indicate a couple of states and have room for future ones. Since NatsMsg is a struct we must keep its size in check. Flags added as a byte allows us to define upto seven flags only adding one byte to the struct size.

[caleblloyd]

I'm not sure that Flags is the right construct - it seems like this would mainly be used for identifying the Code, which are all exclusive of one another.

The original suggestion was to use something like the StatusCodes class

If we are trying to expose the defacto-standard "Code" header as a Status Code it should probably look something like this:

NatsMsg {

int StatusCode: => int.Parse(Headers?.Code)

}

public class NatsStatusCodes {
  public const int Unknown 0;
  public const int NoResponders 503;
}

Then we can get rid of all the booleans like:

public bool IsNoRespondersError => Headers?.Code == 503;

And if someone wants to check the status they can do:

msg.StatusCode == NatsStatusCodes.NoResponders

Or if they want to come up with their own message status codes, they could easily make thier own Status Codes constants and compare against those.

[mtmk]

I'm not sure that Flags is the right construct - it seems like this would mainly be used for identifying the Code, which are all exclusive of one another.

The original suggestion was to use something like the StatusCodes class

If we are trying to expose the defacto-standard "Code" header as a Status Code it should probably look something like this:

NatsMsg {

int StatusCode: => int.Parse(Headers?.Code)

}

public class NatsStatusCodes {
  public const int Unknown 0;
  public const int NoResponders 503;
}

That's fine but 503 might not always mean no-responders, it must have a more generic meaning I would've thought, no? hence the additional check for payload in below Go code? That's the reason I treated no-responders as a special case.

In a lot of cases we have to check the status message as well as the code, in general.

Then we can get rid of all the booleans like:

public bool IsNoRespondersError => Headers?.Code == 503;

I accepted that to not complicate the PR. Payload needs to be checked as well:

https://github.com/nats-io/nats.go/blob/bb64e1b6e378dcafbbffe33145db0b7ee064485a/js.go#L812-L816

We still need a way to identify an empty payload since using default/null doesn't work as an empty payload indicator because of value types. if we removed IsNoRespondersError property we still need a way to check empty payload.

[caleblloyd]

True, wherever we throw for No Responders we'd need the status code check + a message size check. Was that the main point of the flags - we have no way of telling if it was an empty message post-de-serialization?

[mtmk]

True, wherever we throw for No Responders we'd need the status code check + a message size check. Was that the main point of the flags - we have no way of telling if it was an empty message post-de-serialization?

pretty much, I can't think of another way of flagging (no pun intended) the fact payload is empty. I didn't want to use a bool and waste space, then IsNoRespondersError flag was just an easy addition to that.

[caleblloyd]

My main hesitation is that we are copying a state of a condition that could be checked pre-de-serialziation and creating a struct field that persists post-de-serialization. It shouldn't be hard for the user to check to see if it is an empty message - they know what serializer they are using - most of the time they are probably just checking msg.Data == null or msg.Data.Length == 0

Should we instead be settings something like NatsSubOpts.ThrowIfNoResponders?

Likewise, for sentinel message checking, should we set something like NatsSubOpts.StopOnEmptyMsg?

And we could leave these fields out of the user-facing NatsMsg and handle before de-serailzation?

[mtmk]

My main hesitation is that we are copying a state of a condition that could be checked pre-de-serialziation and creating a struct field that persists post-de-serialization. It shouldn't be hard for the user to check to see if it is an empty message - they know what serializer they are using - most of the time they are probably just checking msg.Data == null or msg.Data.Length == 0

problem is with some value types default doesn't mean empty payload e.g. number types. Also in generic methods like Request<T>() you don't know the type upfront.

Should we instead be settings something like NatsSubOpts.ThrowIfNoResponders?

Likewise, for sentinel message checking, should we set something like NatsSubOpts.StopOnEmptyMsg?

This is a good idea but unfortunately because of channels we can only throw exceptions by ending the subscription. I think that would make subscriptions less flexible.

And we could leave these fields out of the user-facing NatsMsg and handle before de-serailzation?

I think empty-payload is warranted because that information is effectively lost after serialization. I know checking for default works most of the time but is that food enough? I personally find using default to detect it unreliable and error-prone especially when dealing with a generic type and value types, I'd rather be certain about the fact.

no-responders to me is not just another error and we should be pragmatic about it. But I get the argument against it. If you feel strongly about it maybe We could move that to an extension method?

[stebet]

On the concept of throwing, I don't really feel that the 503 - No Responders (and similar errors) should throw unless the user specifically wants to.

HttpResponseMessage (from HttpClient) in .NET for example doesn't throw on non-200 statuses, but in case the user want's to see if the call is a valid result, it has a EnsureSuccessStatusCode() method to throw if needed, since there is no actual protocol error taking place.

[caleblloyd]

Alternate approach in #277

[mtmk]

Alternate approach in #277

I still think NatsMsg.HasEmptyPayload is necessary, not just for handling no responders but in general, as I tried to explain above, but if you prefer implementing it in NatsSub my only recomendation may be to keep at least the empty check option internal to avoid nasty bugs which might happen in my opinion.

[caleblloyd]

I still think NatsMsg.HasEmptyPayload is necessary, not just for handling no responders but in general

I agree that the library needs to know when a message payload is empty. But if all of those checks are performed pre-de-serialization, the library will never need to have that indicator on NatsMsg<T>. The end-user has knowledge of what serailzer they are using, so they can pretty easily already check to see if a message payload was empty.

[mtmk]

Closing in favor of #277 and not to stretch already too big NatsMsg struct any further.