search

celery / py-amqp

amqplib fork
Other
309 stars 194 forks source link

Sending 0 in heartbeat negotiation doesn't disable heartbeats #265

Open anurag90x opened 5 years ago

anurag90x commented 5 years ago

Based on https://github.com/pika/pika/commit/3027890081adaa067268aa4839638a32734c263f and 

Heartbeat Timeout Value
The heartbeat timeout value defines after what period of time the peer TCP connection should be considered unreachable (down) by RabbitMQ and client libraries. This value is negotiated between the client and RabbitMQ server at the time of connection. The client must be configured to request heartbeats.

The broker and client will attempt to negotiate heartbeats by default. When both values are non-0, the lower of the requested values will be used. If one side uses a zero value (attempts to disable heartbeats) but the other does not, the non-zero value will be used.

The timeout is in seconds, and default value is 60. Older releases used 580 seconds by default.

it looks like - https://github.com/celery/py-amqp/blob/master/amqp/connection.py#L423 should be removed, since sending 0 does nothing. Does that seem reasonable?

thedrow commented 5 years ago

This should determine if heartbeats are sent.

anurag90x commented 5 years ago

Right, but self.heartbeat is being set to 0 if self.client_heartbeat == 0. So, the client will not send heartbeats but from that pika commit and the docs it looks like the server will still send heartbeats even if the client wants to disable heartbeats (by sending 0 in the tuneOk method).

thedrow commented 5 years ago

I'm not sure I understand. Need more :coffee: and details.

Can you please elaborate.

anurag90x commented 5 years ago

Ah I'm probably not explaining it properly. Essentially, there's this line -

https://github.com/celery/py-amqp/blob/master/amqp/connection.py#L421

which is saying if the client_heartbeat is set to 0 (the default, which also goes into the tuneOk method), the lib turns off heartbeats. However, according to pika/pika@3027890 and the rabbitmq documentation, even if the client sends 0 as the value in the tuneOk call, the server will still send heartbeats.

My question is, if that's the case should that line be removed i.e do not set self.heartbeat to 0 if client_heartbeat is 0. That should make it, similar to pika's implementation which is in the commit I linked.

thedrow commented 5 years ago

According to 4.2.7 in the specification if a peer does not support heartbeats it simply discards those messages without any error.

Specifying that there are no client heartbeats is allowed.

If I'm reading what you are saying correctly, RabbitMQ expects the heartbeat interval to be the non-zero value. While sensible, other implementations may choose a different method.

Correct?

anurag90x commented 5 years ago

👍 I checked the spec, so just to clarify the fact that the client is ignoring the heartbeat won't cause the connection to be terminated by the broker, right?

If that is not the case (termination of the connection) I can see the justification on a client by client basis but I think it can lead to some confusion between different clients especially since pika and the java client are both using the other logic.

thedrow commented 5 years ago

If the client says it doesn't support heartbeats, then the server should respect that fact. If it sends heartbeats anyway, the client silently ignores them.

That's not the case with RabbitMQ, it seems.

I'll accept a PR with the behavior you proposed. I think that for the time being that's the right thing to do.