nats.net icon indicating copy to clipboard operation
nats.net copied to clipboard

Adding async versions of event handlers for connection establishment

Open ebekker opened this issue 4 years ago • 7 comments

The Options class allows a caller to register several event handlers (EventHandler<T>) to delegate various stages of authentication during connection establishment, via methods such as SetUserCredentials(...) or SetNKey(...).

All of these take EventHandlers which means they are limited to synchronous code in the handler, but if the handler needs to call async code, there's no graceful way of doing this other than some sort of synchronization which would typically tie up a thread.

Perhaps it's time to look at adding asynchronous versions of these event handlers?

This would of course mean applying applying async pattern all the way down starting with the ConnectionFactory.Create*Connection(...) family of functions, but that's pby a worthwhile change to modernize the library.

ebekker avatar Jun 12 '20 00:06 ebekker

If this type of change would be welcome, then I would be happy to put together a PR?

ebekker avatar Jun 18 '20 17:06 ebekker

@ebekker , Thanks for the offer! Could you provide an small example with one of the event handlers so we can see what you are proposing without a significant time investment on your end?

ColinSullivan1 avatar Jun 22 '20 16:06 ColinSullivan1

Sure thing, let's say I want to customize my logic for handling the signing of the server nonce with NKeys authentication. Using the NATS.Client.Options class, I would set a custom handler for this event:

async Task ConnectAndPublish()
{
    var opts = ConnectionFactory.GetDefaultOptions();
    var jwt = await ResolveMyJWT(); // Get the JWT using some async approach
    opts.SetNKeys(jwt, HandleUserSigEvent);
    
    var cf = new ConnectionFactory();
    var conn = cf.CreateConnection(opts);
    conn.Publish("to.whom.it-may-concern", "Hello World!");
}

void HandleUserSigEvent(object source, UserSignatureEventArgs ev)
{
    // Because EventHandler<T> has a void return type, we have to do sync-over-async, ugh!
    var signed = SignChallengeNonce(ev.ServerNonce)
        .ConfigureAwait(false)
        .GetAwaiter()
        .GetResult();

    ev.SignedNonce = signed;
}

Task<byte[]> SignChallengeNonce(byte[] nonce)
{
    // TODO: does something async to retrieve NKeys and sign the nonce
}

ebekker avatar Jun 22 '20 21:06 ebekker

Unfortunately, .NET BCL does not include an async version of EventHandler, but you could define your own:

public delegate Task AsyncEventHandler<TEventArgs>(object sender, TEventArgs e);

Then simply provide overloads for all the Options event handler registrations, to take this async handler variation.

In the connection-handling code, these handlers need to be invoked asynchronously now, which will mean the async pattern will percolate throughout it and any code that calls upon it.

ebekker avatar Jun 22 '20 21:06 ebekker

@ColinSullivan1, any thoughts/feedback?

ebekker avatar Aug 19 '20 23:08 ebekker

Apologies for the delay. To be honest, I don't think the performance benefits will outweigh the changes to the existing client, and the current event handlers are designed to block the thread/task firing them.

That said, this is great feedback! We'll be developing a new API atop NATS to better represent the stream and service patterns. I think that would be the best place to add async event handlers.

ColinSullivan1 avatar Sep 01 '20 16:09 ColinSullivan1

How can I access the API, is it in a different branch, or a different project altogether?

ebekker avatar Oct 30 '20 14:10 ebekker