RPC timeouts can cause subsequent ClassCastExceptions

[vikinghawk]

During cluster node failovers we have seen some TimeoutExceptions during connection recovery end up causing ClassCastExceptions because the reply for the timed out rpc request comes in while a 2nd rpc request is waiting.

Ideally in cases like this where the requestor has already timed out and gone away you would just throw that incoming reply away. Looking thru the code I'm not sure there is a good way to handle this though. Checking in all places that the command type is of the expected class prevents the exception, but you still have the issue that the reply delivered is not actually for the current request.

I believe it would require tagging requests with a unique id and then have the server set that request id as a correlation id on the reply allowing the client to match the reply with the original request. Would something like that be feasible in the AMQP spec?

[michaelklishin]

How can a channel implementation know if "the requestor has already timed out and gone away"?

[michaelklishin]

I'm afraid extending every protocol method with a new field is unrealistic at this point.

[vikinghawk]

How can a channel implementation know if "the requestor has already timed out and gone away"?

The AMQChannel code already is catching the TimeoutException and calls cleanRpcChannelState to get ready for the next request before rethrowing the timeout ex.

I'm afraid extending every protocol method with a new field is unrealistic at this point.

Ya thats what I was afraid of. I didn't know if AMQContentHeader could have more fields added to it passively on methods such as Queue.Declare and Queue.DeclareOk

There are still ways to make the code better however.

1. we could protect against ClassCastExceptions by verifying the class passed to RpcContinuation.handleCommand is of the expected type

2. additionally for requests/replies such as Queue.Declare/DeclareOk we could verify the queue names match before assuming the reply if for the current request.

[acogoluegnes]

We can add an option to do the check. This would then go into 4.2.0.

[acogoluegnes]

@vikinghawk @michaelklishin A PR that adapted @vikinghawk's code for 4.2.0.

[vikinghawk]

+1

[acogoluegnes]

@vikinghawk Thanks again for your contribution! You can have a try with the 4.2 snapshot.

[Inverness]

After encountering this issue myself and enabling the response type checking I got this:

2017-11-22 15:33:33 [AMQP Connection 10.5.17.63:5672] ERROR c.s.r.u.RMQDefaultExceptionHandler - Unhandled error in RabbitMQ connection
java.lang.NullPointerException: null
	at com.rabbitmq.client.impl.AMQChannel.handleCompleteInboundCommand(AMQChannel.java:185)
	at com.rabbitmq.client.impl.AMQChannel.handleFrame(AMQChannel.java:111)
	at com.rabbitmq.client.impl.AMQConnection.readFrame(AMQConnection.java:643)
	at com.rabbitmq.client.impl.AMQConnection.access$300(AMQConnection.java:47)
	at com.rabbitmq.client.impl.AMQConnection$MainLoop.run(AMQConnection.java:581)
	at java.lang.Thread.run(Thread.java:748)

The added code does not check if _activeRpc is null before calling canHandleReply()

[acogoluegnes]

@Inverness Thanks for pointing this out, 5ff6ad4 fixes this, it will go in to 4.4.0 (which will be released in the next few days).

[Inverness]

@acogoluegnes I'm currently using 5.0.0. When will the fix be available for that branch?

[michaelklishin]

Reasonably soon.