-
Notifications
You must be signed in to change notification settings - Fork 194
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
Sending 0 in heartbeat negotiation doesn't disable heartbeats #265
Comments
This should determine if heartbeats are sent. |
Right, but |
I'm not sure I understand. Need more ☕ and details. Can you please elaborate. |
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 My question is, if that's the case should that line be removed i.e do not set |
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. Correct? |
👍 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. |
If the client says it doesn't support heartbeats, then the server should respect that fact. 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. |
Based on pika/pika@3027890 and
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?
The text was updated successfully, but these errors were encountered: