Exception Usage Harms Performance - Proposed Solutions

[srs-adamr]

As we all know Exceptions in .NET are costly, and unfortunately the SDK makes use of Exceptions pretty heavily. This can be exacerbated in certain situations especially.

For example, trying to GET a KV Record that does not exist:
GetEntryAsync

Will throw a NatsKVKeyNotFoundException.

One proposed solution, that would not require breaking changes, is to add Try methods that will not throw exceptions, or will only throw exceptions in certain situations.

Example:

public async ValueTask<(bool Success, NatsKVEntry<T>? Entry, string? ErrorReason)> TryGetEntryAsync<T>(string key, ulong revision = default, INatsDeserialize<T>? serializer = default, CancellationToken cancellationToken = default)
{
    if (!ValidateKey(key))
    {
        return (false, null, "Invalid key");
    }

    serializer ??= _context.Connection.Opts.SerializerRegistry.GetDeserializer<T>();

    var request = new StreamMsgGetRequest();
    var keySubject = $"$KV.{Bucket}.{key}";

    if (revision == default)
    {
        request.LastBySubj = keySubject;
    }
    else
    {
        request.Seq = revision;
        request.NextBySubj = keySubject;
    }

    if (_stream.Info.Config.AllowDirect)
    {
        var direct = await _stream.GetDirectAsync<T>(request, serializer, cancellationToken);

        if (direct is { Headers: { } headers } msg)
        {
            if (headers.Code == 404)
            {
                return (false, null, "Key not found");
            }

            if (!headers.TryGetLastValue(NatsSubject, out var subject) ||
                !headers.TryGetLastValue(NatsSequence, out var sequenceValue) ||
                !headers.TryGetLastValue(NatsTimeStamp, out var timestampValue))
            {
                return (false, null, "Missing required headers");
            }

            if (revision != default && !string.Equals(subject, keySubject, StringComparison.Ordinal))
            {
                return (false, null, "Unexpected subject");
            }

            if (!ulong.TryParse(sequenceValue, out var sequence) ||
                !DateTimeOffset.TryParse(timestampValue, out var timestamp))
            {
                return (false, null, "Can't parse sequence or timestamp");
            }

            var operation = NatsKVOperation.Put;
            if (headers.TryGetValue(KVOperation, out var operationValues))
            {
                if (operationValues.Count != 1 || !Enum.TryParse(operationValues[0], ignoreCase: true, out operation))
                {
                    return (false, null, "Invalid operation header");
                }
            }

            if (operation is NatsKVOperation.Del or NatsKVOperation.Purge)
            {
                return (false, null, "Key deleted");
            }

            var entry = new NatsKVEntry<T>(Bucket, key)
            {
                Bucket = Bucket,
                Key = key,
                Created = timestamp,
                Revision = sequence,
                Operation = operation,
                Value = msg.Data,
                Delta = 0,
                UsedDirectGet = true,
                Error = msg.Error,
            };

            return (true, entry, null);
        }
        else
        {
            return (false, null, "Missing headers");
        }
    }
    else
    {
        var response = await _stream.GetAsync(request, cancellationToken);

        if (revision != default && !string.Equals(response.Message.Subject, keySubject, StringComparison.Ordinal))
        {
            return (false, null, "Unexpected subject");
        }

        T? data = default;
        NatsDeserializeException? deserializeException = null;
        if (response.Message.Data.Length > 0)
        {
            var buffer = new ReadOnlySequence<byte>(response.Message.Data);

            try
            {
                data = serializer.Deserialize(buffer);
            }
            catch (Exception e)
            {
                deserializeException = new NatsDeserializeException(buffer.ToArray(), e);
            }
        }

        var entry = new NatsKVEntry<T>(Bucket, key)
        {
            Created = response.Message.Time,
            Revision = response.Message.Seq,
            Value = data,
            UsedDirectGet = false,
            Error = deserializeException,
        };

        return (true, entry, null);
    }
}

[mtmk]

I think this is a good idea and we can potentially create a Try* set of API calls doing the same thing where possible. thanks @srs-adamr 💯

[to11mtm]

We should make sure to agree on API.

My biggest concern with example, is that a ValueTask<(bool Success, NatsKVEntry<T>? Entry, string? ErrorReason)> is likely going to be fairly large due to the padding and general size of NatsKVEntry<T> on top of the nullable.

I do have one idea, this is less extreme than a full TryOption<T> setup...

public readonly struct TryGetValueResult<T>(T? Result, string ErrorMessage) // or exception, IDK.
{
    public bool HasValue => this.Result != null; //Need to verify this is OK with new ctor syntax lol
}

I am not great at names but something like this would at least keep a bool (and thus, likely 4/8 bytes due to padding) off the potential copy or alloc.

-IF- we need to have nullable structs where a 'success' could still be null, we'd need a TryOption<T> to be compact, but that might be too extreme...

public static class TryOption
{
    public static readonly NoValueSentinel = new Exception("No Value");
    public static readonly NoValue<T>() => TryOption<T>.NoValue;
    public static readonly Of<T>(T value) => new TryOption<T>(T value, null);
    public static readonly Error<T>(Exception error) => new TryOption<T>(default, error);
}
public readonly struct TryOption<T>
{
    //All the fun stuff to make this semi-serializable is elided here...
    public static readonly NoValue = new TryOption<T>(default, TryOption.NoValueSentinel);

    public readonly T? Value;
    public readonly Exception? Error;
    public TryOption(T? value, Exception? error)
    {
        Value = value;
        Error = error;
    }
    public bool HasValue => Object.RefrenceEquals(Error, TryOption.NoValueSentinel);
    public bool HasError => Error != null && !HasValue;
}

[galvesribeiro]

Although I agree that exceptions are expensive (not just in .Net but in general due to stack trace pinning and whatnot) and we shouldn't be using it to control application flow and instead only use it for, well, real exceptions, I have some comments that may help to drive the API shape:

1. GetXXX and ReadXXX usually has different semantics.
2. On GetXXX you can expect null if whatever criteria used as parameter/filter (including just the thing Id) can't find the resource.
3. On ReadXXX, it is expected that the thing exist so it can't return null and should throw an exception.
4. The TryXXX pattern is something that was added in .Net initially to help with control flow by returning true if the thing is found and return it on the out parameter. Back in the day when it was introduced, there was no such thing as Tuple nor nullable so we wouldn't be able to return two values on a method return (except if you encapsulate it on a data struct) or, return null with a clear indication to the API consumer.

That being said, in current C#/.Net, I would rather avoid using TryXXX as if the operation requires I/O it will require await on the if() statement which is horrible to read depending if the if() contains multiple operands.

Yes, I understand that breaking changes are not fun, but I would suggest to do it once to whatever pattern we decide and stick with it.

PS: Also, we should drop the XXXAsync pattern. New methods on new APIs doesn't use it anymore. It was only added back in the day when TPL wasn't good on resolving method overloads by return type. Now it is all good and we don't need it anymore.

Just my 2c.

[to11mtm]

That being said, in current C#/.Net, I would rather avoid using TryXXX as if the operation requires I/O it will require await on the if() statement which is horrible to read depending if the if() contains multiple operands.

Where this gets challenging IMO, is whether we choose to:

1. Provide an API that is easy enough to hack the requisite extension methods (with or without composable types) on for your cases
2. Provide the extension methods that are deemed good on top of an API
3. Provide a 'strict' API with types that encourage clean composition.

Yes, I understand that breaking changes are not fun, but I would suggest to do it once to whatever pattern we decide and stick with it.

I do hesitate here; pragmatically speaking, for better or worse, there is at minimum a measurable plurality of .NET Developers that will rely on exceptions for various forms of notification that something unexpected happened. I think the best example of this to me, is in the example from OP: IMO a 'key not found' is not necessarily an exception, but a failure to deserialize a key you -did- get, IMO is far more likely to be truly exceptional.

Make simple cases possible while allowing for complex cases is a good mindset to have for this problem; What can we do to keep the API 'easy' for that plurality while allowing those who want to eke every microsecond of perf out to do what they need to do?

[galvesribeiro]

is in the example from OP: IMO a 'key not found' is not necessarily an exception, but a failure to deserialize a key you -did- get, IMO is far more likely to be truly exceptional.

If the key is indeed expect by a hard serialization "contract" (i.e. protobuf, or any other binary serialization) then yes, for sure, this is an exception. However, if using a flexible and/or version tolerant protocol (i.e. JSON), then it should not be an exception and just ignore the missing property or, like System.Text.Json, give the option for the consumer to handle this like throwing exception with missing field or just ignore it.

Make simple cases possible while allowing for complex cases is a good mindset to have for this problem; What can we do to keep the API 'easy' for that plurality while allowing those who want to eke every microsecond of perf out to do what they need to do?

I agree on this thought line. But that is how you build APIs from bottom up. In a sense that the fastest APIs are the lower level ones. Then on top of it, for the sake of user friendliness, you add the "syntax sugar".

So, IMHO, if we could make the low-level APIs faster by not throwing, then keep the APIs which do throw as the "higher level", that would be better. The problem is that, for this approach, given the current situation, we would need to break people since the "friendly" API is on the core already and everyone is using it unfortunately.

[galvesribeiro]

Having a second thought, we could keep the existing API surface behaving as is by being extension methods on top of a new "core" or "low-level" layer which doesn't throw, don't do anything out of necessary. That would lay the grounds for other extensibility on top of this new surface. It was used on .Net itself in a lot of places in order to avoid breakage.

In other words, don't add new APIs on top of the "slow" existing layer. Make a new core API which is faster/leaner and make the "old" methods extension methods on top of it instead.

[srs-adamr]

This is what I was suggesting with the Try methods, as any extension method if it used the existing methods, would simply be catching and hiding the exception and performance would still be plagued.

I think before any meaningful code can be written, a consensus on what and what shouldn't be an exception needs to be reached, I already see some conflicting views in this discussion 👍

[Kaarel]

PS: Also, we should drop the XXXAsync pattern. New methods on new APIs doesn't use it anymore. It was only added back in the day when TPL wasn't good on resolving method overloads by return type. Now it is all good and we don't need it anymore.

There's a discussion on that topic here #91

[darkwatchuk]

Hi, agreed this is needed, suggested this in #441 but not had the time to pursue in earnest.

I like the idea of @to11mtm's with the TryOption , and whilst I'll be happy to change my code, I think now it's released we should keep the current API calls as they are without breaking. Maybe change the name though?!!

Rather, we could add a whole Try...... command suite on top.

My suggestion if possible would be to take all the current calls and migrate them to not throw any exceptions at all but to return the TryOption in all cases with the exception as one of the fields. Personally I'd rather see a Success field too. We then take all the current API names and call the Try...... ones underneath, and bubble the exceptions That way we can have the new try calls at a lower level with the real code inside, and just the old API names on top for compatibility.

E.g.

Current

KVEntry GetEntryAsync(string key)
{
      ...... main code
      .....
           throw ex;
      .....
}

To something like

TryOption<KVEntry> TryGetEntryAsync(string key)
{
      ...... main code
      .....
            return TryOption.Fail(ex);
      ....
}


KVEntry GetEntryAsync(string key)
{
      x = TryGetEntryAsync(key);
      if (x.Success)
      {  
           return x.Value;
      }
     else {
           throw x.Exception;
     }
}

[srs-adamr]

So something like this? :

Implementation">
public static class TryOption
{
    public static readonly Exception NoValueSentinel = new Exception("No Value");
    public static TryOption<T> NoValue<T>() => TryOption<T>.NoValue;
    public static TryOption<T> Of<T>(T value) => new TryOption<T>(value, null);
    public static TryOption<T> Error<T>(Exception error) => new TryOption<T>(default, error);
}

public readonly struct TryOption<T>
{
    public static readonly TryOption<T> NoValue = new TryOption<T>(default, TryOption.NoValueSentinel);

    public readonly T? Value;
    public readonly Exception? Error;

    public TryOption(T? value, Exception? error)
    {
        Value = value;
        Error = error;
    }

    public bool HasValue => !Object.ReferenceEquals(Error, TryOption.NoValueSentinel);
    public bool HasError => Error != null && !HasValue;
    public bool Success => HasValue && !HasError;
}

public async ValueTask<TryOption<NatsKVEntry<T>>> TryGetEntryAsync<T>(string key, ulong revision = default, INatsDeserialize<T>? serializer = default, CancellationToken cancellationToken = default)
{
    try
    {
        ValidateKey(key);
        serializer ??= _context.Connection.Opts.SerializerRegistry.GetDeserializer<T>();

        var request = new StreamMsgGetRequest();
        var keySubject = $"$KV.{Bucket}.{key}";

        if (revision == default)
        {
            request.LastBySubj = keySubject;
        }
        else
        {
            request.Seq = revision;
            request.NextBySubj = keySubject;
        }

        if (_stream.Info.Config.AllowDirect)
        {
            var direct = await _stream.GetDirectAsync<T>(request, serializer, cancellationToken);

            if (direct is { Headers: { } headers } msg)
            {
                if (headers.Code == 404)
                    return TryOption.Error<NatsKVEntry<T>>(new NatsKVKeyNotFoundException());

                if (!headers.TryGetLastValue(NatsSubject, out var subject))
                    return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Missing sequence header"));

                if (revision != default)
                {
                    if (!string.Equals(subject, keySubject, StringComparison.Ordinal))
                    {
                        return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Unexpected subject"));
                    }
                }

                if (!headers.TryGetLastValue(NatsSequence, out var sequenceValue))
                    return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Missing sequence header"));

                if (!ulong.TryParse(sequenceValue, out var sequence))
                    return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Can't parse sequence header"));

                if (!headers.TryGetLastValue(NatsTimeStamp, out var timestampValue))
                    return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Missing timestamp header"));

                if (!DateTimeOffset.TryParse(timestampValue, out var timestamp))
                    return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Can't parse timestamp header"));

                var operation = NatsKVOperation.Put;
                if (headers.TryGetValue(KVOperation, out var operationValues))
                {
                    if (operationValues.Count != 1)
                        return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Unexpected number of operation headers"));

                    if (!Enum.TryParse(operationValues[0], ignoreCase: true, out operation))
                        return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Can't parse operation header"));
                }

                if (operation is NatsKVOperation.Del or NatsKVOperation.Purge)
                {
                    return TryOption.Error<NatsKVEntry<T>>(new NatsKVKeyDeletedException(sequence));
                }

                return TryOption.Of(new NatsKVEntry<T>(Bucket, key)
                {
                    Bucket = Bucket,
                    Key = key,
                    Created = timestamp,
                    Revision = sequence,
                    Operation = operation,
                    Value = msg.Data,
                    Delta = 0,
                    UsedDirectGet = true,
                    Error = msg.Error,
                });
            }
            else
            {
                return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Missing headers"));
            }
        }
        else
        {
            var response = await _stream.GetAsync(request, cancellationToken);

            if (revision != default)
            {
                if (string.Equals(response.Message.Subject, keySubject, StringComparison.Ordinal))
                {
                    return TryOption.Error<NatsKVEntry<T>>(new NatsKVException("Unexpected subject"));
                }
            }

            T? data;
            NatsDeserializeException? deserializeException = null;
            if (response.Message.Data.Length > 0)
            {
                var buffer = new ReadOnlySequence<byte>(response.Message.Data);

                try
                {
                    data = serializer.Deserialize(buffer);
                }
                catch (Exception e)
                {
                    deserializeException = new NatsDeserializeException(buffer.ToArray(), e);
                    data = default;
                }
            }
            else
            {
                data = default;
            }

            return TryOption.Of(new NatsKVEntry<T>(Bucket, key)
            {
                Created = response.Message.Time,
                Revision = response.Message.Seq,
                Value = data,
                UsedDirectGet = false,
                Error = deserializeException,
            });
        }
    }
    catch (Exception ex)
    {
        return TryOption.Error<NatsKVEntry<T>>(ex);
    }
}

// Keeping the original method for backward compatibility
public async ValueTask<NatsKVEntry<T>> GetEntryAsync<T>(string key, ulong revision = default, INatsDeserialize<T>? serializer = default, CancellationToken cancellationToken = default)
{
    var result = await TryGetEntryAsync(key, revision, serializer, cancellationToken);
    if (result.Success)
    {
        return result.Value!;
    }
    else
    {
        throw result.Error!;
    }
}

With the non-exception generating usage looking like:

var result = await TryGetEntryAsync(key);
if (result.Success)
{
    // Use result.Value
}
else
{
    // Handle the error (result.Error)
}

[darkwatchuk]

Yes, exactly.

Additionally, if any of those Try... functions call anything lower down which throw exceptions that could be considered not exceptions then those should be calling Try too. As you say, might need to decide what, and what isn't an exception.

Certainly calling KV.TryUpdate with a revision number shouldn't cause an exception if I can't do that due to the revision number being incorrect. KV.Put and KV.Update currently call other functions and does other checks so those might need to be adapted too...?

TryPut calls TryUpdate etc....

[to11mtm]

I should note it's -easy- to go down rabbit holes with this, but we should be hesitant to bake too many in.

For example,

public static class TryOptionCompositionExtensions
{
    //Would of course have async versions too.
    public static TryOption<B> Map<A,B>(this TryOption<A> tryOpt, Func<A,TryOption<B>> success, Func<TryOption<B>> none, Func<Exception,TryOption<B>> handleError)
    {
        if (tryOpt.HasError)
            return handleError(tryOpt.Error);
        else if (tryOpt.HasValue)
            return success(tryOpt.Value);
        else
            return none();
    }
}

But that is of course a matter of taste, may be too 'functional' for some...

[darkwatchuk]

Ummmm. No, not what I had in mind!! Just return the lowest one!

[mtmk]

(not directly related but I think we should review consumer next/fetch/consume methods exception driven recovery mechanisms as well)

[mtmk]

couple of other location I can think of we need to untangle exception driven logic is the no-responders in subscriptions and jetstream request-reply error handling. and one more, jetstream publishing is another, but maybe that's a higher level solution using something like TryPubublish.

[mtmk]

• Add flags to NatsMsg and JetStream Request #652

[mtmk]

(post edits: added csharp to your code blocks)

[mtmk]

How about this as the option/result struct?

https://github.com/nats-io/nats.net/pull/652/files#diff-62baedbc9d7a5d5eaf568e61cf98341ad19dc56412ad526d3ddb07af484f9646R5

[srs-adamr]

Have we reached a consensus?

[mtmk]

feels like it. what do you think of #652 with struct NatsResult<T>

[srs-adamr]

LGTM

[mtmk]

• Merged: Add flags to NatsMsg and JetStream Request #652
  I'll prepare a release soon