New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default connection timeout #132

Closed
spetz opened this Issue Oct 26, 2016 · 10 comments

Comments

Projects
None yet
4 participants
@spetz
Copy link

spetz commented Oct 26, 2016

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

This comment has been minimized.

Copy link
Owner

pardahlman commented Oct 26, 2016

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.

@pardahlman pardahlman added this to the 1.10.3 milestone Oct 26, 2016

@spetz

This comment has been minimized.

Copy link

spetz commented Oct 26, 2016

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

This comment has been minimized.

Copy link
Owner

pardahlman commented Oct 27, 2016

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

@ritasker

This comment has been minimized.

Copy link
Contributor

ritasker commented Nov 7, 2016

@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

This comment has been minimized.

Copy link
Owner

pardahlman commented Nov 10, 2016

@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

This comment has been minimized.

Copy link

cocowalla commented Nov 11, 2016

@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

This comment has been minimized.

Copy link
Owner

pardahlman commented Nov 12, 2016

@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

This comment has been minimized.

Copy link

cocowalla commented Nov 12, 2016

Seems like a good way to approach this

pardahlman added a commit that referenced this issue Nov 13, 2016

(#132) Move connect to virtual method
So that it can be overriden, error handled etc by custom
implementations
@pardahlman

This comment has been minimized.

Copy link
Owner

pardahlman commented Nov 13, 2016

@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

This comment has been minimized.

Copy link

spetz commented Nov 23, 2016

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.

@pardahlman pardahlman closed this Dec 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment