rabbitmq / rabbitmq-java-client

RabbitMQ Java client
https://www.rabbitmq.com/java-client.html
Other
1.25k stars 575 forks source link

Changed ClientProperty after recovery in connection.created message #861

Open mheidt opened 2 years ago

mheidt commented 2 years ago

Hello,

We are using this version

com.rabbitmq amqp-client 5.15.0

But 3.16.0 hasn't changed in the following regards.

We are facing the issue that we need a unique identifier that should change after each connection incl. automatic recovery. That's why we added this UUID into the clientProperties of the connection via ConnectionFactory. Everything works but the initial connection.created message the rabbit server is sending after a recovery. Here we see the old UUID still. We tried to change the property in the recoveryStarted call.

But when I look into the source code, we don't have any chance to do this.

AutorecoveringConnection stores params, which are not accessible from outside but in parts through a delegate that is created as RecoveryAwareAMQConnection using this params. Unfortunately they are stored as a new HashMap in AMQConnection: this._clientProperties = new HashMap(params.getClientProperties()); As well as whole as params in RecoveryAwareAMQConnectionFactory.

The AutorecoveringConnection.recoverConnection is calling the internal ConnectionFactory (not ours) to create a newConnection which is using the stored params, which is not accessible for us.

As I previously said, the first call of recoveryStart is too late for us to make any modification, because we need this changed clientProperty in the first communication (connection.created message) that is made before any channel activity.

Or is there a way to do this anyhow? In my small opinion the clientProperties of the delegate of the current connection should be used for the new connection instead of the initially stored params of the factory.

Kind regards, Markus Heidt

I know, I hate those cases as well :)

michaelklishin commented 2 years ago

If the intent is to observe a reconnection event using a property of the connection object we can introduce an "epoch" that would be incremented with every recovery and that's it. No need to mess with client properties which are not meant to be a transient store applications modify.

mheidt commented 2 years ago

Yes, to sum it up in my words: It changes for each connection/recovery and the EPOCH is accessible in the RecoveryListener methods, in the client calls and in the connection.read message as well. That would be nice, if you could provide that.

acogoluegnes commented 2 years ago

These suggestions would require some API changes. Connection has already an ID with the accessors. It's used by the MetricsCollector in newConnection. It would be worth trying to hack something, the newConnection method is called just after the connection initialization (open AMQP method). Maybe with a custom MetricsCollector that decorates calls to a (real) delegate, this way you can intercept newConnection.

mheidt commented 2 years ago

@michaelklishin Is the MetricsCollector-Delegate hack an option or are you planning to provide the EPOCH?

michaelklishin commented 2 years ago

@mheidt this is open source software. You can contribute what you need. I would not be a favor of any hacks if all we need is a counter atomically incremented between connection recoveries.

acogoluegnes commented 2 years ago

The MetricsCollector solution is a form of hack, or at least it means using an API not only in the way it is intended to be used. The epoch is a good and simple solution, but it implies giving access to the epoch somehow. The most obvious place is in the Connection interface, which means breaking the contract unless we provide a default implementation.