gmr / rabbitpy

A pure python, thread-safe, minimalistic and pythonic RabbitMQ client library
http://rabbitpy.readthedocs.org
BSD 3-Clause "New" or "Revised" License
242 stars 58 forks source link

Send heartbeat message back rabbitmq server. #99

Closed tom-railpod closed 7 years ago

tom-railpod commented 8 years ago

See NoAck Consumers get socket closed by rabbitmq due to lack of heartbeat #98

In an environment where only unack'ed data was consumed from a connection... rabbitmq-server was closing the underlying socket.

This fix sends a heartbeat message back down the channel.

codecov-io commented 8 years ago

Current coverage is 72.23%

Merging #99 into master will decrease coverage by 0.08%

@@             master        #99   diff @@
==========================================
  Files            17         17          
  Lines          1875       1887    +12   
  Methods           0          0          
  Messages          0          0          
  Branches        279        280     +1   
==========================================
+ Hits           1356       1363     +7   
- Misses          398        402     +4   
- Partials        121        122     +1   

Powered by Codecov. Last updated by 5b6d55d...8aaac16

gmr commented 7 years ago

So my first reaction to this was that the heartbeat behavior is one where the server sends the heartbeat and the client replies. I reread the spec and it's pretty ambiguous as to the specific expected behavior.

I reviewed the code in rabbitmq-common and rabbitmq-server and it met my expectation that RabbitMQ thinks it should be sending heartbeats and expecting replies:

https://github.com/rabbitmq/rabbitmq-common/blob/aafe885f8782887e1cee4a531fa5c4369df49885/src/rabbit_reader.erl#L43

So I'm curious how your problem came to be in the first place. I've not seen any issues with a client being purely responsive to heartbeat frames with RabbitMQ, as long as it is responding.

eandersson commented 7 years ago

I ran into this with my library, and after talking to the RabbitMQ devs and they agreed with the behavior described in #99. https://groups.google.com/forum/#!searchin/rabbitmq-users/Erik%7Csort:relevance/rabbitmq-users/ZD8FgpcExn4/5n0PB85DAwAJ

eandersson commented 7 years ago

My new implementation for Heartbeats never replies to RabbitMQ heartbeats, but instead sends heartbeats only if there was no outgoing traffic.

gmr commented 7 years ago

@eandersson and it's held up ok with some long duration testing pretty well?

gmr commented 7 years ago

I appreciate the PR, but I don't think it's the right approach. One main thing is rabbitpy.heartbeat.Checker already has a built-in timer. Most, if not all of the logic desired could be applied inside rabbitpy.heartbeat.Checker._check without touching the unrelated rabbitpy.channel0. I'll rework heartbeat checking to more closely match the logic discussed by @eandersson which should help in your situation.

eandersson commented 7 years ago

@gmr: Yep, not a single issue so far.