search

celery / kombu

Messaging library for Python.
http://kombu.readthedocs.org/
BSD 3-Clause "New" or "Revised" License
2.88k stars 928 forks source link

ConsumerMixin no longer send heartbeat if `timeout=None` #2097

Closed vitorquintanilha closed 1 month ago

vitorquintanilha commented 2 months ago

:wave:

First I want to notice that the change in https://github.com/celery/kombu/issues/1997 is backwards incompatible, and still was release on a minor version with no breaking change warning or mention on the Changelog. This change broke all our consumers.

Second I'd like to question the logic behind the change. Why should timeout=None should cause heartbeats not to be sent? timeout=None means that we don't want to have any timeout while waiting for new messages. In this case we STILL want to send hearbeats to the broker, otherwise our connections will get killed.

Would you care to explain why it make sense that timeout=None should make the client stop sending hearbeats? What should I do if I don't want a timeout (e.g. I want my consumer to run forever, regardless of how long it takes for new messages to arrive) and I still want heartbeats to be sent so my connection is not broken?

Am I not properly understanding the API here?

Thanks!

Nusnus commented 2 months ago

@auvipy @smart-programmer @FrankK-1234 #2016

FrankK-1234 commented 2 months ago

Hello @vitorquintanilha,

After a closer look at the changes (from @smart-programmer) and the stated implied change . I have to agree it is not backwards compatible, perhaps the misunderstanding came from the name heartbeat_check. Actually I think there would be a problem if the parameter safety_interval is changed as that dictates how fast the heartbeat_check is called (it should be called about every second). So the question arise if this is the right place to implement that call.

My advice is to revert the change including the tests I had added, sorry for just assuming a right implementation. @auvipy could you make the revert if you agree? I think there might be another way to reach the original problem and that is to only call heartbeat_check when conn.heartbeat is set, but that is not up to me to decide.

As a workaround one could call the method with a high timeout (e.g. 1000000000) value which would still trigger the heartbeat check but never (well 31 years without a message) trigger the time-out raise.

With kind regards,

Frank