search

0x4b53 / amqp-rpc

🐰 Framework to use RabbitMQ as RPC
MIT License
45 stars 8 forks source link

Add OnStarted(f) to Client #65

Closed akarl closed 5 years ago

akarl commented 5 years ago

This commit also ensures that the OnStartedFuncs is called before the Client and Server starts consuming/publishing messages, to ensure that any settings on the channels or connections are applied before the first message.

bombsimon commented 5 years ago

Ok I obviously missed to expand your commit message which clarifys why this is called earlier with this commit.

I guess it makes sense to reason like you do but I would like to know what you think about the use cases where we notify the user that we're connected and running by calling the onStarted function but they error out right after. How should we notify about that?

Should we just ignore that and assume that the user reads the logs? I have a vague memory that one of the use cases of this function was to call it to notify that we won't throw an error and that we're connected.

akarl commented 5 years ago

The OnStarted functions will execute multiple times at a reconnect anyway so I think it's fine to say that they will be executed after the connection has been established but before the consumers has been started.

I'm not sure we need to a way to notify the OnStarted if there is an error in the startup. It would just be called again on the next retry.

Do you have an idea on how to ensure that if I for example setup a NotifyPublish, I can set that up before the first message is sent?

bombsimon commented 5 years ago

I guess you're right. I see no issue with this as long as we document what the proper way to ensure that we're connected might be. Can we do some kind of test? Or maybe just refer to amqp.IsConnected() when it's merged and public?

No I don't and I understand that this is a way around that. I guess it's just to choose one way or the other and your approach seems more reasonable.

LGTM!