pardahlman / RawRabbit

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

Default connection timeout #132

Closed spetz closed 7 years ago

spetz commented 8 years ago

Hi, when I start the application and the RabbitMQ is not ready yet (let's say it's starting and needs a few more seconds before the service is ready) the application immediately crashes due to the connection failure. I couldn't find an option to set a timeout when initializing the client - just like in the MongoDB.Driver where I can set e.g. 30 seconds timeout for the database connection. It seems that if the RabbitMQ service is not ready, the library will just crash after a second. Is there a way to change this behaviour?

pardahlman commented 8 years ago

Hi @spetz, It is the ChannelFactory that tries to connect to the broker as soon as it is initiated. The idea is that if the connection string is incorrect, the application would throw an exception directly, rather than when first used (which could be triggered by an event further seconds, minutes or hours after the application is up and running).

Is it a common scenario that you broker does not respond? Do you start the application at the same time as you start the broker? I always run RabbitMQ Server as a service with autostart, so I never experienced this.

Unfortunately, the connection attempt is performed in the constructor of the ChannelFactory, so the only way I see to add a delay is to write your own implementation and register it when instanciating your client. Perhaps you could copy/paste the default one and remove the connection attempt?

I'd like to keep this ticket open, as it is a rather easy fix that could be a part of the up-comming 1.10.3 release.

spetz commented 8 years ago

Thank you for the quick response @pardahlman. The case that I'm talking about is e.g. when I use the docker-compose for the local development and start the whole infrastructure at the same time. I've found some scripts that can wait till the database/service is ready, but still, I'd like to have such a timeout connection available as an additional parameter if possible :). We've created our own factory like you suggested using the Polly for the retry policy and it's sufficient for now.

pardahlman commented 8 years ago

Ok perfect! I'm looking at Polly for general error handling in version 2.0 👍

ritasker commented 8 years ago

@pardahlman Are you wanting to add Polly to 1.10.3? Here is my own ChannelFactory the only difference being in the constructor were I use Polly to wrap RabbitMQs CreateConnection. Is the idea here to add an extra config property, on ChannelFactoryConfiguration that could be passed to the Polly Policy?

pardahlman commented 8 years ago

@ritasker - I like your implementation 👍. However, I am a bit hesitant to add Polly as a core dependency for RawRabbit. My plans, for 2.0 at least, is to move the Polly based error handling to its own NuGet package.

I think it might be enough to just move the creation of the channel to its own, virtual method and implement a "poor mans" exception handling. This way it is easy to create a custom implementation and override the default implementation with a nice Polly based retry. Makes sense?

cocowalla commented 8 years ago

@pardahlman Like others, I am also using Polly for this, although I handle it in my application code rather than with a custom ChannelFactory implementation.

My own preference would the same as yours, to move connection to a seperate virtual method that could be overriden in customer ChannelFactory implementations.

Slightly off-topic, but perhaps there could also be a toggle that gets passed into the ChannelFactory ctor to allow not connecting automatically. In this case the user would be responsible for calling the Connect() (or whatever it gets called) method. Without this, those of us using DI get massive stack traces about the container not being able to instantiate IBusClient, which hides the underlying problem (e.g. connection timeout, connection refused etc).

pardahlman commented 8 years ago

@cocowalla - thanks for the input! I think that if we move the connect call to a virtual method, we could implement a LazyChannelFactory that is derived from the default one, that overrides that method as empty. That way, a connection will be created when requesting the first channel.

cocowalla commented 8 years ago

Seems like a good way to approach this

pardahlman commented 8 years ago

@spetz - as discussed here, I've moved the connection to the broker to it's own virtual method

protected virtual void ConnectToBroker()
{
    try
    {
        _connection = _connectionFactory.CreateConnection(_config.Hostnames);
    }
    catch (BrokerUnreachableException e)
    {
        _logger.LogError("Unable to connect to broker", e);
        throw e.InnerException;
    }
}

It should be fairly easy to derive from this class and add create your delayed Polly based retry strategy.

spetz commented 7 years ago

Thank you for all of the answers guys, eventually, we did implement a similar solution and it works fine! :) Having this in a virtual method makes it even easier now.