Lazy queue creation, don't recreate on delete

[jamie94bc]

As dicussed in #25, #36, #37 and on Gitter.

Need to do AzureStorageQueue too - RedisQueue already permenently deletes and doesn't have network requests in the ctor, so that should be fine.

[jamie94bc]

I think it would be a good idea if QueueBase handled calling EnsureQueueCreatedAsync(). Couple of ways to do this:

1. Make the abstract methods virtual and update all implementations to call base (if appropriate).
2. Rename the abstract methods to to {MethodName}Impl, and implement the behavior in the public method... eg.

public abstract Task<string> EnqueueAsync(T data);

becomes...

public Task<string> EnqueueAsync(T data) {
    await EnsureQueueCreatedAsync().AnyContext();
    await EnqueueAsyncImpl(data).AnyContext();
}

protected abstract Task<string> EnqueueAsyncImpl(T data);

[jamie94bc]

Would it be worth accounting for the scenario where a queue might be deleted by another instance of the implementation? I didn't do that for the current implementation primarily because I don't want to be making an additional network request (in the case of Azure Storage / Service Bus) for almost every API call. Consider the below, or in a scale out scenario...

var a = new AzureServiceBusQueue("myqueue");
var b = new AzureServiceBusQueue("myqueue");

a.EnqueueAsync(new Thing());
var bEntry = b.DequeueAsync();

a.DeleteQueueAsync();
b.EnqueueAsync(new Thing()); // Throws as queue is deleted
a.EnqueueAsync(new Thing()); // Works as the class knows the queue is deleted and recreates

We could handle this by adding something like:

public abstract class QueueBase<T> {
    private bool _queueCreated;

    protected abstract Task EnsureQueueCreatedImplAsync(CancellationToken cancellationToken);
    protected Task EnsureQueueCreatedAsync(CancellationToken cancellationToken = default(CancellationToken)) {
        if (!_queueCreated) {
            await EnsureQueueCreatedImplAsync(cancellationToken).AnyContext();
            _queueCreated = true;
        }
    }

    protected async Task<TResult> WrapQueueCall<TResult>(Func<Task<TResult>> action, CancellationToken cancellationToken) where TResult : Task {
        await EnsureQueueCreatedAsync(cancellationToken);
        try {
            return await action();
        }
        catch (Exception ex) when (IsQueueDeletedException(ex)) {
            _queueCreated = false;
            await EnsureQueueCreatedAsync(cancellationToken);
            return await action();
        }
    }

    protected abstract bool IsQueueDeletedException(Exception ex);

    protected abstract Task<string> EnqueueImplAsync(T data);
    public async Task<string> EnqueueAsync(T data) {
        return await WrapQueueCall(() => EnqueueImplAsync(data));
        await EnsureQueueCreatedAsync().AnyContext();
        return await .AnyContext();
    }
}

public class AzureStorageQueue<T> : QueueBase<T> {
    protected override bool IsQueueDeletedException(Exception ex) => ex is StorageException && ex.Message.Contains("404");
}

[niemyjski]

I think if you delete it you should have to create it again

[jamie94bc]

Not sure I agree although not a common scenario.

If that is the case then we need a ClearQueueAsync() method (#25) and we also need to wrap the exception that is thrown (like Azure Storage SDK wraps WebExceptions with StorageExceptions).

[niemyjski]

What does this control? Shouldn't the auto complete setting be set from the constructor?

[jamie94bc]

This will tell Service Bus not to autocomplete... Autocompletion is controlled by queueEntry.CompleteAsync() and queueEntry.AbandonAsync() instead in order to trigger the events.

[jamie94bc]

Everyone happy to merge? 😄

[niemyjski]

As soon as the build passes :)

[jamie94bc]

I think @ejsmith was going to look into that Redis test 😉

[moswald]

You should catch the exception thrown when CreateQueueAsync fails because the queue already exists.

We've seen this in our current project using Foundatio: in a multi-process environment, QueueExistsAsync can return false for multiple processes, and then they each call CreateQueueAsync but only one will succeed. The rest will fail with either Microsoft.ServiceBus.Messaging.MessagingException or Microsoft.ServiceBus.Messaging.MessagingEntityAlreadyExistsException.

The correct solution is to catch those exceptions, and then fall back on calling GetQueueAsync. If that also fails, then you can bail. :)

[jamie94bc]

Hadn't noticed that, I'll get that sorted and see if i can add a test to cover this also.

[niemyjski]

We could add a distributed lock around this or bail out.

[niemyjski]

@moswald feel free to join our public slack channel: https://slack.exceptionless.com

[jamie94bc]

Yeah I did think that, but that makes queue coupled to a distributed locking provider. Rather not do that!