0

I have created a custom Observer which basically execute an async Task in its OnNext() method.

I'm wondering if it's a good idea to do it having in mind async void is not great.

public class MessageObserver : IObserver<Message>
{
    private IDisposable _unsubscriber;
    private readonly IQueueClient _client;
    private readonly TelemetryClient _telemetryClient;

    public MessageObserver(IQueueClient client, TelemetryClient telemetryClient)
    {
        _client = client;
        _telemetryClient = telemetryClient;
    }

    public virtual void Subscribe(IObservable<Message> provider)
    {
        _unsubscriber = provider.Subscribe(this);
    }

    public virtual void Unsubscribe()
    {
        _unsubscriber.Dispose();
    }

    public virtual void OnCompleted()
    {
    }

    public virtual void OnError(Exception error)
    {
    }

    public virtual async void OnNext(Message message)
    {
        try
        {
            await _client.SendAsync(message);
        }
        catch (Exception ex)
        {
            _telemetryClient.TrackException(ex);
        }
    }
}

EDIT/Add code

I have an API to which I post a resource from an Angular client and once the resource has been recorded into database, I immediately send a message to Azure Service Bus then I return the previously recorded entity.

I do not want to wait for the Azure Service Bus message to be sent before returning back to the client, so I want to notify a Rx Observer I have a new message that needs to be processed asynchronously on an other thread.

Here's my structure:

    // POST: /api/management/campaign
    [HttpPost]
    public async Task<IActionResult> Create([FromBody] CampaignViewModel model)
    {
        try
        {
            if (ModelState.IsValid)
            {
                var createdCampaign = await _campaignService.CreateCampaign(Mapping.ToCampaign(model));
                _upsertServiceBus.SendMessage(new Message(Encoding.UTF8.GetBytes(createdCampaign.CampaignId.ToString())));
                return Ok(Mapping.ToCampaignViewModel(createdCampaign));
            }

            return BadRequest(ModelState);
        }
        catch (Exception ex)
        {
            _telemetryClient.TrackException(ex);
            return BadRequest(new OpenIdConnectResponse
            {
                Error = OpenIdConnectConstants.Errors.InvalidRequest,
                ErrorDescription = Constants.GenericError
            });
        }
    }

-

    public class BusService : IBusService
    {
        private readonly IObservable<Message> _messageObservable;
        private readonly ICollection<Message> _messages = new Collection<Message>();
        private readonly IQueueClient _queueClient;
        private readonly MessageObserver _messageObserver;
        private readonly TelemetryClient _telemetryClient;

        protected BusService(IConfiguration configuration, string queueName, TelemetryClient telemetryClient)
        {
            _telemetryClient = telemetryClient;
            _queueClient = new QueueClient(configuration["ServiceBusConnectionString"], queueName);
            _messageObservable = _messages.ToObservable();
            _messageObserver = new MessageObserver(_queueClient, _telemetryClient);
            _messageObserver.Subscribe(_messageObservable);
        }

        public void SendMessage(Message message)
        {
            _messageObserver.OnNext(message);
        }
    }

EDIT/Solution with the help of @Shlomo's answer:

public class BusService : IBusService
{
    private readonly IQueueClient _queueClient;
    private readonly TelemetryClient _telemetryClient;
    private readonly Subject<Message> _subject = new Subject<Message>();

    protected BusService(IConfiguration configuration, string queueName, TelemetryClient telemetryClient)
    {
        _telemetryClient = telemetryClient;
        _queueClient = new QueueClient(configuration["ServiceBusConnectionString"], queueName);
        _subject
            .ObserveOn(TaskPoolScheduler.Default)
            .SelectMany(message =>
            {
                return Observable.FromAsync(() =>
                {
                    var waitAndRetryPolicy = Policy
                        .Handle<Exception>()
                        .WaitAndRetryAsync(3, retryAttempt =>
                                TimeSpan.FromSeconds(Math.Pow(2, retryAttempt)),
                            (exception, retryCount, context) =>
                            {
                                _telemetryClient.TrackEvent(
                                    $"Sending message to Azure Service Bus failed with exception ${exception.Message}. Retrying...");
                            }
                        );

                    return waitAndRetryPolicy.ExecuteAsync(async ct =>
                    {
                        _telemetryClient.TrackEvent("Sending message to Azure Service Bus...");
                        await _queueClient.SendAsync(message);
                    }, CancellationToken.None);
                });
            })
            .Subscribe(unit => { _telemetryClient.TrackEvent("Message sent to Azure Service Bus."); },
                ex => _telemetryClient.TrackException(ex));
    }

    public void SendMessage(Message message)
    {
        _subject.OnNext(message);
    }
}
  • 1
    No. This is not a good idea. `async void` is a bad idea, as is implementing your own Observer. Whatever you're trying to do, I'm positive there's a better way to do it. I suggest you post what you're trying to achieve. – Shlomo Aug 20 '18 at 17:47
  • I have updated my topic accordingly. –  Aug 20 '18 at 19:51

1 Answers1

1

I can't replicate or test, but hopefully this gets you started.

This solution replaces _messages, _messageObserver and _messageObservable with a subject and a Reactive Query. A couple notes:

  • ObserveOn allows you to shift threads by changing 'schedulers'. I selected the TaskPoolScheduler which will execute the rest of the query on a different Task.
  • I would recommend calling the syncronous version of _queueClient.SendAsync since this example allows Rx to handle the threading.
  • This solution uses Rx exception handling which will terminate the observable/handling in case of an exception. If you want it to restart automatically, then add a .Catch/.Retry.

Code:

public class BusService : IBusService
{
    private readonly IQueueClient _queueClient;
    private readonly TelemetryClient _telemetryClient;
    private readonly Subject<Message> _subject = new Subject<Message>();

    protected BusService(IConfiguration configuration, string queueName, TelemetryClient telemetryClient)
    {
        _telemetryClient = telemetryClient;
        _queueClient = new QueueClient(configuration["ServiceBusConnectionString"], queueName);
        _subject
            .ObserveOn(TaskPoolScheduler.Default)  // handle on an available task
            .Select(msg => _queueClient.Send(msg)) // syncronous, not async, because already on a different task
            .Subscribe(result => { /* log normal result */}, ex => _telemetryClient.TrackException(e), () => {});
    }

    public void SendMessage(Message message)
    {
        _subject.OnNext(message);
    }
}

As I mentioned, the code uses a Subject, and you'll find plenty of Q&A on here not recommending them. If you want, you can replace the subject with an event and an observable sitting on that event. Subjects are easier to demonstrate, and I would argue fine when kept private.

Shlomo
  • 14,102
  • 3
  • 28
  • 43
  • Seems great, thanks! So I'm thinking about calling _queueClient.SendAsync(msg).Wait(); since there is no synchronous method call from QueueClient. But I got the point, many thanks. –  Aug 21 '18 at 05:00
  • I've updated my topic to include the solution, involving also Polly retry policy. Thanks again. –  Aug 21 '18 at 06:01