pardahlman / RawRabbit

A modern .NET framework for communication over RabbitMq
MIT License
747 stars 144 forks source link

Remove subscription for RPC request when queue name specified #266

Closed glenthomas closed 7 years ago

glenthomas commented 7 years ago

RawRabbit v1.10.3

I am using the RequestAsync method and specifying a reply queue name. The queue is created with Exclusive and Auto-Delete and the RPC request returns successfully, but the consumer and the queue remain and no subscription object is available to clean them up.

Is there any way, in the current release, to have the queue removed after the RPC request has completed?

pardahlman commented 7 years ago

Top of the morning! This should not be a problem, give me a few days to try to reproduce this.

pardahlman commented 7 years ago

I've been giving this some thought - and it might not be as easy as I first thought. The reason for this is that there is no trivial way to know if the consumer has been bound to the same queue in a concurrent RPC request. By removing the consumer from the queue upon receiving a response it might hinder the deliverance of a different response to a different request.

What's your scenario: do you create unique response queue name per RPC?

pardahlman commented 7 years ago

I've been giving this some more thought and came up with a solution for the 2.0 client. If you are curious, you can have a look at the relevant integration test as well as the new section in the documentation. It might be worth a read through as it addresses some of the things to consider when using custom queues.

My suggestion is, if at all possible, to migrate to the 2.0 client where this fix will be released shortly.

glenthomas commented 7 years ago

This would do what is required. It would be good if this was a default behaviour without having to explicitly call UseDedicatedResponseConsumer because having a shared queue for RPC requests is not a valid usage as far as I can see. The reason that I am naming the response queues is for visibility of queues in the management interface rather than for sharing them among consumers.

I have upgraded my projects to use v2 beta9. One thing that is confusing me is that there are multiple ways to specify a consumer queue and I am not sure what is the correct method, or if both are equal. (applies to various operations) For example, here I can specify the queue name in 2 different places:

busClient.RespondAsync<MyRequest, MyResponse>(
    request => OnRequest(request),
    context => context.UseRespondConfiguration(responder => responder
        .OnDeclaredExchange(exchange => exchange.WithName("Exchange1"))
        .FromDeclaredQueue(queue => queue.WithName("MyQueueName"))
        .Consume(builder => builder.WithRoutingKey("MyRoutingKey").FromQueue("MyQueueName"))), cancellationToken);
glenthomas commented 7 years ago

I think there might also be a problem with RawRabbit setting the ReplyTo header in v2. I used response.FromDeclaredQueue to specify a response queue and the header was set to

reply_to: | topic:///amq.rabbitmq.reply-to

instead of my queue name.

pardahlman commented 7 years ago

There are performance aspect for re-using response consumers in an application. In order to be able to consume a response, a queue needs to be declare, a exchange (might) be declare, the queue needs to be bound to the queue and a basic consume needs to occur. That's four round-trips to the broker.

If the response queues uses an application/instance specific, deterministic routing key, responses will be routed to the right application and complete the corresponding RPC.

pardahlman commented 7 years ago

As to your second comment, is that related to #275 ?

glenthomas commented 7 years ago

Yes, I opened #275 after writing that comment as it was a separate issue.

glenthomas commented 7 years ago

I haven't tried using the dedicated consumer extension, but it would satisfy this issue. Thanks

pardahlman commented 7 years ago

The related code has been released in rc1 🎉

Closing this, feel free to comment, re-open or create new ticket related to this.